Bug 53899

Summary: [Chromium][DRT] some PostDelayedTask() accesses possibly dangling this pointer.
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: fishd, tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch tkent: review+

Description Hajime Morrita 2011-02-06 18:43:14 PST
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.
Comment 1 Kent Tamura 2011-02-09 00:51:41 PST
(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().
Comment 2 Hajime Morrita 2011-02-17 01:09:26 PST
Created attachment 82766 [details]
Patch
Comment 3 Kent Tamura 2011-02-17 01:17:17 PST
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 {}).
Comment 4 Hajime Morrita 2011-02-18 02:33:50 PST
Created attachment 82938 [details]
Patch
Comment 5 Hajime Morrita 2011-02-18 02:38:18 PST
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.
Comment 6 Hajime Morrita 2011-02-18 02:45:30 PST
Committed r78984: <http://trac.webkit.org/changeset/78984>