Bug 53899 – [Chromium][DRT] some PostDelayedTask() accesses possibly dangling this pointer.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Hajime Morrita]

In WebViewHost, there are several calls like: 
    webkit_support::PostDelayedTask(invokeFinishLastTextCheck, static_cast<void*>(this), 0);
But this is not safe when |this| object is deleted before the task callback is invoked.

Pointed at Bug 51013.

[Kent Tamura]

(In reply to comment #0)
> In WebViewHost, there are several calls like: 
>     webkit_support::PostDelayedTask(invokeFinishLastTextCheck, static_cast<void*>(this), 0);
> But this is not safe when |this| object is deleted before the task callback is invoked.

Yes.  In that case, we had better use DumpRenderTree/chromium/Taks.h instead of the bare PostDelayedTask().

[Hajime Morrita]

Created attachment 82766 [details]
Patch

[Kent Tamura]

Comment on attachment 82766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82766&action=review

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:419
> +    postDelayedTask(new WebViewHost::HostMethodTask(this, &WebViewHost::finishLastTextCheck), 0);

"WebViewHost::" looks redundant.

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:662
> +    postDelayedTask(new WebViewHost::HostMethodTask(this, &WebViewHost::scheduleComposite), 0);

ditto.

> Tools/DumpRenderTree/chromium/WebViewHost.cpp:709
> +    postDelayedTask(new WebViewHost::HostMethodTask(this, &WebViewHost::closeWidget), 0);

ditto.

> Tools/DumpRenderTree/chromium/WebViewHost.h:70
> +    class HostMethodTask : public MethodTask<WebViewHost> {

Do we need to make this class public?

> Tools/DumpRenderTree/chromium/WebViewHost.h:76
> +        {}

nit: We usually use "{ }" (a blank between {}).

[Hajime Morrita]

Created attachment 82938 [details]
Patch

[Hajime Morrita]

Hi Kent-san, thank you for reviewing!
I updated the patch.

(In reply to comment #3)
> (From update of attachment 82766 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82766&action=review
> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:419
> > +    postDelayedTask(new WebViewHost::HostMethodTask(this, &WebViewHost::finishLastTextCheck), 0);
> 
> "WebViewHost::" looks redundant.
Sure. removed.

> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:662
> > +    postDelayedTask(new WebViewHost::HostMethodTask(this, &WebViewHost::scheduleComposite), 0);
> 
> ditto.
Done.

> 
> > Tools/DumpRenderTree/chromium/WebViewHost.cpp:709
> > +    postDelayedTask(new WebViewHost::HostMethodTask(this, &WebViewHost::closeWidget), 0);
> 
> ditto.
Done.

> 
> > Tools/DumpRenderTree/chromium/WebViewHost.h:70
> > +    class HostMethodTask : public MethodTask<WebViewHost> {
> 
> Do we need to make this class public?
Good point. Moved to private.

> 
> > Tools/DumpRenderTree/chromium/WebViewHost.h:76
> > +        {}
> 
> nit: We usually use "{ }" (a blank between {}).
Done.

[Hajime Morrita]

Committed r78984: <http://trac.webkit.org/changeset/78984>