Bug 95187

Summary: Web Inspector: Add ability to replay XHR in network panel.
Product: WebKit Reporter: Pavel Chadnov <chadnov>
Component: Web Inspector (Deprecated)Assignee: Vsevolod Vlasov <vsevik>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, ap, bweinstein, dglazkov, gtk-ews, gustavo, japhet, joepeck, keishi, loislo, ossy, peter+ews, pfeldman, pmuellr, rik, timothy, vsevik, webkit.review.bot, xan.lopez, yurys, zarvai
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: https://code.google.com/p/chromium/issues/detail?id=107006
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch yurys: review+

Description Pavel Chadnov 2012-08-28 03:21:55 PDT
Ability to replay existing XHR.
Comment 1 Pavel Chadnov 2012-08-28 04:39:33 PDT
Created attachment 160952 [details]
Patch
Comment 2 Vsevolod Vlasov 2012-08-28 05:45:59 PDT
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.
Comment 3 Vsevolod Vlasov 2012-08-28 05:48:19 PDT
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.
Comment 4 Pavel Chadnov 2012-08-28 06:28:02 PDT
Created attachment 160961 [details]
Patch
Comment 5 Vsevolod Vlasov 2012-08-28 06:39:05 PDT
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.
Comment 6 Vsevolod Vlasov 2012-08-28 06:53:48 PDT
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
Comment 7 Pavel Chadnov 2012-08-29 07:14:53 PDT
Created attachment 161220 [details]
Patch
Comment 8 Build Bot 2012-08-29 07:45:13 PDT
Comment on attachment 161220 [details]
Patch

Attachment 161220 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13686209
Comment 9 Build Bot 2012-08-29 07:49:20 PDT
Comment on attachment 161220 [details]
Patch

Attachment 161220 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13683283
Comment 10 Vsevolod Vlasov 2012-08-29 08:00:45 PDT
Comment on attachment 161220 [details]
Patch

Clearing r? as some comments were not addressed yet.
Comment 11 Pavel Chadnov 2012-08-29 08:25:47 PDT
Created attachment 161232 [details]
Patch
Comment 12 Build Bot 2012-08-29 09:14:46 PDT
Comment on attachment 161232 [details]
Patch

Attachment 161232 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13685292
Comment 13 Pavel Chadnov 2012-08-29 10:01:28 PDT
Created attachment 161256 [details]
Patch
Comment 14 Build Bot 2012-08-29 10:45:12 PDT
Comment on attachment 161256 [details]
Patch

Attachment 161256 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13680301
Comment 15 Build Bot 2012-08-29 10:55:22 PDT
Comment on attachment 161256 [details]
Patch

Attachment 161256 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13682283
Comment 16 Vsevolod Vlasov 2012-08-29 12:51:20 PDT
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.
Comment 17 Vsevolod Vlasov 2012-08-29 23:37:12 PDT
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)
Comment 18 Pavel Chadnov 2012-08-30 07:58:10 PDT
Created attachment 161470 [details]
Patch
Comment 19 Vsevolod Vlasov 2012-08-30 08:14:57 PDT
Comment on attachment 161470 [details]
Patch

This patch looks identical to the previous one.
Comment 20 Pavel Chadnov 2012-08-30 08:20:10 PDT
Created attachment 161475 [details]
Patch
Comment 21 WebKit Review Bot 2012-08-30 08:29:14 PDT
Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13694695
Comment 22 Gyuyoung Kim 2012-08-30 08:36:29 PDT
Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13682716
Comment 23 Build Bot 2012-08-30 08:37:07 PDT
Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13681720
Comment 24 Early Warning System Bot 2012-08-30 08:43:14 PDT
Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13690727
Comment 25 Early Warning System Bot 2012-08-30 08:47:32 PDT
Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13686692
Comment 26 Peter Beverloo (cr-android ews) 2012-08-30 08:52:27 PDT
Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13695710
Comment 27 Build Bot 2012-08-30 08:52:38 PDT
Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13686693
Comment 28 kov's GTK+ EWS bot 2012-08-30 08:59:56 PDT
Comment on attachment 161475 [details]
Patch

Attachment 161475 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13690732
Comment 29 Pavel Chadnov 2012-08-30 09:05:12 PDT
Created attachment 161487 [details]
Patch
Comment 30 Build Bot 2012-08-30 09:45:30 PDT
Comment on attachment 161487 [details]
Patch

Attachment 161487 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13682752
Comment 31 Vsevolod Vlasov 2012-08-30 23:59:04 PDT
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)
Comment 32 Pavel Chadnov 2012-08-31 01:55:14 PDT
Created attachment 161641 [details]
Patch
Comment 33 Andrey Adaikin 2012-08-31 02:22:39 PDT
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;
Comment 34 Build Bot 2012-08-31 02:25:25 PDT
Comment on attachment 161641 [details]
Patch

Attachment 161641 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13717093
Comment 35 Pavel Chadnov 2012-08-31 03:24:20 PDT
Created attachment 161651 [details]
Patch
Comment 36 Pavel Chadnov 2012-08-31 08:35:28 PDT
Created attachment 161707 [details]
Patch
Comment 37 WebKit Review Bot 2012-08-31 09:27:40 PDT
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
Comment 38 Vsevolod Vlasov 2012-09-13 10:32:35 PDT
Created attachment 163904 [details]
Patch
Comment 39 Vsevolod Vlasov 2012-09-13 10:33:24 PDT
ap, japhet: Could you please review XMLHttpRequest / DocumentThreadableLoader / CachedResource changes?
Comment 40 Nate Chapin 2012-09-13 10:51:56 PDT
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.
Comment 41 Nate Chapin 2012-09-13 10:51:56 PDT
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.
Comment 42 Vsevolod Vlasov 2012-09-13 11:01:20 PDT
(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.
Comment 43 WebKit Review Bot 2012-09-13 19:04:47 PDT
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
Comment 44 WebKit Review Bot 2012-09-13 20:01:55 PDT
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
Comment 45 Vsevolod Vlasov 2012-09-13 23:59:39 PDT
Created attachment 164061 [details]
Patch
Comment 46 Vsevolod Vlasov 2012-09-14 01:47:48 PDT
Created attachment 164070 [details]
Patch
Comment 47 Yury Semikhatsky 2012-09-14 02:44:16 PDT
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
Comment 48 Vsevolod Vlasov 2012-09-14 03:41:31 PDT
Committed r128580: <http://trac.webkit.org/changeset/128580>
Comment 49 Csaba Osztrogon√°c 2012-09-14 04:04:16 PDT
(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.
Comment 50 Vsevolod Vlasov 2012-09-14 04:38:03 PDT
(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!
Comment 51 Zoltan Arvai 2012-09-14 05:37:15 PDT
http/tests/inspector/network/network-xhr-replay.html crashes on EFL, Lion, MountenLion, Qt. Can you check it, please?
Comment 52 Zoltan Arvai 2012-09-14 05:41:04 PDT
(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.