Bug 63216

Summary: [Chromium] Invalid write inside WebKit::FrameLoaderClientImpl::dispatchDidClearWindowObjectInWorld
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: PlatformAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: tkent, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch tkent: review+

Description Hajime Morrita 2011-06-22 18:45:05 PDT
From http://code.google.com/p/chromium/issues/detail?id=84774.
Comment 1 Hajime Morrita 2011-06-23 01:53:36 PDT
Created attachment 98324 [details]
Patch
Comment 2 Hajime Morrita 2011-06-23 01:56:30 PDT
This looks same. http://code.google.com/p/chromium/issues/detail?id=86808

I suspect this change might cause another ASAN error, but this looks obvious leak.
So I'd like to see what happens with this change.
Comment 3 Kent Tamura 2011-06-23 02:12:52 PDT
Comment on attachment 98324 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestShell.cpp:153
> +    delete m_webViewHost;

Raw new&delete are not good.
We had better make m_webViewHost OwnPtr<WebViewHost>, and TestShell::createNewWindow() should return PassOwnPtr<WebViewHost>.
Comment 4 Kent Tamura 2011-06-23 02:19:02 PDT
Comment on attachment 98324 [details]
Patch

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

>> Tools/DumpRenderTree/chromium/TestShell.cpp:153
>> +    delete m_webViewHost;
> 
> Raw new&delete are not good.
> We had better make m_webViewHost OwnPtr<WebViewHost>, and TestShell::createNewWindow() should return PassOwnPtr<WebViewHost>.

Changing createNewWindow() might be complex.  So, just making m_webViewHost OwnPtr<WebViewHost> is enough.
Note that we can't do closeWindow(m_webViewHost).
Comment 5 Hajime Morrita 2011-06-24 00:32:11 PDT
Created attachment 98467 [details]
Patch
Comment 6 Hajime Morrita 2011-06-24 00:34:47 PDT
Kent-san, thank you for taking a look!

> Changing createNewWindow() might be complex.  So, just making m_webViewHost OwnPtr<WebViewHost> is enough.
> Note that we can't do closeWindow(m_webViewHost).
Sure. I did it on the updated patch.

> Changing createNewWindow() might be complex.  So, just making m_webViewHost OwnPtr<WebViewHost> is enough.
> Note that we can't do closeWindow(m_webViewHost).
Ah, I didn't notice that...
Fortunately, there is no such call at this time.
Comment 7 Kent Tamura 2011-06-24 03:35:11 PDT
Comment on attachment 98467 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestShell.cpp:153
> +    m_webViewHost.clear();

No need to call clear() explicitly. ~OwnPtr() is called automatically.
Comment 8 Tony Chang 2011-06-24 14:56:22 PDT
Committed http://trac.webkit.org/changeset/89663 .
Comment 9 Hajime Morrita 2011-06-27 00:33:38 PDT
Thanks you for updating this, Tony.
It looks I forgot to do it.