Bug 25136 - CRASH in DocumentLoader::removeSubresourceLoader due to null m_frame
: CRASH in DocumentLoader::removeSubresourceLoader due to null m_frame
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: All All
: P1 Critical
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2009-04-10 16:16 PST by
Modified: 2009-04-21 07:19 PST (History)


Attachments
testcase - frame.html (215 bytes, text/html)
2009-04-10 16:18 PST, Darin Fisher (:fishd, Google)
no flags Details
testcase - careful, this may crash your browser (89 bytes, text/html)
2009-04-10 16:19 PST, Darin Fisher (:fishd, Google)
no flags Details
patch v1 - simplistic null check (1.01 KB, patch)
2009-04-10 17:09 PST, Darin Fisher (:fishd, Google)
darin: review+
Review Patch | Details | Formatted Diff | Diff
patch v1 + layout test (5.47 KB, patch)
2009-04-13 10:35 PST, Darin Fisher (:fishd, Google)
no flags Review Patch | Details | Formatted Diff | Diff
patch v1 + layout test (5.13 KB, patch)
2009-04-13 10:37 PST, Darin Fisher (:fishd, Google)
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-04-10 16:16:59 PST
CRASH in DocumentLoader::removeSubresourceLoader due to null m_frame

This started appearing as a top-crash in a recent Chrome dev channel build:
http://code.google.com/p/chromium/issues/detail?id=9944

Sample crash stack:

[chrome.dll     - frameloader.cpp:2670]                 WebCore::FrameLoader::activeDocumentLoader()
[chrome.dll     - frameloader.cpp:2677]                 WebCore::FrameLoader::isLoading()
[chrome.dll     - documentloader.cpp:768]               WebCore::DocumentLoader::removeSubresourceLoader(WebCore::ResourceLoader *)
[chrome.dll     - subresourceloader.cpp:225]             WebCore::SubresourceLoader::didCancel(WebCore::ResourceError const &)
[chrome.dll     - resourceloader.cpp:359]             WebCore::ResourceLoader::cancel(WebCore::ResourceError const &)
[chrome.dll     - resourceloader.cpp:349]             WebCore::ResourceLoader::cancel()
[chrome.dll     - documentthreadableloader.cpp:109]     WebCore::DocumentThreadableLoader::cancel()
[chrome.dll     - xmlhttprequest.cpp:709]             WebCore::XMLHttpRequest::internalAbort()
[chrome.dll     - xmlhttprequest.cpp:1182]             WebCore::XMLHttpRequest::stop()
[chrome.dll     - scriptexecutioncontext.cpp:156]     WebCore::ScriptExecutionContext::stopActiveDOMObjects()
[chrome.dll     - document.cpp:1310]                     WebCore::Document::detach()
[chrome.dll     - frame.cpp:232]                     WebCore::Frame::setView(WebCore::FrameView *)
[chrome.dll     - frameloader.cpp:3434]             WebCore::FrameLoader::closeAndRemoveChild(WebCore::Frame *)
[chrome.dll     - frameloader.cpp:3516]             WebCore::FrameLoader::detachFromParent()
[chrome.dll     - frameloader.cpp:3426]             WebCore::FrameLoader::detachChildren()

It appears that this crash is only possible if a ResourceLoader starts after FrameLoader::stopAllLoaders has been called.  I found a way to do that via an XMLHttpRequest onabort handler.  Test case coming up...
------- Comment #1 From 2009-04-10 16:18:57 PST -------
Created an attachment (id=29406) [details]
testcase - frame.html
------- Comment #2 From 2009-04-10 16:19:57 PST -------
Created an attachment (id=29407) [details]
testcase - careful, this may crash your browser
------- Comment #3 From 2009-04-10 17:09:10 PST -------
Created an attachment (id=29412) [details]
patch v1 - simplistic null check

This is a very simple minded solution to the crash.  Other parts of DocumentLoader are fairly careful to null test m_frame before using it, and indeed removeSubresourceLoader null checks m_frame before using it (except in the case here where it calls updateLoading).

I have been struggling to get a good layout test for this.  For some reason my layout test appears to be rather flakey so I don't want to submit it just yet.  I'll keep working on it.  I just wanted to share this patch for feedback.
------- Comment #4 From 2009-04-10 17:12:39 PST -------
(From update of attachment 29412 [details])
r=me

If you can figure out how to do a regression test, that would be a great addition. Also, it's much better when there are per-function comments, even for a simple function like this one.
------- Comment #5 From 2009-04-10 17:13:55 PST -------
(In reply to comment #4)
> Also, it's much better when there are per-function comments, even for
> a simple function like this one.

Per-function comment in the ChangeLog I mean.
------- Comment #6 From 2009-04-10 19:29:08 PST -------
> Per-function comment in the ChangeLog I mean.

Will do.  I think I figured out how to write a stable test :)
------- Comment #7 From 2009-04-13 10:35:47 PST -------
Created an attachment (id=29429) [details]
patch v1 + layout test

This is the same patch as before but now with a layout test.  The test requires subframes, so there are a couple resource files included.
------- Comment #8 From 2009-04-13 10:37:49 PST -------
Created an attachment (id=29430) [details]
patch v1 + layout test

Take 2.  This time with commented out code snippets removed.
------- Comment #9 From 2009-04-13 10:40:21 PST -------
(From update of attachment 29430 [details])
r=me
------- Comment #10 From 2009-04-13 10:48:25 PST -------
Landed as:  http://trac.webkit.org/changeset/42442
------- Comment #11 From 2009-04-21 07:19:51 PST -------
fwiw this is also rdar://6748719