Bug 53123 – Crashes loading pages when cancelling subresource loads through WebKit

• Component: Page Loading
• Status: RESOLVED FIXED

[Brian Weinstein]

r75174 added a way to cancel resource loads for WebKit2 at the WebKit layer. However, using it to cancel resource loads showed some crashes in both WebKit1 and WebKit2 modes that are exposed when willSendRequest clears our the ResourceRequest.

One of the first crashes was infinite recursion under CachedResourceLoader::checkForPendingPreloads.

 >	WebKit.dll!_chkstk()  Line 99	Asm
 	WebKit.dll!WebCore::KURL::init(const WebCore::KURL & base={file:///C:/Users/bweinstein/Desktop/cnn_main.html}, const WTF::String & relative={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WebCore::TextEncoding & encoding={...})  Line 412	C++
 	WebKit.dll!WebCore::KURL::KURL(const WebCore::KURL & base={file:///C:/Users/bweinstein/Desktop/cnn_main.html}, const WTF::String & relative={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WebCore::TextEncoding & encoding={...})  Line 337	C++
 	WebKit.dll!WebCore::Document::completeURL(const WTF::String & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js})  Line 3934 + 0x26 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::checkForPendingPreloads()  Line 592 + 0x16 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::loadDone(WebCore::CachedResourceRequest * request=0x00000000)  Line 523	C++
 	WebKit.dll!WebCore::CachedResourceRequest::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48, WebCore::CachedResource * resource=0x17170e00, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 128	C++
 	WebKit.dll!WebCore::CachedResourceLoader::load(WebCore::CachedResource * resource=0x17170e00, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 509 + 0x1d bytes	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 133	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48)  Line 83 + 0x20 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::loadResource(WebCore::CachedResource::Type type=Script, const WebCore::KURL & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1}, WebCore::ResourceLoadPriority priority=ResourceLoadPriorityUnresolved)  Line 361 + 0x13 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type type=Script, const WTF::String & resourceURL={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1}, WebCore::ResourceLoadPriority priority=ResourceLoadPriorityUnresolved, bool forPreload=true)  Line 301 + 0x18 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestPreload(WebCore::CachedResource::Type type=Script, const WTF::String & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={})  Line 604 + 0x18 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::checkForPendingPreloads()  Line 594	C++
 	WebKit.dll!WebCore::CachedResourceLoader::loadDone(WebCore::CachedResourceRequest * request=0x00000000)  Line 523	C++
 	WebKit.dll!WebCore::CachedResourceRequest::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48, WebCore::CachedResource * resource=0x172b4e00, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 128	C++
 	WebKit.dll!WebCore::CachedResourceLoader::load(WebCore::CachedResource * resource=0x172b4e00, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 509 + 0x1d bytes	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 133	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48)  Line 83 + 0x20 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::loadResource(WebCore::CachedResource::Type type=Script, const WebCore::KURL & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1}, WebCore::ResourceLoadPriority priority=ResourceLoadPriorityUnresolved)  Line 361 + 0x13 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type type=Script, const WTF::String & resourceURL={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1}, WebCore::ResourceLoadPriority priority=ResourceLoadPriorityUnresolved, bool forPreload=true)  Line 301 + 0x18 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestPreload(WebCore::CachedResource::Type type=Script, const WTF::String & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={})  Line 604 + 0x18 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::checkForPendingPreloads()  Line 594	C++
 	WebKit.dll!WebCore::CachedResourceLoader::loadDone(WebCore::CachedResourceRequest * request=0x00000000)  Line 523	C++
 	WebKit.dll!WebCore::CachedResourceRequest::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48, WebCore::CachedResource * resource=0x172a4e00, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 128	C++
 	WebKit.dll!WebCore::CachedResourceLoader::load(WebCore::CachedResource * resource=0x172a4e00, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 509 + 0x1d bytes	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 133	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48)  Line 83 + 0x20 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::loadResource(WebCore::CachedResource::Type type=Script, const WebCore::KURL & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1}, WebCore::ResourceLoadPriority priority=ResourceLoadPriorityUnresolved)  Line 361 + 0x13 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type type=Script, const WTF::String & resourceURL={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1}, WebCore::ResourceLoadPriority priority=ResourceLoadPriorityUnresolved, bool forPreload=true)  Line 301 + 0x18 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestPreload(WebCore::CachedResource::Type type=Script, const WTF::String & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={})  Line 604 + 0x18 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::checkForPendingPreloads()  Line 594	C++
 	WebKit.dll!WebCore::CachedResourceLoader::loadDone(WebCore::CachedResourceRequest * request=0x00000000)  Line 523	C++
 	WebKit.dll!WebCore::CachedResourceRequest::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48, WebCore::CachedResource * resource=0x16d6ce00, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 128	C++
 	WebKit.dll!WebCore::CachedResourceLoader::load(WebCore::CachedResource * resource=0x16d6ce00, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 509 + 0x1d bytes	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 133	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48)  Line 83 + 0x20 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::loadResource(WebCore::CachedResource::Type type=Script, const WebCore::KURL & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1}, WebCore::ResourceLoadPriority priority=ResourceLoadPriorityUnresolved)  Line 361 + 0x13 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type type=Script, const WTF::String & resourceURL={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1}, WebCore::ResourceLoadPriority priority=ResourceLoadPriorityUnresolved, bool forPreload=true)  Line 301 + 0x18 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestPreload(WebCore::CachedResource::Type type=Script, const WTF::String & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={})  Line 604 + 0x18 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::checkForPendingPreloads()  Line 594	C++
 	WebKit.dll!WebCore::CachedResourceLoader::loadDone(WebCore::CachedResourceRequest * request=0x00000000)  Line 523	C++
 	WebKit.dll!WebCore::CachedResourceRequest::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48, WebCore::CachedResource * resource=0x17184e00, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 128	C++
 	WebKit.dll!WebCore::CachedResourceLoader::load(WebCore::CachedResource * resource=0x17184e00, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 509 + 0x1d bytes	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 133	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0c4f8f48)  Line 83 + 0x20 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::loadResource(WebCore::CachedResource::Type type=Script, const WebCore::KURL & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1}, WebCore::ResourceLoadPriority priority=ResourceLoadPriorityUnresolved)  Line 361 + 0x13 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type type=Script, const WTF::String & resourceURL={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1}, WebCore::ResourceLoadPriority priority=ResourceLoadPriorityUnresolved, bool forPreload=true)  Line 301 + 0x18 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestPreload(WebCore::CachedResource::Type type=Script, const WTF::String & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={})  Line 604 + 0x18 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::checkForPendingPreloads()  Line 594	C++

