Bug 49693 – REGRESSION(71884): Crash when installing extension that references safari-extension URL

• Component: Page Loading
• Status: RESOLVED FIXED

[Brian Weinstein]

STEPS TO REPRODUCE:
1) Build a revision of WebKit after 71884.
2) Run it with Safari
3) Install the attached extension

EXPECTED RESULTS:
The browser shouldn't crash

ACTUAL RESULTS:
It does - it hits an assert, then crashes:

>	WebKit.dll!WTF::Deque<WTF::RefPtr<WebCore::ResourceLoader> >::removeFirst()  Line 470 + 0x2b bytes	C++
 	WebKit.dll!WebCore::ResourceLoadScheduler::servePendingRequests(WebCore::ResourceLoadScheduler::HostInformation * host=0x1580af70, WebCore::ResourceLoadScheduler::Priority minimumPriority=Medium)  Line 198	C++
 	WebKit.dll!WebCore::ResourceLoadScheduler::scheduleLoad(WebCore::ResourceLoader * resourceLoader=0x21714d58, WebCore::ResourceLoadScheduler::Priority priority=Medium)  Line 120	C++
 	WebKit.dll!WebCore::ResourceLoadScheduler::scheduleSubresourceLoad(WebCore::Frame * frame=0x1fc118d0, WebCore::SubresourceLoaderClient * client=0x15800e5c, const WebCore::ResourceRequest & request={...}, WebCore::ResourceLoadScheduler::Priority priority=Medium, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true, bool shouldContentSniff=true)  Line 89	C++
 	WebKit.dll!WebCore::Loader::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x1edcef78, WebCore::CachedResource * resource=0x22888e08, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 132 + 0x4f bytes	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x1edcef78, bool incremental=false, WebCore::SecurityCheckPolicy securityCheck=DoSecurityCheck, bool sendResourceLoadCallbacks=true)  Line 111	C++
 	WebKit.dll!WebCore::CachedResource::load(WebCore::CachedResourceLoader * cachedResourceLoader=0x1edcef78)  Line 79 + 0x20 bytes	C++
 	WebKit.dll!WebCore::MemoryCache::requestResource(WebCore::CachedResourceLoader * cachedResourceLoader=0x1edcef78, WebCore::CachedResource::Type type=Script, const WebCore::KURL & url={???}, const WTF::String & charset={???}, bool requestIsPreload=false)  Line 131 + 0x13 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestResource(WebCore::CachedResource::Type type=Script, const WTF::String & url={???}, const WTF::String & charset={???}, bool isPreload=false)  Line 266 + 0x20 bytes	C++
 	WebKit.dll!WebCore::CachedResourceLoader::requestScript(const WTF::String & url={???}, const WTF::String & charset={???})  Line 163	C++
 	WebKit.dll!WebCore::HTMLScriptRunner::requestPendingScript(WebCore::PendingScript & pendingScript={...}, WebCore::Element * script=0x1eb5cfa0)  Line 284 + 0x44 bytes	C++
 	WebKit.dll!WebCore::HTMLScriptRunner::requestParsingBlockingScript(WebCore::Element * element=0x1eb5cfa0)  Line 250 + 0x13 bytes	C++
 	WebKit.dll!WebCore::HTMLScriptRunner::runScript(WebCore::Element * script=0x1eb5cfa0, const WTF::TextPosition<WTF::OneBasedNumber> & scriptStartPosition={...})  Line 316	C++
 	WebKit.dll!WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element> scriptElement={...}, const WTF::TextPosition<WTF::OneBasedNumber> & scriptStartPosition={...})  Line 185	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder()  Line 199 + 0x23 bytes	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode mode=AllowYield)  Line 235 + 0x8 bytes	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode mode=AllowYield)  Line 170	C++
 	WebKit.dll!WebCore::HTMLDocumentParser::append(const WebCore::SegmentedString & source={...})  Line 312	C++
 	WebKit.dll!WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter * writer=0x1fc11a54, const char * data=0x00000000, int length=0, bool shouldFlush=true)  Line 54 + 0x1f bytes	C++
 	WebKit.dll!WebCore::DocumentWriter::addData(const char * str=0x00000000, int len=0, bool flush=true)  Line 200 + 0x1f bytes	C++
 	WebKit.dll!WebCore::DocumentWriter::endIfNotLoadingMainResource()  Line 221	C++
 	WebKit.dll!WebCore::DocumentWriter::end()  Line 207	C++
 	WebKit.dll!WebCore::DocumentLoader::finishedLoading()  Line 278	C++
 	WebKit.dll!WebCore::FrameLoader::finishedLoading()  Line 2174	C++
 	WebKit.dll!WebCore::MainResourceLoader::didFinishLoading(double finishTime=0.00000000000000000)  Line 458	C++
 	WebKit.dll!WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle * __formal=0x0d0aaff0, double finishTime=0.00000000000000000)  Line 435 + 0x18 bytes	C++
 	WebKit.dll!WebCore::didFinishLoading(_CFURLConnection * conn=0x1f075fe0, const void * clientInfo=0x0d0aaff0)  Line 244 + 0x26 bytes	C++

[Brian Weinstein]

Created attachment 74166 [details]
Extension that crashes

[Brian Weinstein]

<rdar://problem/8680809>

[Nate Chapin]

I'll take a look at this first thing tomorrow.

[Brian Weinstein]

(In reply to comment #3)
> I'll take a look at this first thing tomorrow.

Thanks, let me know if you have any questions.

[Nate Chapin]

Created attachment 74280 [details]
patch + test

It's a pretty simple fix.  If we start the load, then remove the ResourceLoader from the queue after, it might have been cancelled and already removed from the queue (causing a crash if the queue is now empty).  Instead, we should remove the ResourceLoader from the queue before starting it.

[Nate Chapin]

Created attachment 74281 [details]
new patch + test

This version moves both removing from the queue and adding to the set of loads in progress.  By doing them both before, we can trust ResourceLoadScheduler::remove() to do the right thing without any special logic in servePendingRequests(), even when called syncrhonously.

[Antti Koivisto]

looks good to me too

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74281 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html
compositing/iframes/overlapped-nested-iframes.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 74281 [details]:

inspector/timeline-parse-html.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 74281 [details]
new patch + test

Clearing flags on attachment: 74281

Committed r72435: <http://trac.webkit.org/changeset/72435>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.