Bug 63216 - [Chromium] Invalid write inside WebKit::FrameLoaderClientImpl::dispatchDidClearWindowObjectInWorld
Summary: [Chromium] Invalid write inside WebKit::FrameLoaderClientImpl::dispatchDidCle...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-22 18:45 PDT by Hajime Morrita
Modified: 2011-06-27 00:33 PDT (History)
2 users (show)

See Also:

Attachments
Patch (1.22 KB, patch)
2011-06-23 01:53 PDT, Hajime Morrita 		no flags Details | Formatted Diff | Diff
Patch (2.88 KB, patch)
2011-06-24 00:32 PDT, Hajime Morrita 		tkent: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.