Bug 151398 – Web Inspector: Client Blocked Resource Requests causes Crash under InspectorPageAgent::cachedResource

• Component: Web Inspector
• Status: RESOLVED FIXED

[João Oliveira]

Created attachment 265761 [details]
crash log

test to reproduce: https://gist.github.com/jxs/59ec6d623ad7d912fcd4

[Radar WebKit Bug Importer]

<rdar://problem/23597038>

[Joseph Pecoraro]

Seems we are calling WebCore::CachedResourceLoader::cachedResource with a null String and that results in issues:

(lldb) f
frame #9: 0x0000000112003e40 WebCore`WebCore::InspectorPageAgent::cachedResource(frame=0x0000000119aea000, url=0x00007fff5af4e400) + 48 at InspectorPageAgent.cpp:261
   258 	
   259 	CachedResource* InspectorPageAgent::cachedResource(Frame* frame, const URL& url)
   260 	{
-> 261 	    CachedResource* cachedResource = frame->document()->cachedResourceLoader().cachedResource(url);
   262 	    if (!cachedResource) {
   263 	        ResourceRequest request(url);
   264 	#if ENABLE(CACHE_PARTITIONING)

(lldb) p url
(const WebCore::URL) $5 = {
  m_string = { length = 0, contents = '' } {
    m_impl = {
      m_ptr = 0x0000000000000000
    }
  }

(lldb) bt
* thread #1: tid = 0xd59a5c, 0x00000001111763fc WebCore`WTF::StringImpl::rawHash(this=0x0000000000000000) const + 12 at StringImpl.h:544, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
    frame #0: 0x00000001111763fc WebCore`WTF::StringImpl::rawHash(this=0x0000000000000000) const + 12 at StringImpl.h:544
    frame #1: 0x0000000111176375 WebCore`WTF::StringImpl::hasHash(this=0x0000000000000000) const + 21 at StringImpl.h:550
    frame #2: 0x0000000111176329 WebCore`WTF::StringImpl::hash(this=0x0000000000000000) const + 25 at StringImpl.h:561
    frame #3: 0x00000001111762fd WebCore`WTF::StringHash::hash(key=0x00007fff5af4db38) + 29 at StringHash.h:65
    frame #4: 0x0000000111176275 WebCore`unsigned int WTF::IdentityHashTranslator<WTF::StringHash>::hash<WTF::String>(key=0x00007fff5af4db38) + 21 at HashTable.h:283
    frame #5: 0x00000001113443a0 WebCore`WTF::KeyValuePair<WTF::String, WebCore::CachedResourceHandle<WebCore::CachedResource> >* WTF::HashTable<WTF::String, WTF::KeyValuePair<WTF::String, WebCore::CachedResourceHandle<WebCore::CachedResource> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::String, WebCore::CachedResourceHandle<WebCore::CachedResource> > >, WTF::StringHash, WTF::HashMap<WTF::String, WebCore::CachedResourceHandle<WebCore::CachedResource>, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WebCore::CachedResourceHandle<WebCore::CachedResource> > >::KeyValuePairTraits, WTF::HashTraits<WTF::String> >::lookup<WTF::IdentityHashTranslator<WTF::StringHash>, WTF::String>(this=0x0000000119ae5ed0, key=0x00007fff5af4db38) + 80 at HashTable.h:602
    frame #6: 0x00000001113442dd WebCore`WTF::HashTable<WTF::String, WTF::KeyValuePair<WTF::String, WebCore::CachedResourceHandle<WebCore::CachedResource> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WTF::String, WebCore::CachedResourceHandle<WebCore::CachedResource> > >, WTF::StringHash, WTF::HashMap<WTF::String, WebCore::CachedResourceHandle<WebCore::CachedResource>, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WebCore::CachedResourceHandle<WebCore::CachedResource> > >::KeyValuePairTraits, WTF::HashTraits<WTF::String> >::lookup(this=0x0000000119ae5ed0, key=0x00007fff5af4db38) + 29 at HashTable.h:412
    frame #7: 0x000000011133f1ab WebCore`WTF::HashMap<WTF::String, WebCore::CachedResourceHandle<WebCore::CachedResource>, WTF::StringHash, WTF::HashTraits<WTF::String>, WTF::HashTraits<WebCore::CachedResourceHandle<WebCore::CachedResource> > >::get(this=0x0000000119ae5ed0, key=0x00007fff5af4db38) const + 43 at HashMap.h:351
    frame #8: 0x000000011133a65d WebCore`WebCore::CachedResourceLoader::cachedResource(this=0x0000000119ae5ea0, resourceURL=0x00007fff5af4e400) const + 93 at CachedResourceLoader.cpp:159
  * frame #9: 0x0000000112003e40 WebCore`WebCore::InspectorPageAgent::cachedResource(frame=0x0000000119aea000, url=0x00007fff5af4e400) + 48 at InspectorPageAgent.cpp:261
    frame #10: 0x0000000111fe074b WebCore`WebCore::InspectorNetworkAgent::willSendRequest(this=0x0000000119bc0000, identifier=3, loader=0x000000011983a000, request=0x00007fff5af4e400, redirectResponse=0x00007fff5af4e528) + 315 at InspectorNetworkAgent.cpp:282
    ...

[Joseph Pecoraro]

Higher up in the stack, it looks like the inspector is told about a resource request which is basically empty:

(lldb) f
frame #13: 0x0000000112e57812 WebCore`WebCore::ResourceLoadNotifier::dispatchWillSendRequest(this=0x0000000119aea0c0, loader=0x000000011983a000, identifier=3, request=0x00007fff5af4e400, redirectResponse=0x00007fff5af4e528) + 338 at ResourceLoadNotifier.cpp:139
   136 	    if (!request.isNull() && oldRequestURL != request.url().string())
   137 	        m_frame.loader().documentLoader()->didTellClientAboutLoad(request.url());
   138 	
-> 139 	    InspectorInstrumentation::willSendRequest(&m_frame, identifier, loader, request, redirectResponse);
   140 	
   141 	    // Report WebTiming for all frames.
   142 	    if (loader && !request.isNull() && request.url() == loader->url())

(lldb) p request.isNull()
(bool) $12 = true

So perhaps we shouldn't be telling the inspector about null requests.

Why would we even get here... I'll ping some loader folks.

[Joseph Pecoraro]

This is happening because the FrameLoaderClient clears the request:

    void ResourceLoadNotifier::dispatchWillSendRequest(DocumentLoader* loader, unsigned long identifier, ResourceRequest& request, const ResourceResponse& redirectResponse)
    {
        ...
        String oldRequestURL = request.url().string();
        m_frame.loader().documentLoader()->didTellClientAboutLoad(request.url());

        m_frame.loader().client().dispatchWillSendRequest(loader, identifier, request, redirectResponse);

        // If the URL changed, then we want to put that new URL in the "did tell client" set too.
        if (!request.isNull() && oldRequestURL != request.url().string())
            m_frame.loader().documentLoader()->didTellClientAboutLoad(request.url());

        InspectorInstrumentation::willSendRequest(&m_frame, identifier, loader, request, redirectResponse);
        ...
    }

In this case, the inspector code and below does not gracefully handle a null string.

[Joseph Pecoraro]

Created attachment 265780 [details]
[PATCH] Proposed Fix

[Blaze Burg]

Comment on attachment 265780 [details]
[PATCH] Proposed Fix

r=me, but please wait until debug EWS in case that ASSERT is too strong for some obscure reason.

[WebKit Commit Bot]

Comment on attachment 265780 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 265780

Committed r192592: <http://trac.webkit.org/changeset/192592>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.