Bug 23736 – WebKit Crashes on http://g-conquest.fr/~server2

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jon@Chromium]

Although this was found in Chromium it also crashes the nightly WebKit at third_party\webkit\webcore\loader\docloader.cpp @ 280

It is easy to repro.  Open the URL with the nightly build and wait a little while.

[Mark Rowe (bdash)]

Can you please attach a crash log when reporting crashes?  See <http://webkit.org/quality/crashlogs.html> for details.

[Jon@Chromium]

Created attachment 27323 [details]
Crash Log

Sorry about not including the crash log immediately.  I was moving a little too quickly.  :)

[Alexey Proskuryakov]

On a debug build, I'm getting an assertion failure:

ASSERTION FAILED: !m_deletionHasBegun
(/Users/ap/Safari/OpenSource/WebCore/dom/Document.h:197 void WebCore::Document::selfOnlyRef())

[Alexey Proskuryakov]

<rdar://problem/6560278>

[Mark Rowe (bdash)]

*** Bug 23807 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

The ASSERT is hit because
WebCore::Loader::Host::didFinishLoading
is trying to put docLoader->doc() in a DocPtr and the document is already dead!

    DocLoader* docLoader = request->docLoader();
    // Prevent the document from being destroyed before we are done with
    // the docLoader that it will delete when the document gets deleted.
    DocPtr<Document> protector(docLoader->doc());

I guess requests don't keep documents alive?  But I'm slightly surprised that all requests aren't correctly canceled when a document goes away.  Honestly I don't know much about the loader machinery yet.

Here is the interesting part of the stacktrace leading to the ASSERT:

#0	0x035d686b in WebCore::Document::selfOnlyRef at Document.h:197
#1	0x035d68b3 in WebCore::DocPtr<WebCore::Document>::DocPtr at DocPtr.h:30
#2	0x03d809e6 in WebCore::Loader::Host::didFinishLoading at loader.cpp:292
#3	0x03cd911c in WebCore::SubresourceLoader::didFinishLoading at SubresourceLoader.cpp:183
#4	0x03bba11e in WebCore::ResourceLoader::didFinishLoading at ResourceLoader.cpp:416
#5	0x03bb7c30 in -[WebCoreResourceHandleAsDelegate connectionDidFinishLoading:] at ResourceHandleMac.mm:603
#6	0x91b9fcd7 in -[NSURLConnection(NSURLConnectionReallyInternal) sendDidFinishLoading]
#7	0x91b9fc43 in _NSURLConnectionDidFinishLoading
#8	0x94f331f4 in URLConnectionClient::clientDidFinishLoading
#9	0x94f31d31 in URLConnectionClient::ClientConnectionEventQueue::processAllEventsAndConsumePayload
#10	0x94f32d70 in URLConnectionClient::processEvents

[Eric Seidel (no email)]

Documents own their DocLoaders, which would suggest that when we hit this code the DocLoader is also destroyed.

Loader::cancelRequests(DocLoader*) should have already taken care of removing this request from the RequestMap though...

