Bug 25394 – REGRESSION: crash in DocumentLoader::addResponse due to bad |this| pointer

• Component: Page Loading
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

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>

[Darin Fisher (:fishd, Google)]

The corresponding chromium bug report:
http://code.google.com/p/chromium/issues/detail?id=9947

[Darin Fisher (:fishd, Google)]

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)]

Created attachment 29800 [details]
testcase1 - inner frame

[Darin Fisher (:fishd, Google)]

Created attachment 29801 [details]
testcase1 - outer frame (load this to run test)

[Darin Fisher (:fishd, Google)]

This crashes both Safari 4 and Chrome 2.  By contrast, Safari 3 and Chrome 1 are fine.

[Darin Fisher (:fishd, Google)]

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]

Is onabort dispatched asynchronously? XMLHttpRequest used to not start from onunload, see bug 10904.

[Darin Fisher (:fishd, Google)]

> 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]

(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)]

> 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]

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)]

> 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)]

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]

Created attachment 30263 [details]
proposed fix.

Working on converting the attached files into a layout test before submitting for review.

[Darin Adler]

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 on attachment 30263 [details]
proposed fix.

Fix looks good, by the way.

[David Levin]

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)]

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)]

> 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]

Created attachment 30280 [details]
Proposed fix with layout test.

[David Levin]

Committed http://trac.webkit.org/changeset/43650.