CRASH in DocumentLoader::removeSubresourceLoader due to null m_frame

• Component: Page Loading
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

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

[Darin Fisher (:fishd, Google)]

Created attachment 29406 [details]
testcase - frame.html

[Darin Fisher (:fishd, Google)]

Created attachment 29407 [details]
testcase - careful, this may crash your browser

[Darin Fisher (:fishd, Google)]

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.

[Darin Adler]

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.

[Darin Adler]

(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.

[Darin Fisher (:fishd, Google)]

> Per-function comment in the ChangeLog I mean.

Will do.  I think I figured out how to write a stable test :)

[Darin Fisher (:fishd, Google)]

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.

[Darin Fisher (:fishd, Google)]

Created attachment 29430 [details]
patch v1 + layout test

Take 2.  This time with commented out code snippets removed.

[Darin Adler]

Comment on attachment 29430 [details]
patch v1 + layout test

r=me

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/42442

[David Harrison]

fwiw this is also rdar://6748719