Bug 130211 - Web Inspector: Network.loadResource XHR crash if page reloaded while request is ongoing
Summary: Web Inspector: Network.loadResource XHR crash if page reloaded while request ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-03-13 14:53 PDT by Joseph Pecoraro
Modified: 2014-03-13 18:12 PDT (History)
4 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (16.02 KB, patch)
2014-03-13 15:10 PDT, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-03-13 14:53:42 PDT
<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.
Comment 1 Joseph Pecoraro 2014-03-13 14:56:01 PDT
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.
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;

Yay!
Comment 4 Joseph Pecoraro 2014-03-13 18:12:18 PDT
<http://trac.webkit.org/changeset/165582>