WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
25394
REGRESSION: crash in DocumentLoader::addResponse due to bad |this| pointer
https://bugs.webkit.org/show_bug.cgi?id=25394
Summary
REGRESSION: crash in DocumentLoader::addResponse due to bad |this| pointer
Darin Fisher (:fishd, Google)
Reported
2009-04-25 09:08:49 PDT
CRASH in DocumentLoader::addResponse due to bad |this| pointer This is possibly a regression since we have started to see it show up with greater frequency in our automated reliability testing as well as in end-user crash reports. Here's the relevant portion of the stack: chrome_2390000!WebCore::DocumentLoader::addResponse+0x3 [c:\b\slave\chromium-rel-xp\build\src\third_party\webkit\webcore\loader\documentloader.cpp @ 670] chrome_2390000!WebCore::FrameLoader::didReceiveResponse+0x1f [c:\b\slave\chromium-rel-xp\build\src\third_party\webkit\webcore\loader\frameloader.cpp @ 3729] chrome_2390000!WebCore::ResourceLoader::didReceiveResponse+0x8c [c:\b\slave\chromium-rel-xp\build\src\third_party\webkit\webcore\loader\resourceloader.cpp @ 243] chrome_2390000!WebCore::SubresourceLoader::didReceiveResponse+0x80 [c:\b\slave\chromium-rel-xp\build\src\third_party\webkit\webcore\loader\subresourceloader.cpp @ 144] chrome_2390000!WebCore::ResourceLoader::didReceiveResponse+0xe [c:\b\slave\chromium-rel-xp\build\src\third_party\webkit\webcore\loader\resourceloader.cpp @ 407] chrome_2390000!WebCore::ResourceHandleInternal::OnReceivedResponse+0xc0 [c:\b\slave\chromium-rel-xp\build\src\webkit\glue\resource_handle_impl.cc @ 547] chrome_2390000!ResourceDispatcher::OnReceivedResponse+0x93 [c:\b\slave\chromium-rel-xp\build\src\chrome\common\resource_dispatcher.cc @ 368] <snip... called from an incoming IPC message>
Attachments
testcase1 - inner frame
(329 bytes, text/html)
2009-04-26 00:23 PDT
,
Darin Fisher (:fishd, Google)
no flags
Details
testcase1 - outer frame (load this to run test)
(129 bytes, text/html)
2009-04-26 00:24 PDT
,
Darin Fisher (:fishd, Google)
no flags
Details
proposed fix.
(1012 bytes, patch)
2009-05-13 00:14 PDT
,
David Levin
no flags
Details
Formatted Diff
Diff
Proposed fix with layout test.
(5.31 KB, patch)
2009-05-13 10:52 PDT
,
David Levin
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Darin Fisher (:fishd, Google)
Comment 1
2009-04-25 09:26:31 PDT
The corresponding chromium bug report:
http://code.google.com/p/chromium/issues/detail?id=9947
Darin Fisher (:fishd, Google)
Comment 2
2009-04-26 00:19:42 PDT
This turns out to be a variant of another crash I recently patched. See
bug 25136
. If a subresource like an IMG is requested after 'unload' but before the next page load completes, it can result in this crash. The IMG load has to complete very quickly, which can be synthesized by loading a data URL. Test case coming up.
Darin Fisher (:fishd, Google)
Comment 3
2009-04-26 00:23:13 PDT
Created
attachment 29800
[details]
testcase1 - inner frame
Darin Fisher (:fishd, Google)
Comment 4
2009-04-26 00:24:25 PDT
Created
attachment 29801
[details]
testcase1 - outer frame (load this to run test)
Darin Fisher (:fishd, Google)
Comment 5
2009-04-26 00:26:12 PDT
This crashes both Safari 4 and Chrome 2. By contrast, Safari 3 and Chrome 1 are fine.
Darin Fisher (:fishd, Google)
Comment 6
2009-04-26 00:41:41 PDT
We could probably hack around this crash, but I think there is a bigger problem at play here. It seems like we should not allow any resource requests to begin after the closeURL() call in FrameLoader::detachFromParent(). For reference: The new resource requests are generated from within stopAllLoaders. There may be more than one way to achieve this, but the trick I am using is to define an onabort handler on a XMLHttpRequest that I start in my unload handler. The onabort handler then starts another resource load. That resource load should not be allowed. Or, maybe we should not even fire event handlers at all after unload. Thoughts?
Alexey Proskuryakov
Comment 7
2009-04-26 06:23:17 PDT
Is onabort dispatched asynchronously? XMLHttpRequest used to not start from onunload, see
bug 10904
.
Darin Fisher (:fishd, Google)
Comment 8
2009-04-26 07:14:02 PDT
> Is onabort dispatched asynchronously? XMLHttpRequest used to not start from > onunload, see
bug 10904
.
onabort is dispatched synchronously. that bug is a little vague. i don't have any trouble starting an XHR from unload. what happens is that the request is aborted immediately afterwards when the containing IFRAME object is removed from the DOM. this is why my testcase involves an inner frame.
Alexey Proskuryakov
Comment 9
2009-04-26 07:43:21 PDT
(In reply to
comment #8
)
> onabort is dispatched synchronously. that bug is a little vague. i don't have > any trouble starting an XHR from unload.
I see, it changed since that bug was filed - the loader refused to start loading then.
Darin Fisher (:fishd, Google)
Comment 10
2009-05-12 12:12:08 PDT
> I see, it changed since that bug was filed - the loader refused to start > loading then.
I think the right fix here is to change the loader to refuse to start loading after unload. The trick is to find out when to allow loading to start again. I think we have to allow navigations of the frame itself, and we have to re-enable subresource loads once we transition to the committed state.
Alexey Proskuryakov
Comment 11
2009-05-12 12:14:49 PDT
Also note this comment in ResourceHandleMac.mm: // If we are no longer attached to a Page, this must be an attempted load from an // onUnload handler, so let's just block it. Page* page = frame->page(); if (!page) return false;
Darin Fisher (:fishd, Google)
Comment 12
2009-05-12 13:01:15 PDT
> Also note this comment in ResourceHandleMac.mm:
Interesting. I guess it is OK that ResourceHandleMac still allows synchronous XHR. We can easily implement this same fix in the other ResourceHandle implementations. Does that sound like the right solution to you?
Darin Fisher (:fishd, Google)
Comment 13
2009-05-12 13:05:34 PDT
Hmm... I just ran my testcase using Safari 4 on Mac, and it also crashes. Maybe there is more that we need to do...
David Levin
Comment 14
2009-05-13 00:14:39 PDT
Created
attachment 30263
[details]
proposed fix. Working on converting the attached files into a layout test before submitting for review.
Darin Adler
Comment 15
2009-05-13 06:22:32 PDT
Comment on
attachment 30263
[details]
proposed fix.
> if (!skipCanLoadCheck > - && FrameLoader::restrictAccessToLocal() > - && !FrameLoader::canLoad(request.url(), String(), frame->document())) { > + && FrameLoader::restrictAccessToLocal() > + && !FrameLoader::canLoad(request.url(), String(), frame->document())) { > FrameLoader::reportLocalLoadFailed(frame, request.url().string()); > return 0; > }
This code is intentionally indented this way, so the if statement conditions don't line up with the if statement body. Please don't reformat it. We could discuss preferred style for cases like this and put it in the style guide document, but in the mean time it would be better not to make this change, which is unrelated to your fix. If the formatting really does bother you and you want to sidestep it in another way, you can restructure the code so we don't have a long if statement like this, perhaps by introducing another boolean.
Darin Adler
Comment 16
2009-05-13 06:22:54 PDT
Comment on
attachment 30263
[details]
proposed fix. Fix looks good, by the way.
David Levin
Comment 17
2009-05-13 07:40:18 PDT
Re: formatting: No problem. I'll undo the formatting change when I add the patch with the layout test. I simply thought it was a mistake. Thanks for the info.
Darin Fisher (:fishd, Google)
Comment 18
2009-05-13 08:40:50 PDT
It looks like m_isStopping will be set to true when FrameLoader::stopAllLoaders is called. That gets called when the user hits the stop button and/or initiates a download on a page. In those cases, we don't unload the page, and we don't want to suppress further subresource loads. That said, I haven't tested this... but I worry the change could be wrong.
Darin Fisher (:fishd, Google)
Comment 19
2009-05-13 08:42:28 PDT
> That said, I haven't tested this... but I worry the change could be wrong.
Doh, I realize now that m_isStopping is toggled back to false a little while later. Then my concerns are ill founded. Sorry for the spam.
David Levin
Comment 20
2009-05-13 10:52:47 PDT
Created
attachment 30280
[details]
Proposed fix with layout test.
David Levin
Comment 21
2009-05-13 12:17:06 PDT
Committed
http://trac.webkit.org/changeset/43650
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug