Web Inspector: Network.loadResource XHR crash if page reloaded while request is ongoing

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

<rdar://problem/16281293>

* STEPS TO REPRODUCe
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);
        --m_pendingActivityCount;
        thisObject->deref();
    }

So this leads to the CRASH in:

    void ScriptExecutionContext::willDestroyActiveDOMObject(ActiveDOMObject* object)
    {
        ASSERT(object);
        if (m_iteratingActiveDOMObjects)
            CRASH();
        m_activeDOMObjects.remove(object);
    }

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.

[Joseph Pecoraro]

Turns out, a better approach may be what Blink / Chromium do:
<http://src.chromium.org/viewvc/blink?view=revision&revision=152712>

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.

[Joseph Pecoraro]

Created attachment 226625 [details]
[PATCH] Proposed Fix

[Timothy Hatcher]

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;

Yay!

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/165582>