Summary: | Crashes loading pages when cancelling subresource loads through WebKit | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Weinstein <bweinstein> | ||||||
Component: | Page Loading | Assignee: | Brian Weinstein <bweinstein> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, ap, beidson, eric, koivisto, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Brian Weinstein
2011-01-25 13:44:24 PST
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++ Created attachment 80118 [details]
[PATCH] Fix
Created attachment 80143 [details]
[PATCH] Take 2
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. (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! 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 |