Bug 61205 – Web Inspector: Cache XHR content in backend, do not use initialContentSet for XHRs.

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

[Vsevolod Vlasov]

Cache XHR content in backend, do not use initialContentSet for XHRs.

[Vsevolod Vlasov]

Created attachment 94272 [details]
Patch

[Early Warning System Bot]

Comment on attachment 94272 [details]
Patch

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

[Vsevolod Vlasov]

Created attachment 94405 [details]
Patch with qt build fix

[Yury Semikhatsky]

Comment on attachment 94405 [details]
Patch with qt build fix

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

Looks good overall, several minor comments.

> LayoutTests/http/tests/inspector/network/network-xhr-async.html:25
> +    InspectorTest.evaluateInPage("loadData()");

Please use InspectorTest.evaluateInConsole instead of these two lines.

> LayoutTests/http/tests/inspector/network/network-xhr-async.html:47
> +<p>Tests XHR network resource type and content for synchronous requests.</p>

synchronous -> asynchronous. Also please include a link to the bug.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:441
> +            m_pageAgent->getResourceContent(errorString, resourceData->frameId, resourceData->url, optionalBase64Encode, content);

There is a similar logic on the frontend side. Shouldn't this be done by the frontend in a separate request?

> Source/WebCore/inspector/InspectorResourceAgent.cpp:469
> +NetworkResourceData::NetworkResourceData()

identifier and isXHR initialization is missing.

> Source/WebCore/inspector/InspectorResourceAgent.cpp:532
> +    if (m_loadingXHRSynchronously)

Shouldn't we check that the identifier passed as parameter is the same as the id of currently loading XHR?

> Source/WebCore/inspector/InspectorResourceAgent.cpp:552
> +        if (m_identifierToResourceDataMap.get(resourceData->identifier)->loaderId != preservedLoaderId)

replace with if (resourceData->loaderId != preservedLoaderId) ?

> Source/WebCore/inspector/InspectorResourceAgent.cpp:573
> +        resourceData->content = String();

Remove this line, resourceData destructor will take care of the content string?

