Bug 95187 – Web Inspector: Add ability to replay XHR in network panel.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Chadnov]

Ability to replay existing XHR.

[Pavel Chadnov]

Created attachment 160952 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 160952 [details]
Patch

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

Please add tests.

> Source/WebCore/inspector/Inspector.json:878
> +                "description": "Replays XHR.",

Please make this description more verbose. It should mention that XHR resent is identical to the original one and specify the concrete parameters that are identical in them.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:333
> +void InspectorInstrumentation::DidCreateXhrRequestImpl(InstrumentingAgents* instrumentingAgents, String method, bool async, PassRefPtr<FormData> formData, const HTTPHeaderMap& headers, bool includeCredentials, String login, String password)

didCreateXhrRequestImpl

> Source/WebCore/inspector/InspectorResourceAgent.h:71
> +struct XhrData;

XHRReplayData

> Source/WebCore/inspector/InspectorResourceAgent.h:-108
> -

Please revert unneeded changes.

> Source/WebCore/inspector/front-end/NetworkPanel.js:965
> +        if (request && request.type.name() === "xhr") {

request.type === WebInspector.resourceTypes.XHR

> Source/WebCore/inspector/front-end/NetworkRequest.js:801
> +        

Please revert

> Source/WebCore/loader/DocumentThreadableLoader.cpp:403
> +    if (m_preflightRequestIdentifier)

Let's make it look like 
if (m_actualRequest)
  ... 
else if (m_preflightRequestIdentifier)
  ... 
for consistency.

> Source/WebCore/xml/XMLHttpRequest.cpp:690
> +    createRequest(ec);

As discussed we should set Content-Type header here.

[Vsevolod Vlasov]

Comment on attachment 160952 [details]
Patch

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

Please add a screenshot

> Source/WebCore/ChangeLog:8
> +        Added XHR replay in network tab.

Please add a description of implementation details.

[Pavel Chadnov]

Created attachment 160961 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 160961 [details]
Patch

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

> Source/WebCore/inspector/InspectorInstrumentation.h:165
> +    static void didCreateXHRRequestImpl(InstrumentingAgents*, String, bool, PassRefPtr<FormData>, const HTTPHeaderMap&, bool, String, String);

...Impl methods should be below (together with the others).

> Source/WebCore/inspector/InspectorInstrumentation.h:167
> +    static void preflightRequestSuccededImpl(InstrumentingAgents*, unsigned long);

Ditto

> Source/WebCore/inspector/InspectorResourceAgent.cpp:226
> +        m_preflightIdToRequestId.add(requestId, IdentifiersFactory::requestId(m_preflightRequestId));

m_resourcesData->setXHRReplayData(requestId, m_resourcesData->XHRData(m_preflightRequestId));
This way you don't need this map at all.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:650
> +    , m_XHRData(0)

m_XHRReplayData

> Source/WebCore/inspector/InspectorResourceAgent.h:164
> +    unsigned long m_preflightRequestId;

I eould call it m_xhrReplayDataRequestId and reuse in replayXHR to avoid duplicating xhrReplayData for replayed xhrs.

> Source/WebCore/inspector/InspectorResourceAgent.h:165
> +    HashMap<String, String> m_preflightIdToRequestId;

This map is not really needed, see above.

[Vsevolod Vlasov]

Comment on attachment 160961 [details]
Patch

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

> Source/WebCore/inspector/InspectorResourceAgent.h:163
> +    XHRReplayData* m_XHRData;

RefPtr

> Source/WebCore/inspector/NetworkResourcesData.h:58
> +    XHRReplayData(String method, bool async, PassRefPtr<FormData> formData, const HTTPHeaderMap& headers, bool includeCredentials, String login, String password): m_method(method), m_async(async), m_formData(formData), m_headers(headers), m_includeCredentials(includeCredentials), m_login(login), m_password(password) { }

Please move implementation to .cpp

> Source/WebCore/inspector/NetworkResourcesData.h:118
> +        XHRReplayData* m_XHRData;

RefPtr

[Pavel Chadnov]

Created attachment 161220 [details]
Patch

[Build Bot]

Comment on attachment 161220 [details]
Patch

Attachment 161220 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13686209

[Build Bot]

Comment on attachment 161220 [details]
Patch

Attachment 161220 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13683283

[Vsevolod Vlasov]

Comment on attachment 161220 [details]
Patch

Clearing r? as some comments were not addressed yet.

[Pavel Chadnov]

Created attachment 161232 [details]
Patch

[Build Bot]

Comment on attachment 161232 [details]
Patch

Attachment 161232 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13685292

[Pavel Chadnov]

Created attachment 161256 [details]
Patch

[Build Bot]

Comment on attachment 161256 [details]
Patch

Attachment 161256 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13680301

[Build Bot]

Comment on attachment 161256 [details]
Patch

Attachment 161256 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13682283

[Vsevolod Vlasov]

Comment on attachment 161256 [details]
Patch

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

> Source/WebCore/inspector/InspectorResourceAgent.cpp:595
> +void InspectorResourceAgent::replayXHR(ErrorString* errorString, const String& requestId)

Add ASSERT_UNUSED(errorString); below to make mac bot happy.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:602
> +    RefPtr<XHRReplayData> XHRData = m_resourcesData->XHRData(actualRequestId);

xhrData

> Source/WebCore/inspector/InspectorResourceAgent.cpp:654
> +    , m_XHRData(0)

m_xhrData

> Source/WebCore/inspector/NetworkResourcesData.h:120
> +        RefPtr<XHRReplayData> m_XHRData;

ditto

> Source/WebCore/inspector/NetworkResourcesData.h:153
> +    PassRefPtr<XHRReplayData> XHRData(const String& requestId);

ditto

> Source/WebCore/loader/DocumentThreadableLoader.cpp:386
> +            InspectorInstrumentation::preflightRequestSucceded(m_document, m_preflightRequestIdentifier);

No brackets for one line conditionals.

[Vsevolod Vlasov]

Comment on attachment 161256 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:882
> +                "returns": [

Empty returns is redundant.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:333
> +void InspectorInstrumentation::didCreateXHRRequestImpl(InstrumentingAgents* instrumentingAgents, String method, bool async, PassRefPtr<FormData> formData, const HTTPHeaderMap& headers, bool includeCredentials, String login, String password)

Here and in other places: s/String/const String&/

> Source/WebCore/xml/XMLHttpRequest.cpp:689
> +    m_requestEntityBody = formData;

You are not really passing the ownership of formData here. You should pass it by pointer and deepCopy back here.

> Source/WebCore/xml/XMLHttpRequest.cpp:728
> +    InspectorInstrumentation::didCreateXHRRequest(scriptExecutionContext(), m_method, m_async, m_requestEntityBody ? m_requestEntityBody->deepCopy() : 0, m_requestHeaders, m_includeCredentials, m_url.user(), m_url.pass());

We'd better use m_requestHeaders.copyData() here instead of m_requestHeaders (and adopt() the result on the other side)

[Pavel Chadnov]

Created attachment 161470 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 161470 [details]
Patch

This patch looks identical to the previous one.

[Pavel Chadnov]

Created attachment 161475 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13694695

[Gyuyoung Kim]

Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13682716

[Build Bot]

Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13681720

[Early Warning System Bot]

Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13690727

[Early Warning System Bot]

Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13686692

[Peter Beverloo (cr-android ews)]

Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13695710

[Build Bot]

Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13686693

[kov's GTK+ EWS bot]

Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13690732

[Pavel Chadnov]

Created attachment 161487 [details]
Patch

[Build Bot]

Comment on attachment 161487 [details]
Patch

Attachment 161487 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13682752

[Vsevolod Vlasov]

Comment on attachment 161487 [details]
Patch

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

> Source/WebCore/inspector/InspectorResourceAgent.cpp:212
> +        OwnPtr<CrossThreadHTTPHeaderMapData> headerMapData = headers.copyData();

Please remove

> Source/WebCore/inspector/InspectorResourceAgent.cpp:614
> +        xhr->setRequestHeader(it->first, it->second, code);    xhr->sendFromInspector(xhrData->m_formData, code);

New line

> Source/WebCore/inspector/NetworkResourcesData.cpp:276
> +void NetworkResourcesData::setXHRReplayData(const String& requestId, PassRefPtr<XHRReplayData> XHRData)

zhrData

> Source/WebCore/inspector/NetworkResourcesData.h:51
> +    String m_method;

Please make fields private an add getters.

> Source/WebCore/inspector/NetworkResourcesData.h:108
> +        void setXHRReplayData(PassRefPtr<XHRReplayData> XHRData) { m_xhrReplayData = XHRData;};

space between ; and }

> Source/WebCore/loader/DocumentThreadableLoader.cpp:404
> +    if (m_actualRequest)

Please surround with #if ENABLE(INSPECTOR)

[Pavel Chadnov]

Created attachment 161641 [details]
Patch

[Andrey Adaikin]

Comment on attachment 161641 [details]
Patch

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

> Source/WebCore/inspector/Inspector.json:878
> +                "description": "Replays XHR. The new XHR is identical to original (parameters method, async, body, headers, withCredentials, login, password are copied)",

to original -> to the original one

> Source/WebCore/inspector/InspectorInstrumentation.h:1185
> +

extra line?

> Source/WebCore/inspector/InspectorResourceAgent.cpp:600
> +void InspectorResourceAgent::replayXHR(ErrorString* errorString, const String& requestId)

"ErrorString* errorString," -> "ErrorString*," and remove UNUSED_PARAM macro below

> Source/WebCore/inspector/InspectorResourceAgent.cpp:615
> +    m_originalRequestId = String();

I guess String should have a method like clear() or reset()

> Source/WebCore/inspector/InspectorResourceAgent.h:71
> +struct XHRReplayData;

struct -> class

> Source/WebCore/inspector/NetworkResourcesData.cpp:49
> +PassRefPtr<XHRReplayData> XHRReplayData::create(String method, bool async, PassRefPtr<FormData> formData, const HTTPHeaderMap& headers, bool includeCredentials, String login, String password)

String method -> const String& method? for all other places also.

> Source/WebCore/inspector/NetworkResourcesData.h:52
> +    String method() { return m_method; }

const String& method() const;

and for other methods also.

> Source/WebCore/inspector/NetworkResourcesData.h:53
> +    bool async() { return m_async; }

bool async() const;

[Build Bot]

Comment on attachment 161641 [details]
Patch

Attachment 161641 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13717093

[Pavel Chadnov]

Created attachment 161651 [details]
Patch

[Pavel Chadnov]

Created attachment 161707 [details]
Patch

[WebKit Review Bot]

Comment on attachment 161707 [details]
Patch

Attachment 161707 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13723250

New failing tests:
animations/suspend-resume-animation-events.html

[Vsevolod Vlasov]

Created attachment 163904 [details]
Patch

[Vsevolod Vlasov]

ap, japhet: Could you please review XMLHttpRequest / DocumentThreadableLoader / CachedResource changes?

[Nate Chapin]

Comment on attachment 163904 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:46
> +#include "SubresourceLoader.h"

I don't think this include is necessary.

> Source/WebCore/loader/cache/CachedRawResource.h:-44
> -    // FIXME: This is exposed for the InpsectorInstrumentation for preflights in DocumentThreadableLoader. It's also really lame.
> -    unsigned long identifier() const { return m_identifier; }
> -

Does this actually need to move down to CachedResource? I don't immediately see a call that requires it.

[Nate Chapin]

Comment on attachment 163904 [details]
Patch

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

> Source/WebCore/loader/DocumentThreadableLoader.cpp:46
> +#include "SubresourceLoader.h"

I don't think this include is necessary.

> Source/WebCore/loader/cache/CachedRawResource.h:-44
> -    // FIXME: This is exposed for the InpsectorInstrumentation for preflights in DocumentThreadableLoader. It's also really lame.
> -    unsigned long identifier() const { return m_identifier; }
> -

Does this actually need to move down to CachedResource? I don't immediately see a call that requires it.

[Vsevolod Vlasov]

(In reply to comment #40)
> (From update of attachment 163904 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163904&action=review
> 
> > Source/WebCore/loader/DocumentThreadableLoader.cpp:46
> > +#include "SubresourceLoader.h"
> 
> I don't think this include is necessary.
> 
It is not, thanks

> > Source/WebCore/loader/cache/CachedRawResource.h:-44
> > -    // FIXME: This is exposed for the InpsectorInstrumentation for preflights in DocumentThreadableLoader. It's also really lame.
> > -    unsigned long identifier() const { return m_identifier; }
> > -
> 
> Does this actually need to move down to CachedResource? I don't immediately see a call that requires it.
It does not need it, I will move it back.

[WebKit Review Bot]

Comment on attachment 163904 [details]
Patch

Attachment 163904 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13849390

New failing tests:
http/tests/inspector/network/network-xhr-replay.html

[WebKit Review Bot]

Comment on attachment 163904 [details]
Patch

Attachment 163904 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13858019

New failing tests:
http/tests/inspector/network/network-xhr-replay.html

[Vsevolod Vlasov]

Created attachment 164061 [details]
Patch

[Vsevolod Vlasov]

Created attachment 164070 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 164070 [details]
Patch

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

> Source/WebCore/inspector/InspectorResourceAgent.cpp:-364
> -void InspectorResourceAgent::willLoadXHRSynchronously()

Let's keep will/didLoadXHRSynchronously as before for clarity as discussed off the bug.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:618
> +    xhr->sendFromInspector(xhrReplayData->formData(), code);return;

Remove 'return;'

> Source/WebCore/loader/DocumentThreadableLoader.cpp:410
> +#if ENABLE(INSPECTOR)

No need in this guard as we already have one in the InspectorInstrumentation

[Vsevolod Vlasov]

Committed r128580: <http://trac.webkit.org/changeset/128580>

[Csaba Osztrogonác]

(In reply to comment #48)
> Committed r128580: <http://trac.webkit.org/changeset/128580>

It broke the !ENABLE(INSPECTOR) builds:
----------------------------------------
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/loader/DocumentThreadableLoader.cpp: In member function 'void WebCore::DocumentThreadableLoader::loadRequest(const WebCore::ResourceRequest&, WebCore::SecurityCheckPolicy)':
/ramdisk/qt-linux-release-minimal/build/Source/WebCore/loader/DocumentThreadableLoader.cpp:410: error: 'InspectorInstrumentation' has not been declared

Could you fix it, please? It seems only an ifdef guard is missing.

[Vsevolod Vlasov]

(In reply to comment #49)
> (In reply to comment #48)
> > Committed r128580: <http://trac.webkit.org/changeset/128580>
> 
> It broke the !ENABLE(INSPECTOR) builds:
> ----------------------------------------
> /ramdisk/qt-linux-release-minimal/build/Source/WebCore/loader/DocumentThreadableLoader.cpp: In member function 'void WebCore::DocumentThreadableLoader::loadRequest(const WebCore::ResourceRequest&, WebCore::SecurityCheckPolicy)':
> /ramdisk/qt-linux-release-minimal/build/Source/WebCore/loader/DocumentThreadableLoader.cpp:410: error: 'InspectorInstrumentation' has not been declared
> 
> Could you fix it, please? It seems only an ifdef guard is missing.

Should be fixed, thanks!

[Zoltan Arvai]

http/tests/inspector/network/network-xhr-replay.html crashes on EFL, Lion, MountenLion, Qt. Can you check it, please?

[Zoltan Arvai]

(In reply to comment #51)
> http/tests/inspector/network/network-xhr-replay.html crashes on EFL, Lion, MountenLion, Qt. Can you check it, please?

On Qt http/tests/inspector/network/network-xhr-same-url-as-main-resource.html is crashing, sorry.