Bug 25136 - CRASH in DocumentLoader::removeSubresourceLoader due to null m_frame
: CRASH in DocumentLoader::removeSubresourceLoader due to null m_frame
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Page Loading
: 528+ (Nightly build)
: All All
: P1 Critical
Assigned To: Darin Fisher (:fishd, Google)
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-10 16:16 PDT by Darin Fisher (:fishd, Google)
Modified: 2009-04-21 07:19 PDT (History)
1 user (show)

See Also:


Attachments
testcase - frame.html (215 bytes, text/html)
2009-04-10 16:18 PDT, Darin Fisher (:fishd, Google)
no flags Details
testcase - careful, this may crash your browser (89 bytes, text/html)
2009-04-10 16:19 PDT, Darin Fisher (:fishd, Google)
no flags Details
patch v1 - simplistic null check (1.01 KB, patch)
2009-04-10 17:09 PDT, Darin Fisher (:fishd, Google)
darin: review+
Details | Formatted Diff | Diff
patch v1 + layout test (5.47 KB, patch)
2009-04-13 10:35 PDT, Darin Fisher (:fishd, Google)
no flags Details | Formatted Diff | Diff
patch v1 + layout test (5.13 KB, patch)
2009-04-13 10:37 PDT, Darin Fisher (:fishd, Google)
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-10 16:16:59 PDT
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 Darin Fisher (:fishd, Google) 2009-04-10 16:18:57 PDT
Created attachment 29406 [details]
testcase - frame.html
Comment 2 Darin Fisher (:fishd, Google) 2009-04-10 16:19:57 PDT
Created attachment 29407 [details]
testcase - careful, this may crash your browser
Comment 3 Darin Fisher (:fishd, Google) 2009-04-10 17:09:10 PDT
Created attachment 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 Darin Adler 2009-04-10 17:12:39 PDT
Comment on attachment 29412 [details]
patch v1 - simplistic null check

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 Darin Adler 2009-04-10 17:13:55 PDT
(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 Darin Fisher (:fishd, Google) 2009-04-10 19:29:08 PDT
> Per-function comment in the ChangeLog I mean.

Will do.  I think I figured out how to write a stable test :)
Comment 7 Darin Fisher (:fishd, Google) 2009-04-13 10:35:47 PDT
Created attachment 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 Darin Fisher (:fishd, Google) 2009-04-13 10:37:49 PDT
Created attachment 29430 [details]
patch v1 + layout test

Take 2.  This time with commented out code snippets removed.
Comment 9 Darin Adler 2009-04-13 10:40:21 PDT
Comment on attachment 29430 [details]
patch v1 + layout test

r=me
Comment 10 Darin Fisher (:fishd, Google) 2009-04-13 10:48:25 PDT
Landed as:  http://trac.webkit.org/changeset/42442
Comment 11 David Harrison 2009-04-21 07:19:51 PDT
fwiw this is also rdar://6748719