Loader::cancelRequests is called by:
    if (Document* doc = m_frame->document()) {
        if (DocLoader* docLoader = doc->docLoader())
            cache()->loader()->cancelRequests(docLoader);

From within void FrameLoader::stopLoading(bool sendUnload)

It's a little odd that you can delete a document and not necessarily have stopped all the requests by it... investigating.

[Eric Seidel (no email)]

I added an ASSERT to ~DocLoader(), which we now hit:

DocLoader::~DocLoader()
{
    clearPreloads();
    DocumentResourceMap::iterator end = m_documentResources.end();
    for (DocumentResourceMap::iterator it = m_documentResources.begin(); it != end; ++it)
        it->second->setDocLoader(0);
    m_cache->removeDocLoader(this);

    // Make sure no requests still point to this DocLoader
    ASSERT(m_requestCount == 0);
}

m_requestCount == 1.  The stack trace of hitting the ASSERT is:
#0	0x036ba6d2 in WebCore::DocLoader::~DocLoader at DocLoader.cpp:70
#1	0x036c8d12 in WebCore::Document::~Document at Document.cpp:436
#2	0x037b697f in WebCore::HTMLDocument::~HTMLDocument at HTMLDocument.cpp:91
#3	0x035d68ab in WebCore::Document::selfOnlyDeref at Document.h:208
#4	0x035d68d1 in WebCore::DocPtr<WebCore::Document>::~DocPtr at DocPtr.h:32
#5	0x036c65fe in WebCore::Document::removedLastRef at Document.cpp:410
#6	0x0347247f in WebCore::TreeShared<WebCore::Node>::deref at TreeShared.h:69
#7	0x03c7ff3e in WTF::RefPtr<WebCore::Node>::~RefPtr at RefPtr.h:50
#8	0x03971a92 in WebCore::JSNode::~JSNode at JSNode.cpp:256
#9	0x038ed205 in WebCore::JSDocument::~JSDocument at JSDocument.cpp:251
#10	0x03915c7a in WebCore::JSHTMLDocument::~JSHTMLDocument at JSHTMLDocument.h:30
#11	0x0065c964 in JSC::Heap::sweep<(JSC::HeapType)0> at Collector.cpp:917
#12	0x005cbd76 in JSC::Heap::collect at Collector.cpp:996
#13	0x0378e1fb in WebCore::GCController::gcTimerFired at GCController.cpp:74
#14	0x0378e3ef in WebCore::Timer<WebCore::GCController>::fired at Timer.h:93
#15	0x03cf40f1 in WebCore::ThreadTimers::fireTimers at ThreadTimers.cpp:111
#16	0x03cf4305 in WebCore::ThreadTimers::sharedTimerFiredInternal at ThreadTimers.cpp:141
#17	0x03cf4351 in WebCore::ThreadTimers::sharedTimerFired at ThreadTimers.cpp:122
#18	0x03cbe98a in WebCore::timerFired at SharedTimerMac.mm:86

It seems like someone forgot about this poor little Document, and the only thing left holding onto it is JavaScript.  When JavaScript lets go of it, the DocLoader is destroyed, which happens before the load actually finishes... How could the rest of WebCore have forgotten about this Document w/o having canceled the loads?

One way to fix this would be to cancel all loads from within ~DocLoader(), but maybe that's just masking other bugs?

[Eric Seidel (no email)]

Added the above mentioned ASSERT:
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/loader/DocLoader.cpp
Committed r41435
http://trac.webkit.org/changeset/41435

Hopefully that will make it easier to find out what's actually going on here.

[Eric Seidel (no email)]

So I think that detached documents (those not in frames) can have pending loads associated with them.  I'm not exactly sure how I would create one of those, but it seems possible.  If that's the case then the FrameLoader code which is supposed to stop the DocLoader's loads, would not be run for these detached documents (since they don't have a Frame).

I'm not sure yet what the right fix is, but if I can figure out how to get a detached document to make a load (or to detach a document from a frame while it still has pending loads) then we can make a test case for this. ;)

[Eric Seidel (no email)]

Maybe it's possible to create an XHR in a subframe, and then grab it from the parent frame, destroy the subframe and the start a request from that XHR?

I'm not even sure that this is related to XHR loads (as those are different from normal loads).  It's possible that the right fix is to just always cancel all loads from within ~DocLoader() and not worry so much about if someone has always canceled them before the document might get destroyed?

[Alexey Proskuryakov]

This newly added assertion worked well, see bug 24449.

[Cameron Zwarich (cpst)]

*** Bug 24449 has been marked as a duplicate of this bug. ***

[Cameron Zwarich (cpst)]

I posted a reduction and a fix in bug 24449. I'll turn the reduction into an HTTP layout test and post it for review, hopefully some time later this evening.

[Cameron Zwarich (cpst)]

Created attachment 28431 [details]
Proposed patch

[Cameron Zwarich (cpst)]

Created attachment 28432 [details]
Proposed patch

The last patch had a typo in the test text.

[Geoffrey Garen]

Comment on attachment 28432 [details]
Proposed patch

r=me

This is a slightly better way to do GC in a layout test, since it works outside DRT too:

function gc()
{
    if (window.GCController)
        GCController.collect();
    else
        for (var i = 0; i < 10000; ++i) // Allocate a sufficient number of objects to force a GC.
            new Object;
}

[Cameron Zwarich (cpst)]

Committed as r41563 with Geoff's suggestion.