[webkit-reviews] review granted: [Bug 31633] Need to ensure that Document::postTask does not provide the Task with a dangling pointer to destroyed Document : [Attachment 46531] Patch.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dmitry Titov <dimich at chromium.org>'s
request for review:
Bug 31633: Need to ensure that Document::postTask does not provide the Task
with a dangling pointer to destroyed Document
https://bugs.webkit.org/show_bug.cgi?id=31633

Attachment 46531: Patch.
https://bugs.webkit.org/attachment.cgi?id=46531&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +DocumentWeakReference::DocumentWeakReference(Document* doc)
> +    : m_document(doc)
> +{
> +    ASSERT(isMainThread());
> +}
> +
> +Document* DocumentWeakReference::document()
> +{
> +    ASSERT(isMainThread());
> +    return m_document;
> +}
> +
> +void DocumentWeakReference::clear()
> +{
> +    ASSERT(isMainThread());
> +    m_document = 0;
> +}

These super-short functions seem like they ought to be inlined, unless there is
a reason not to. If they are only used within this file, then I suggest just
marking them inline. If they are used elsewhere we could consider putting them
in the header. Perhaps it's the main thread bit that can't be easily be put in
the header?

It's a bit strange to have this class's function definitions appear in the file
above the definition of the global variable, documentsThatNeedStyleRecalc. I'd
prefer they be further down. Maybe even at the end of the file instead of the
start.

Argument name should be "document", not "doc".

> +    if (m_weakReference)
> +	   m_weakReference->clear();

No need for the null check here.

> +    PerformTaskContext(PassRefPtr<DocumentWeakReference> documentWeakRef,
PassOwnPtr<ScriptExecutionContext::Task> task)
> +	   : documentWeakRef(documentWeakRef)
>	   , task(task)
>      {
>      }

I suggest naming the argument "document" or "documentReference" instead of
documentWeakRef.

> +    RefPtr<DocumentWeakReference> documentWeakRef;

I think you should name data member "document" or "documentReference"; no need
to repeat the word "weak" or to abbreviate "reference".

> +    if (Document* doc = ptctx->documentWeakRef->document())
> +	   ptctx->task->performTask(doc);

Please use the name "document" instead of "doc" here for the local variable.

I also wise the variable "ptctx" was instead named "context".

> +	   ASSERT(m_weakReference);
> +	   callOnMainThread(performTask, new
PerformTaskContext(m_weakReference, task));

I don't think you need this assertion.

> +    static PassRefPtr<DocumentWeakReference> create(Document* doc)
> +    {
> +	   return adoptRef(new DocumentWeakReference(doc));
> +    }

Please use the argument name "document" instead of "doc".

I'm going to say review+ because all my comments are essentially stylistic
ones; please consider addressing them anyway. I'm disappointed we can't test
it!