Summary: | Web Inspector: Cache XHR content in backend, do not use initialContentSet for XHRs. | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vsevolod Vlasov <vsevik> | ||||||||||||||||||||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, caseq, dglazkov, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||
Bug Depends on: | 62799 | ||||||||||||||||||||||||||||||||||||
Bug Blocks: | 62476 | ||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Vsevolod Vlasov
2011-05-20 11:45:46 PDT
Created attachment 94272 [details]
Patch
Comment on attachment 94272 [details] Patch Attachment 94272 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8723270 Created attachment 94405 [details]
Patch with qt build fix
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. 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 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 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
Created attachment 94782 [details]
Updated test expectations
Created attachment 94812 [details]
Patch (rebaselined)
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. Created attachment 95157 [details]
Patch (with fixes)
(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 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 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. Created attachment 95169 [details]
Patch (another one with fixes)
Created attachment 95171 [details]
Patch
Created attachment 96596 [details]
patch (minor tests fixes and NetworkResourcesData::clear() rewritten)
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 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! Created attachment 96741 [details]
patch with fixes
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. Created attachment 96743 [details]
patch with fixes + added assertion to NetworkResourcesData::resourceCreated
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 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? Created attachment 97136 [details]
patch with fixes
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 Created attachment 97281 [details]
patch with fixes
Comment on attachment 97281 [details] patch with fixes Clearing flags on attachment: 97281 Committed r88937: <http://trac.webkit.org/changeset/88937> All reviewed patches have been landed. Closing bug. Rolled this back due to mac build breakage as of r88940, reopening. Created attachment 97437 [details]
patch with build fixes
Comment on attachment 97437 [details] patch with build fixes Clearing flags on attachment: 97437 Committed r89025: <http://trac.webkit.org/changeset/89025> All reviewed patches have been landed. Closing bug. Some layout tests failed on Chromium http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20%28dbg%29%282%29/builds/5404 Created attachment 97452 [details]
patch with failing tests fixes
Comment on attachment 97452 [details] patch with failing tests fixes Clearing flags on attachment: 97452 Committed r89036: <http://trac.webkit.org/changeset/89036> All reviewed patches have been landed. Closing bug. |