Bug 61205 - Web Inspector: Cache XHR content in backend, do not use initialContentSet for XHRs.
Summary: Web Inspector: Cache XHR content in backend, do not use initialContentSet for...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 62799
Blocks: 62476
  Show dependency treegraph
 
Reported: 2011-05-20 11:45 PDT by Vsevolod Vlasov
Modified: 2011-06-16 09:53 PDT (History)
13 users (show)

See Also:


Attachments
Patch (48.92 KB, patch)
2011-05-20 14:33 PDT, Vsevolod Vlasov
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch with qt build fix (48.91 KB, patch)
2011-05-23 05:42 PDT, Vsevolod Vlasov
yurys: review-
yurys: commit-queue-
Details | Formatted Diff | Diff
Patch with fixes (59.29 KB, patch)
2011-05-25 05:32 PDT, Vsevolod Vlasov
yurys: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.24 MB, application/zip)
2011-05-25 06:19 PDT, WebKit Review Bot
no flags Details
Updated test expectations (59.31 KB, patch)
2011-05-25 07:56 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (rebaselined) (61.69 KB, patch)
2011-05-25 11:25 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (with fixes) (62.02 KB, patch)
2011-05-27 05:00 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (another one with fixes) (61.60 KB, patch)
2011-05-27 07:17 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (61.62 KB, patch)
2011-05-27 07:29 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
patch (minor tests fixes and NetworkResourcesData::clear() rewritten) (62.21 KB, patch)
2011-06-09 09:39 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
patch with fixes (64.08 KB, patch)
2011-06-10 06:09 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
patch with fixes + added assertion to NetworkResourcesData::resourceCreated (63.85 KB, patch)
2011-06-10 07:07 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
patch with fixes (65.50 KB, patch)
2011-06-14 10:39 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
patch with fixes (65.48 KB, patch)
2011-06-15 06:04 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
patch with build fixes (67.86 KB, patch)
2011-06-16 04:47 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
patch with failing tests fixes (68.50 KB, patch)
2011-06-16 09:34 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2011-05-20 11:45:46 PDT
Cache XHR content in backend, do not use initialContentSet for XHRs.
Comment 1 Vsevolod Vlasov 2011-05-20 14:33:55 PDT
Created attachment 94272 [details]
Patch
Comment 2 Early Warning System Bot 2011-05-20 14:48:07 PDT
Comment on attachment 94272 [details]
Patch

Attachment 94272 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8723270
Comment 3 Vsevolod Vlasov 2011-05-23 05:42:24 PDT
Created attachment 94405 [details]
Patch with qt build fix
Comment 4 Yury Semikhatsky 2011-05-23 08:05:17 PDT
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.
Comment 5 Vsevolod Vlasov 2011-05-25 05:32:53 PDT
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.
Comment 6 WebKit Review Bot 2011-05-25 06:19:49 PDT
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
Comment 7 WebKit Review Bot 2011-05-25 06:19:55 PDT
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
Comment 8 Vsevolod Vlasov 2011-05-25 07:56:24 PDT
Created attachment 94782 [details]
Updated test expectations
Comment 9 Vsevolod Vlasov 2011-05-25 11:25:39 PDT
Created attachment 94812 [details]
Patch (rebaselined)
Comment 10 Yury Semikhatsky 2011-05-26 05:48:21 PDT
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.
Comment 11 Vsevolod Vlasov 2011-05-27 05:00:09 PDT
Created attachment 95157 [details]
Patch (with fixes)
Comment 12 Vsevolod Vlasov 2011-05-27 05:01:32 PDT
(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.
Comment 13 Yury Semikhatsky 2011-05-27 06:54:38 PDT
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
Comment 14 Yury Semikhatsky 2011-05-27 07:03:09 PDT
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.
Comment 15 Vsevolod Vlasov 2011-05-27 07:17:13 PDT
Created attachment 95169 [details]
Patch (another one with fixes)
Comment 16 Vsevolod Vlasov 2011-05-27 07:29:05 PDT
Created attachment 95171 [details]
Patch
Comment 17 Vsevolod Vlasov 2011-06-09 09:39:35 PDT
Created attachment 96596 [details]
patch (minor tests fixes and NetworkResourcesData::clear() rewritten)
Comment 18 Andrey Kosyakov 2011-06-09 11:08:10 PDT
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
Comment 19 Andrey Kosyakov 2011-06-09 11:24:22 PDT
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!
Comment 20 Vsevolod Vlasov 2011-06-10 06:09:08 PDT
Created attachment 96741 [details]
patch with fixes
Comment 21 Andrey Kosyakov 2011-06-10 06:34:51 PDT
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.
Comment 22 Vsevolod Vlasov 2011-06-10 07:07:03 PDT
Created attachment 96743 [details]
patch with fixes + added assertion to NetworkResourcesData::resourceCreated
Comment 23 Andrey Kosyakov 2011-06-14 06:06:28 PDT
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.
Comment 24 Pavel Feldman 2011-06-14 06:08:50 PDT
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?
Comment 25 Vsevolod Vlasov 2011-06-14 10:39:04 PDT
Created attachment 97136 [details]
patch with fixes
Comment 26 Pavel Feldman 2011-06-15 04:36:12 PDT
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
Comment 27 Vsevolod Vlasov 2011-06-15 06:04:43 PDT
Created attachment 97281 [details]
patch with fixes
Comment 28 WebKit Review Bot 2011-06-15 08:20:39 PDT
Comment on attachment 97281 [details]
patch with fixes

Clearing flags on attachment: 97281

Committed r88937: <http://trac.webkit.org/changeset/88937>
Comment 29 WebKit Review Bot 2011-06-15 08:20:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Andrey Kosyakov 2011-06-15 09:17:03 PDT
Rolled this back due to mac build breakage as of r88940, reopening.
Comment 31 Vsevolod Vlasov 2011-06-16 04:47:31 PDT
Created attachment 97437 [details]
patch with build fixes
Comment 32 Yury Semikhatsky 2011-06-16 07:14:21 PDT
Comment on attachment 97437 [details]
patch with build fixes

Clearing flags on attachment: 97437

Committed r89025: <http://trac.webkit.org/changeset/89025>
Comment 33 Yury Semikhatsky 2011-06-16 07:14:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Yury Semikhatsky 2011-06-16 08:20:53 PDT
Some layout tests failed on Chromium
http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20%28dbg%29%282%29/builds/5404
Comment 35 Vsevolod Vlasov 2011-06-16 09:34:59 PDT
Created attachment 97452 [details]
patch with failing tests fixes
Comment 36 Pavel Feldman 2011-06-16 09:53:17 PDT
Comment on attachment 97452 [details]
patch with failing tests fixes

Clearing flags on attachment: 97452

Committed r89036: <http://trac.webkit.org/changeset/89036>
Comment 37 Pavel Feldman 2011-06-16 09:53:39 PDT
All reviewed patches have been landed.  Closing bug.