| Summary: | Web Inspector: Resource agent's reference to cached resources should be weak. | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Vsevolod Vlasov <vsevik> | ||||||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Vsevolod Vlasov <vsevik> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | apavlov, bweinstein, cdumez, gustavo, japhet, jochen, joepeck, keishi, loislo, pfeldman, philn, pmuellr, rik, timothy, webkit.review.bot, xan.lopez, yurys | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Bug Depends on: | 92356, 92628, 92632 | ||||||||||||||
| Bug Blocks: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Vsevolod Vlasov
2012-07-24 06:02:22 PDT
Created attachment 154051 [details]
Patch
Comment on attachment 154051 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154051&action=review This change looks like a hack both from the API and implementation standpoint. Is there a way to implement it in a more elegant manner? > Source/WebCore/inspector/Inspector.json:871 > + { "name": "resourceId", "$ref": "Page.ResourceId", "description": "Identifier of the relevant resource." }, I don't think we should introduce resourceId concept. Why is requestId not sufficient in the API? > Source/WebCore/inspector/InspectorController.cpp:186 > + InspectorInstrumentation::inspectedPageDestroyed(m_instrumentingAgents.get()); This is very strange - InspectorController has pointers to agents explicitly as well as to m_instrumentingAgents. Why does it use InspectorInstrumentation ? > Source/WebCore/inspector/InspectorInstrumentation.cpp:854 > + if (!cachedResourceToAgentsMap || !cachedResourceToAgentsMap->contains(cachedResource)) InspectorInstrumentation is just a dispatch, it should not have logic like this. > Source/WebCore/inspector/InspectorInstrumentation.cpp:1167 > +void InspectorInstrumentation::inspectedPageDestroyed(InstrumentingAgents* instrumentingAgents) ditto > Source/WebCore/inspector/InspectorPageAgent.cpp:960 > + InspectorInstrumentation::addInstrumentingAgentsForCachedResource(cachedResource, m_instrumentingAgents); This looks like a hack. Created attachment 154351 [details]
Patch
Comment on attachment 154351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154351&action=review > Source/WebCore/inspector/InspectorInstrumentation.cpp:1160 > +void InspectorInstrumentation::registerInstrumentingAgents(InstrumentingAgents* instrumentingAgents) Why not iterate through all pages (see allPages in Page.cpp) and get InstrumentingAgents from Page instead of introducing a HashSet of all InstrumentingAgents instances? Comment on attachment 154351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154351&action=review > Source/WebCore/inspector/InspectorInstrumentation.h:98 > + friend class InspectorController; I'd rather make (un)registerInstrumentingAgents methods public instead of adding friend class. Comment on attachment 154351 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154351&action=review How do we test this? > Source/WebCore/loader/cache/CachedResource.cpp:434 > + if (!deleteIfPossible() && !hasClients() && inCache()) { bool deleted = ... > Source/WebCore/loader/cache/CachedResourceLoader.cpp:816 > + if (!res->deleteIfPossible() && res->preloadResult() == CachedResource::PreloadNotReferenced) ditto Committed r123715: <http://trac.webkit.org/changeset/123715> Re-opened since this is blocked by 92356 Created attachment 154590 [details]
Patch
Comment on attachment 154590 [details] Patch Attachment 154590 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13351684 Comment on attachment 154590 [details] Patch Attachment 154590 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13349985 Created attachment 154874 [details]
Patch
Comment on attachment 154351 [details] Patch Cleared Pavel Feldman's review+ from obsolete attachment 154351 [details] so that this bug does not appear in http://webkit.org/pending-commit. Comment on attachment 154874 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=154874&action=review > Source/WebCore/inspector/NetworkResourcesData.h:97 > + size_t dataLength() const; > + void appendData(const char* data, size_t dataLength); > + size_t decodeDataToContent(); All these int => size_t chanegs should be done in a seperate change. It has nothing to do with making the resources weak, it is just noise here. Comment on attachment 154874 [details] Patch Attachment 154874 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13372702 Created attachment 155178 [details]
Patch
Committed r124000: <http://trac.webkit.org/changeset/124000> This patch appears to cause crashes at least on EFL port. I opened Bug 92628 to address it. Comment on attachment 155178 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155178&action=review > Source/WebCore/inspector/InspectorInstrumentation.cpp:1173 > + delete instrumentingAgentsSet; I believe we should reset instrumentingAgentsSet to 0 after calling delete. Re-opened since this is blocked by 92632 Committed r124032: <http://trac.webkit.org/changeset/124032> |