(this is a very small part of the stack).

[Brian Weinstein]

To fix this, I switched CachedResourceLoader::m_pendingPreloads to be a Deque instead of a Vector, so we wouldn't infinitely recurse trying to preload the same subresource - however that gave us a crash under CachedResource::load because CachedResource::setRequest was being called, which deleted the CachedResource before we were done with it.

This object was deleted soon after - and this code had been there since the beginning of time, and was never called in normal browsing except in the new code path that can cancel resource loads.

Backtrace:

>	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0ee57f40, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 133 + 0x3 bytes	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x0ee57f40)  Line 83 + 0x20 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::loadResource(WebCore::CachedResource::Type type=Script, const WebCore::KURL & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1}, WebCore::ResourceLoadPriority priority=ResourceLoadPriorityUnresolved)  Line 361 + 0x13 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type type=Script, const WTF::String & resourceURL={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1}, WebCore::ResourceLoadPriority priority=ResourceLoadPriorityUnresolved, bool forPreload=false)  Line 297 + 0x18 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestScript(const WTF::String & url={http://i.cdn.turner.com/cnn/.element/js/3.0/s_code.js}, const WTF::String & charset={ISO-8859-1})  Line 176	C++
 	WebKit.dll!WebCore::HTMLScriptRunner::requestPendingScript(WebCore::PendingScript & pendingScript={...}, WebCore::Element * script=0x10c2cfa8)  Line 274 + 0x32 bytes	C++
 	WebKit.dll!WebCore::HTMLScriptRunner::requestParsingBlockingScript(WebCore::Element * element=0x10c2cfa8)  Line 240 + 0x13 bytes	C++
 	WebKit.dll!WebCore::HTMLScriptRunner::runScript(WebCore::Element * script=0x10c2cfa8, const WTF::TextPosition<WTF::OneBasedNumber> & scriptStartPosition={...})  Line 305 + 0xc bytes	C++
 	WebKit.dll!WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element> scriptElement={...}, const WTF::TextPosition<WTF::OneBasedNumber> & scriptStartPosition={...})  Line 175	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder()  Line 199 + 0x23 bytes	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode mode=AllowYield)  Line 244 + 0x8 bytes	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode mode=AllowYield)  Line 170	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution()  Line 430	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource * cachedResource=0x0efe0e00)  Line 475	C++
 	WebKit.dll!WebCore::CachedScript::checkNotify()  Line 104 + 0x13 bytes	C++
 	WebKit.dll!WebCore::CachedScript::data(WTF::PassRefPtr<WebCore::SharedBuffer> data={...}, bool allDataReceived=true)  Line 95	C++
 	WebKit.dll!WebCore::CachedResourceRequest::didFinishLoading(WebCore::SubresourceLoader * loader=0x0f118d58)  Line 160	C++
 	WebKit.dll!WebCore::SubresourceLoader::didFinishLoading(double finishTime=0.00000000000000000)  Line 181 + 0x1f bytes	C++
 	WebKit.dll!WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle * __formal=0x0d342ff0, double finishTime=0.00000000000000000)  Line 434 + 0x18 bytes	C++
 	WebKit.dll!WebCore::didFinishLoading(_CFURLConnection * conn=0x10c26fe0, const void * clientInfo=0x0d342ff0)  Line 241 + 0x26 bytes	C++

