Bug 130211

Summary: Web Inspector: Network.loadResource XHR crash if page reloaded while request is ongoing
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Severity: Normal CC: graouts, joepeck, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
[PATCH] Proposed Fix timothy: review+

Description Joseph Pecoraro 2014-03-13 14:53:42 PDT

1. Go to a page with a SourceMap that takes a while to load (e.g. a php page that sleeps for 10 seconds before serving the source map)
2. Open Inspector
3. Reload the page while the inspector is requesting the source map file
  => CRASH, mutating data structure while iterating

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   WTFCrash + 62
1   WebCore::ScriptExecutionContext::willDestroyActiveDOMObject
2   WebCore::ActiveDOMObject::~ActiveDOMObject
3   WebCore::XMLHttpRequest::~XMLHttpRequest
4   WebCore::XMLHttpRequest::~XMLHttpRequest
5   WebCore::XMLHttpRequest::dropProtection
6   WebCore::ScriptExecutionContext::stopActiveDOMObjects
7   WebCore::Document::prepareForDestruction
8   WebCore::Frame::setView
9   WebCore::Frame::createView
10  WebKit::WebFrameLoaderClient::transitionToCommittedForNewPage
11  WebCore::FrameLoader::transitionToCommitted
12  WebCore::FrameLoader::commitProvisionalLoad
13  WebCore::DocumentLoader::commitLoad

The XMLHttpRequest::dropProtection triggers ActiveObject::unsetPendingActivity which may cause its destruction:

    template<class T> void unsetPendingActivity(T* thisObject)
        ASSERT(m_pendingActivityCount > 0);

So this leads to the CRASH in:

    void ScriptExecutionContext::willDestroyActiveDOMObject(ActiveDOMObject* object)
        if (m_iteratingActiveDOMObjects)

My guess is that we will need to hold a reference to the XHR's we create in InspectorResourceAgent::loadResource to prevent the object from being deallocated inside stopActiveDOMObjects, and instead be aborted / deallocated in some other way.
Comment 1 Joseph Pecoraro 2014-03-13 14:56:01 PDT
Turns out, a better approach may be what Blink / Chromium do:

They moved from XMLHttpRequests to a pure DocumentThreadableLoader. This has some advantages:

  - Now that the load is backed by a non-ActiveDOM-object it can load even if the page is paused
  - We don't need to worry about this crash.

So I followed that approach with my patches.
Comment 2 Joseph Pecoraro 2014-03-13 15:10:59 PDT
Created attachment 226625 [details]
[PATCH] Proposed Fix
Comment 3 Timothy Hatcher 2014-03-13 16:02:11 PDT
Comment on attachment 226625 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/inspector/InspectorResourceAgent.cpp:102
> +        }
> +        m_decoder = TextResourceDecoder::create(ASCIILiteral("text/plain"), textEncoding, useDetector);

Nit: I like newlines between things like this.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:783
> +    }
> +    loader->setDefersLoading(false);

Nit: Newline.

> Source/WebCore/xml/XMLHttpRequest.cpp:794
> -    options.crossOriginRequestPolicy = m_sendingForInspector ? AllowCrossOriginRequests : UseAccessControl;
> +    options.crossOriginRequestPolicy = UseAccessControl;

Comment 4 Joseph Pecoraro 2014-03-13 18:12:18 PDT