> Source/WebCore/inspector/InspectorResourceAgent.h:78
> +struct NetworkResourceData {

This should be moved into NetworkResourcesData as it's a part of the latter.

> Source/WebCore/inspector/InspectorResourceAgent.h:92
> +class NetworkResourcesData {

This class and the struct above should go into their own file.

> Source/WebCore/inspector/front-end/NetworkManager.js:56
> +        // FIXME: We should distinct NetworkResource (NetworkPanel resource) from ResourceRevision (ResourcesPanel/ScriptsPanel resource)

distinct -> distinguish, also please file corresponding bug and put its number next to the FIXME.

> Source/WebCore/xml/XMLHttpRequest.cpp:664
> +        // Inspector needs to know that a particular resource is XHR at the moment when didReceiveResponse is received.

I think you can remove this comment since it's clear enough that there are two calls before and after the synchronous load.

> Source/WebCore/xml/XMLHttpRequest.cpp:1039
> +void XMLHttpRequest::didReceiveAuthenticationCancellation(unsigned long identifier, const ResourceResponse& failureResponse)

Please remove the name of unused param, otherwise you'll break compilation on some platforms.

[Vsevolod Vlasov]

Created attachment 94770 [details]
Patch with fixes

> > LayoutTests/http/tests/inspector/network/network-xhr-async.html:25
> > +    InspectorTest.evaluateInPage("loadData()");
> 
> Please use InspectorTest.evaluateInConsole instead of these two lines.
No, that would be a different thing since I need to wait for XHR loaded callback.

> > LayoutTests/http/tests/inspector/network/network-xhr-async.html:47
> > +<p>Tests XHR network resource type and content for synchronous requests.</p>
> 
> synchronous -> asynchronous. Also please include a link to the bug.
Done.

> > Source/WebCore/inspector/InspectorResourceAgent.cpp:441
> > +            m_pageAgent->getResourceContent(errorString, resourceData->frameId, resourceData->url, optionalBase64Encode, content);
> 
> There is a similar logic on the frontend side. Shouldn't this be done by the frontend in a separate request?
No, the idea is that eventually network panel will always request content from InspectorResourceAgent and Resources Panel will request content from InspectorPageAgent.
This is to be done on the front-end as part of separation of network resources from page resources.
Meanwhile InspectorResourceAgent could need page resources content when requested from network panel. This content is requested here from InspectorPageAgent.

> > Source/WebCore/inspector/InspectorResourceAgent.cpp:469
> > +NetworkResourceData::NetworkResourceData()
> 
> identifier and isXHR initialization is missing.
This constructor was not used, removed.

> > Source/WebCore/inspector/InspectorResourceAgent.cpp:532
> > +    if (m_loadingXHRSynchronously)
> 
> Shouldn't we check that the identifier passed as parameter is the same as the id of currently loading XHR?
As discussed, no we can not and need not to do that. Since in this case XHR is requested synchronously we will not receive any other requests callbacks in between.

> > Source/WebCore/inspector/InspectorResourceAgent.cpp:552
> > +        if (m_identifierToResourceDataMap.get(resourceData->identifier)->loaderId != preservedLoaderId)
> 
> replace with if (resourceData->loaderId != preservedLoaderId) ?
Done.

> > Source/WebCore/inspector/InspectorResourceAgent.cpp:573
> > +        resourceData->content = String();
> 
> Remove this line, resourceData destructor will take care of the content string?
This particular line is fine, but the overall logic around it was flawed, thanks for spotting that.
This is now rewritten with the following approach:
 - m_identifierToResourceDataMap is used to store metadata (loaderId, frameId, url, isXHR, etc.) for all resources.
 - when resource has some content, it is added to m_resourceDataDeque as well
Thus m_identifierToResourceDataMap owns resourceData objects while m_resourceDataDeque remembers the order in which content was added only.
Now resourceData will still be available from identifierToResourceDataMap after we clear its content.

> > Source/WebCore/inspector/InspectorResourceAgent.h:78
> > +struct NetworkResourceData {
> 
> This should be moved into NetworkResourcesData as it's a part of the latter.
Done.

> > Source/WebCore/inspector/InspectorResourceAgent.h:92
> > +class NetworkResourcesData {
> 
> This class and the struct above should go into their own file.
Done.

> > Source/WebCore/inspector/front-end/NetworkManager.js:56
> > +        // FIXME: We should distinct NetworkResource (NetworkPanel resource) from ResourceRevision (ResourcesPanel/ScriptsPanel resource)
> 
> distinct -> distinguish, also please file corresponding bug and put its number next to the FIXME.
'separate' should be even better, done.

> > Source/WebCore/xml/XMLHttpRequest.cpp:664
> > +        // Inspector needs to know that a particular resource is XHR at the moment when didReceiveResponse is received.
> 
> I think you can remove this comment since it's clear enough that there are two calls before and after the synchronous load.
Done.

> > Source/WebCore/xml/XMLHttpRequest.cpp:1039
> > +void XMLHttpRequest::didReceiveAuthenticationCancellation(unsigned long identifier, const ResourceResponse& failureResponse)
> 
> Please remove the name of unused param, otherwise you'll break compilation on some platforms.
Done.

[WebKit Review Bot]

Comment on attachment 94770 [details]
Patch with fixes

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

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

[WebKit Review Bot]

Created attachment 94775 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Vsevolod Vlasov]

Created attachment 94782 [details]
Updated test expectations

[Vsevolod Vlasov]

Created attachment 94812 [details]
Patch (rebaselined)

[Yury Semikhatsky]

Comment on attachment 94770 [details]
Patch with fixes

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

> Source/WebCore/inspector/InspectorResourceAgent.h:61
>  class EventsCollector;

Please move EventsCollector up to keep alphabetic order.

> Source/WebCore/inspector/InspectorResourceAgent.h:124
> +    NetworkResourcesData* resourcesData() { return m_resourcesData.get(); }

resourcesData() is not used outside InspectorResourceAgent, so it should be private or inlined.

> Source/WebCore/inspector/NetworkResourcesData.cpp:94
> +        resourceData->content.append(content);

It may occur that ensureFreeSpace has just removed resourceData from m_resourceDataDeque and reset its accumulated content in which case the content here will be corrupted.

> Source/WebCore/inspector/NetworkResourcesData.h:34
> +#include <wtf/text/StringHash.h>

Remove this include?

> Source/WebCore/inspector/NetworkResourcesData.h:68
> +    ~NetworkResourcesData() { }

style: please move destructor to the next line after the constructor. 

Also, you should delete all values in the destructor. r- for this.

[Vsevolod Vlasov]

Created attachment 95157 [details]
Patch (with fixes)

[Vsevolod Vlasov]

(In reply to comment #10)
> (From update of attachment 94770 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94770&action=review
> 
> > Source/WebCore/inspector/InspectorResourceAgent.h:61
> >  class EventsCollector;
> 
> Please move EventsCollector up to keep alphabetic order.
Done.
 
> > Source/WebCore/inspector/InspectorResourceAgent.h:124
> > +    NetworkResourcesData* resourcesData() { return m_resourcesData.get(); }
> 
> resourcesData() is not used outside InspectorResourceAgent, so it should be private or inlined.
Inlined.
 
> > Source/WebCore/inspector/NetworkResourcesData.cpp:94
> > +        resourceData->content.append(content);
> 
> It may occur that ensureFreeSpace has just removed resourceData from m_resourceDataDeque and reset its accumulated content in which case the content here will be corrupted.
Fixed.
 
> > Source/WebCore/inspector/NetworkResourcesData.h:34
> > +#include <wtf/text/StringHash.h>
> 
> Remove this include?
Changed to WTFString.h
 
> > Source/WebCore/inspector/NetworkResourcesData.h:68
> > +    ~NetworkResourcesData() { }
> 
> style: please move destructor to the next line after the constructor. 
Done.
 
> Also, you should delete all values in the destructor. r- for this.
Done.

[Yury Semikhatsky]

Comment on attachment 95157 [details]
Patch (with fixes)

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

> Source/WebCore/inspector/NetworkResourcesData.cpp:98
> +        if (resourceData->content.isNull())

You should check for resourceData->isContentDeleted after ensureFreeSpace as well as the resourceData->content can be disposed during the call to ensureFreeSpace

[Yury Semikhatsky]

Comment on attachment 95157 [details]
Patch (with fixes)

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

> Source/WebCore/inspector/InspectorResourceAgent.cpp:299
> +    m_resourcesData->willLoadXHRSynchronously();

No need to delegate this calls to the storage, lets inline this and the next method and use m_loadingXHRSynchronously only in InspectorResourceAgent. It should make XHR check in InspectorResourceAgent::didReceiveResponse more clear:

else if (m_loadingXHRSynchronously || m_resourcesData->isXHR(identifier))
    type = InspectorPageAgent::XHRResource;

> Source/WebCore/inspector/NetworkResourcesData.cpp:107
> +    if (m_loadingXHRSynchronously)

This flag should move to InspectorResourceAgent which tracks network activity while this class should play role of storage.

[Vsevolod Vlasov]

Created attachment 95169 [details]
Patch (another one with fixes)

[Vsevolod Vlasov]

Created attachment 95171 [details]
Patch

[Vsevolod Vlasov]

Created attachment 96596 [details]
patch (minor tests fixes and NetworkResourcesData::clear() rewritten)

[Andrey Kosyakov]

Comment on attachment 96596 [details]
patch (minor tests fixes and NetworkResourcesData::clear() rewritten)

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

> LayoutTests/http/tests/inspector/network/network-xhr-async.html:5
> +function loadData()

nit: I think this is common enough to go to inspector-test or some other shared collection of test utilities.

> LayoutTests/http/tests/inspector/network/network-xhr-async.html:10
> +        if (xhr.readyState == XMLHttpRequest.DONE)

nit: ===

> LayoutTests/http/tests/inspector/network/network-xhr-async.html:13
> +    xhr.open('GET', 'resources/resource.php', true);

nit: we prefer double quotes.

> Source/WebCore/inspector/Inspector.json:493
> +                    { "name": "loaderId", "type": "string", "description": "Loader id of the resource to get content for." },

I don't think we need to require loaderId to fetch the resource content. Through the reset of ResourceAgent interface, we presume that resource identifier uniquely identifies the resource. I'm aware of that it's not so in case front-end preserves resource log during navigation, but this should be addressed on front-end rather than exposed in the back-end interface (front-end has loaderIds anyway).

> Source/WebCore/inspector/NetworkResourcesData.cpp:76
> +    if (!m_identifierToResourceDataMap.contains(identifier))

This appears redundant, just do a get() below and check for 0 (or use find).

> Source/WebCore/inspector/NetworkResourcesData.cpp:83
> +    if (!m_identifierToResourceDataMap.contains(identifier))

ditto.

> Source/WebCore/inspector/NetworkResourcesData.cpp:105
> +    if (!m_identifierToResourceDataMap.contains(identifier))

ditto.

> Source/WebCore/inspector/NetworkResourcesData.cpp:131
> +    m_identifierToResourceDataMap.clear();
> +
> +    end = preservedMap.end();
> +    for (it = preservedMap.begin(); it != end; ++it)
> +        m_identifierToResourceDataMap.set(it->first, it->second);
> +}

m_identifierToResourceDataMap.swap(preservedMap);

> Source/WebCore/inspector/NetworkResourcesData.h:53
> +        bool isContentDeleted;

nit: I don't quite like deleted here -- I'd suggest either isContentPurged, or reversing the meaning of the flag and calling it isContentAvailable.

> Source/WebCore/inspector/front-end/NetworkManager.js:53
> +        if (resource.identifier != null && resource.loaderId != null)

nit: resource.identifier && resource.loaderId

[Andrey Kosyakov]

Comment on attachment 96596 [details]
patch (minor tests fixes and NetworkResourcesData::clear() rewritten)

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

> Source/WebCore/inspector/NetworkResourcesData.cpp:88
> +    if (ensureFreeSpace(content.length()) && !resourceData->isContentDeleted) {

I'd rather reverse the order of these checks  -- otherwise we may try to purge some useful data to make space for resource data we won't save anyway.

> Source/WebCore/inspector/NetworkResourcesData.cpp:125
> +    for (it = m_identifierToResourceDataMap.begin(); it != end; ++it) {
> +        ResourceData* resourceData = it->second;
> +        if (!preservedLoaderId.isNull() && resourceData->loaderId == preservedLoaderId)
> +            preservedMap.set(it->first, it->second);
> +    }

So we don't delete actual resource data that we discard from the map. Looks like a leak!

[Vsevolod Vlasov]

Created attachment 96741 [details]
patch with fixes

[Andrey Kosyakov]

Comment on attachment 96741 [details]
patch with fixes

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

A couple of nits, but otherwise looks good. Yury & Pavel, please have a look.

> Source/WebCore/inspector/NetworkResourcesData.cpp:128
> +    m_identifierToResourceDataMap.clear();

This is redundant.

> Source/WebCore/inspector/NetworkResourcesData.h:45
> +        ~ResourceData() { }

Why do we need this?

> Source/WebCore/inspector/NetworkResourcesData.h:73
> +    HashMap<unsigned long, ResourceData*> m_identifierToResourceDataMap;

nit: it might be worth having a typedef for this, given that it's used in a few other places.

[Vsevolod Vlasov]

Created attachment 96743 [details]
patch with fixes + added assertion to NetworkResourcesData::resourceCreated

[Andrey Kosyakov]

Comment on attachment 96743 [details]
patch with fixes + added assertion to NetworkResourcesData::resourceCreated

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

> Source/WebCore/xml/XMLHttpRequest.cpp:1025
> +

Redundant empty line.

[Pavel Feldman]

Comment on attachment 96743 [details]
patch with fixes + added assertion to NetworkResourcesData::resourceCreated

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

> Source/WebCore/inspector/InspectorResourceAgent.cpp:432
> +        *errorString = "No resource with given loaderId and identifier found";

Where is loaderId provided?

> Source/WebCore/inspector/InspectorResourceAgent.cpp:440
> +            *errorString = "No data found for resource with given loaderId and identifier";

ditto

> Source/WebCore/inspector/NetworkResourcesData.cpp:93
> +        resourceData->content.append(content);

Can we do it a bit more efficiently? (a builder?)

> Source/WebCore/inspector/front-end/NetworkPanel.js:814
> +                    resource.forbidContentRequestsFromNetworkAgent();

Can we clear the ids in this case?

[Vsevolod Vlasov]

Created attachment 97136 [details]
patch with fixes

[Pavel Feldman]

Comment on attachment 97136 [details]
patch with fixes

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

> Source/WebCore/inspector/NetworkResourcesData.cpp:153
> +    if (size > maximalResourcesContentSize)

maximum

[Vsevolod Vlasov]

Created attachment 97281 [details]
patch with fixes

[WebKit Review Bot]

Comment on attachment 97281 [details]
patch with fixes

Clearing flags on attachment: 97281

Committed r88937: <http://trac.webkit.org/changeset/88937>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andrey Kosyakov]

Rolled this back due to mac build breakage as of r88940, reopening.

[Vsevolod Vlasov]

Created attachment 97437 [details]
patch with build fixes

[Yury Semikhatsky]

Comment on attachment 97437 [details]
patch with build fixes

Clearing flags on attachment: 97437

Committed r89025: <http://trac.webkit.org/changeset/89025>

[Yury Semikhatsky]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

Some layout tests failed on Chromium
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20%28dbg%29%282%29/builds/5404

[Vsevolod Vlasov]

Created attachment 97452 [details]
patch with failing tests fixes

[Pavel Feldman]

Comment on attachment 97452 [details]
patch with failing tests fixes

Clearing flags on attachment: 97452

Committed r89036: <http://trac.webkit.org/changeset/89036>

[Pavel Feldman]

All reviewed patches have been landed.  Closing bug.