Bug 25394 - REGRESSION: crash in DocumentLoader::addResponse due to bad |this| pointer
Summary: REGRESSION: crash in DocumentLoader::addResponse due to bad |this| pointer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P1 Normal
Assignee: David Levin
URL:
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2009-04-25 09:08 PDT by Darin Fisher (:fishd, Google)
Modified: 2009-05-13 12:17 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 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>
Comment 1 Darin Fisher (:fishd, Google) 2009-04-25 09:26:31 PDT
The corresponding chromium bug report:
http://code.google.com/p/chromium/issues/detail?id=9947
Comment 2 Darin Fisher (:fishd, Google) 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.
Comment 3 Darin Fisher (:fishd, Google) 2009-04-26 00:23:13 PDT
Created attachment 29800 [details]
testcase1 - inner frame
Comment 4 Darin Fisher (:fishd, Google) 2009-04-26 00:24:25 PDT
Created attachment 29801 [details]
testcase1 - outer frame (load this to run test)
Comment 5 Darin Fisher (:fishd, Google) 2009-04-26 00:26:12 PDT
This crashes both Safari 4 and Chrome 2.  By contrast, Safari 3 and Chrome 1 are fine.
Comment 6 Darin Fisher (:fishd, Google) 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?
Comment 7 Alexey Proskuryakov 2009-04-26 06:23:17 PDT
Is onabort dispatched asynchronously? XMLHttpRequest used to not start from onunload, see bug 10904.
Comment 8 Darin Fisher (:fishd, Google) 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.
Comment 9 Alexey Proskuryakov 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.
Comment 10 Darin Fisher (:fishd, Google) 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.
Comment 11 Alexey Proskuryakov 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;

Comment 12 Darin Fisher (:fishd, Google) 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?
Comment 13 Darin Fisher (:fishd, Google) 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...
Comment 14 David Levin 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.
Comment 15 Darin Adler 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.
Comment 16 Darin Adler 2009-05-13 06:22:54 PDT
Comment on attachment 30263 [details]
proposed fix.

Fix looks good, by the way.
Comment 17 David Levin 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.
Comment 18 Darin Fisher (:fishd, Google) 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.
Comment 19 Darin Fisher (:fishd, Google) 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.
Comment 20 David Levin 2009-05-13 10:52:47 PDT
Created attachment 30280 [details]
Proposed fix with layout test.
Comment 21 David Levin 2009-05-13 12:17:06 PDT
Committed http://trac.webkit.org/changeset/43650.