[Brian Weinstein]

Created attachment 80118 [details]
[PATCH] Fix

[Brian Weinstein]

Created attachment 80143 [details]
[PATCH] Take 2

[Antti Koivisto]

Comment on attachment 80143 [details]
[PATCH] Take 2

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:83
> +    , m_loadDonePendingActionTimer(this, &CachedResourceLoader::loadDonePendingActionTimerFired)

i would remove the word "Pending" from these names. It doesn't add much.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:532
> +
> +    if (request) {
> +        checkForPendingPreloads();
> +        resourceLoadScheduler()->servePendingRequests();
> +    } else {
> +        // If the request passed to this function is null, loadDone finished synchronously from when
> +        // the load was started, so we want to kick off our next set of loads (via checkForPendingPreloads
> +        // and servePendingRequests) asynchronously.
> +        m_loadDonePendingActionTimer.startOneShot(0);
> +    }

This would read better with early return and the usual case on the left:

if (!request) {
   ....
   m_loadDonePendingActionTimer.startOneShot(0)
   return;
}
...

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:540
> +void CachedResourceLoader::loadDonePendingActionTimerFired(Timer<CachedResourceLoader>*)
> +{
>      checkForPendingPreloads();
>      resourceLoadScheduler()->servePendingRequests();
>  }

You could put these calls to a function (performPostLoadActions() or something) and call that from both loadDone() and here.

[Brian Weinstein]

(In reply to comment #4)
> (From update of attachment 80143 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80143&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:83
> > +    , m_loadDonePendingActionTimer(this, &CachedResourceLoader::loadDonePendingActionTimerFired)
> 
> i would remove the word "Pending" from these names. It doesn't add much.

Fixed.

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:532
> > +
> > +    if (request) {
> > +        checkForPendingPreloads();
> > +        resourceLoadScheduler()->servePendingRequests();
> > +    } else {
> > +        // If the request passed to this function is null, loadDone finished synchronously from when
> > +        // the load was started, so we want to kick off our next set of loads (via checkForPendingPreloads
> > +        // and servePendingRequests) asynchronously.
> > +        m_loadDonePendingActionTimer.startOneShot(0);
> > +    }
> 
> This would read better with early return and the usual case on the left:
> 
> if (!request) {
>    ....
>    m_loadDonePendingActionTimer.startOneShot(0)
>    return;
> }
> ...

Fixed.

> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:540
> > +void CachedResourceLoader::loadDonePendingActionTimerFired(Timer<CachedResourceLoader>*)
> > +{
> >      checkForPendingPreloads();
> >      resourceLoadScheduler()->servePendingRequests();
> >  }
> 
> You could put these calls to a function (performPostLoadActions() or something) and call that from both loadDone() and here.

Fixed.

Thanks for the review!

[Brian Weinstein]

Landed in r76701.

[WebKit Review Bot]

http://trac.webkit.org/changeset/76701 might have broken Qt Linux Release
The following tests are not passing:
fast/loader/willSendRequest-null-for-preload.html