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+

Hajime Morrita
Reported 2011-06-22 18:45:05 PDT
From http://code.google.com/p/chromium/issues/detail?id=84774.
Attachments
Patch (1.22 KB, patch)
2011-06-23 01:53 PDT, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Patch (2.88 KB, patch)
2011-06-24 00:32 PDT, Hajime Morrita 		tkent: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Hajime Morrita
Comment 1 2011-06-23 01:53:36 PDT
Created attachment 98324 [details] Patch
Hajime Morrita
Comment 2 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.
Kent Tamura
Comment 3 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>.
Kent Tamura
Comment 4 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).
Hajime Morrita
Comment 5 2011-06-24 00:32:11 PDT
Created attachment 98467 [details] Patch
Hajime Morrita
Comment 6 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.
Kent Tamura
Comment 7 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.
Tony Chang
Comment 8 2011-06-24 14:56:22 PDT
Committed http://trac.webkit.org/changeset/89663 .
Hajime Morrita
Comment 9 2011-06-27 00:33:38 PDT
Thanks you for updating this, Tony. It looks I forgot to do it.
Note You need to log in before you can comment on or make changes to this bug.