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
testcase1 - outer frame (load this to run test) (129 bytes, text/html)
2009-04-26 00:24 PDT, Darin Fisher (:fishd, Google)
no flags
proposed fix. (1012 bytes, patch)
2009-05-13 00:14 PDT, David Levin
no flags
Proposed fix with layout test. (5.31 KB, patch)
2009-05-13 10:52 PDT, David Levin
darin: review+
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
Note You need to log in before you can comment on or make changes to this bug.