Bug 116995 – Crash in HTMLScriptRunner going back (much more reproducible on slow networks)

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Brady Eidson]

Crash in HTMLScriptRunner going back from slow page loads

Steps to repro in Safari/WebKit nightly: 
1 - Go to any web page (In my case, I always used pages that are eligible for the page cache)
2 - Enable "Network Link Conditioner" and emulate an average Edge network (mac-only, makes it highly reproducible)
3 - Go to a long running page load.  I used engadget.com and nytimes.com with luck.
4 - Wait for the progress bar to suggest that the response was received and the load was committed
5 - "Cmd-[" to go back.

>  1 com.apple.WebCore              0x1120dff20 WTF::OwnPtr<WebCore::DocumentStyleSheetCollection>::operator->() const + 0x10 (OwnPtr.h:72)
   2 com.apple.WebCore              0x1120cf905 WebCore::Document::haveStylesheetsLoaded() const + 0x25 (Document.cpp:5997)
   3 com.apple.WebCore              0x112549f50 WebCore::HTMLScriptRunner::isPendingScriptReady(WebCore::PendingScript const&) + 0x20 (HTMLScriptRunner.cpp:104)
   4 com.apple.WebCore              0x11254a885 WebCore::HTMLScriptRunner::executeParsingBlockingScripts() + 0x45 (HTMLScriptRunner.cpp:198)
   5 com.apple.WebCore              0x11254a9c5 WebCore::HTMLScriptRunner::executeScriptsWaitingForLoad(WebCore::CachedResource*) + 0x115 (HTMLScriptRunner.cpp:209)
   6 com.apple.WebCore              0x1124b5294 WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*) + 0xf4 (HTMLDocumentParser.cpp:935)
   7 com.apple.WebCore              0x1124b530f non-virtual thunk to WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*) + 0x2f
   8 com.apple.WebCore              0x111e2296d WebCore::CachedResource::checkNotify() + 0x6d (CachedResource.cpp:362)
   9 com.apple.WebCore              0x111e23402 WebCore::CachedResource::stopLoading() + 0xc2 (CachedResource.cpp:463)
  10 com.apple.WebCore              0x1133ebff1 WebCore::SubresourceLoader::releaseResources() + 0xd1 (SubresourceLoader.cpp:330)
  11 com.apple.WebCore              0x1131e776e WebCore::ResourceLoader::cancel(WebCore::ResourceError const&) + 0x23e (ResourceLoader.cpp:439)
  12 com.apple.WebCore              0x1131e6403 WebCore::ResourceLoader::cancel() + 0x43 (ResourceLoader.cpp:387)
  13 com.apple.WebCore              0x112112b15 WebCore::cancelAll(WTF::HashSet<WTF::RefPtr<WebCore::ResourceLoader>, WTF::PtrHash<WTF::RefPtr<WebCore::ResourceLoader> >, WTF::HashTraits<WTF::RefPtr<WebCore::ResourceLoader> > > const&) + 0x65 (DocumentLoader.cpp:84)
  14 com.apple.WebCore              0x112112cfc WebCore::DocumentLoader::stopLoadingSubresources() + 0x1c (DocumentLoader.cpp:1272)
  15 com.apple.WebCore              0x112112a56 WebCore::DocumentLoader::stopLoading() + 0x286 (DocumentLoader.cpp:310)
  16 com.apple.WebCore              0x112353913 WebCore::FrameLoader::stopAllLoaders(WebCore::ClearProvisionalItemPolicy) + 0x1f3 (FrameLoader.cpp:1599)
  17 com.apple.WebCore              0x112e61478 WebCore::Page::goToItem(WebCore::HistoryItem*, WebCore::FrameLoadType) + 0x98 (Page.cpp:427)

The crash is because HTMLScriptRunner has a null m_document.

Reproducing the crash is highly timing dependent, even with the Network Link Conditioner, so an automated test is probably out of reach.

In radar as <rdar://problem/13996730>

[Brady Eidson]

Created attachment 203274 [details]
Patch v1 

While something deeper might be going on here and warrants further exploration, this null check certainly fixes the crash.  Other sites in HTMLScriptRunner null check or ASSERT m_document so it's clearly expected in some situations.

[Anders Carlsson]

Comment on attachment 203274 [details]
Patch v1 

View in context: https://bugs.webkit.org/attachment.cgi?id=203274&action=review

> Source/WebCore/html/parser/HTMLScriptRunner.cpp:105
> +    if (!m_document)
> +        return false;

Why is m_document null here? I'd like a comment explaining this (as well as the ChangeLog explaining it).

[Brady Eidson]

(In reply to comment #2)
> (From update of attachment 203274 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203274&action=review
> 
> > Source/WebCore/html/parser/HTMLScriptRunner.cpp:105
> > +    if (!m_document)
> > +        return false;
> 
> Why is m_document null here? I'd like a comment explaining this (as well as the ChangeLog explaining it).

It's null because it was cleared out in implicitClose before stopallloaders was called.  This appears to be unique to page cache loading.

I'll add comments.

[luowenping]

<script>
 document.write('');
</script>

will lead to HTMLDocumentParser detach, but HTMLDocumentParser maybe a client to CachedScript at this time.Detach operation won't cancel any  load request ,so Cachedscript will notify HTMLDocumentParser when is finishLoading.

and more, we did'nt cancel any request when we detached a document, is right? we should cancel MainResourceLoad at least.

[Brady Eidson]

After http://trac.webkit.org/changeset/151088 this doesn't reproduce.