Bug 92108 - Web Inspector: Resource agent's reference to cached resources should be weak.
Summary: Web Inspector: Resource agent's reference to cached resources should be weak.
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: Vsevolod Vlasov
URL:
Keywords:
Depends on: 92356 92628 92632
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-24 06:02 PDT by Vsevolod Vlasov
Modified: 2012-07-30 08:59 PDT (History)
17 users (show)

See Also:


Attachments
Patch (52.76 KB, patch)
2012-07-24 07:07 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (26.36 KB, patch)
2012-07-25 07:37 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (40.17 KB, patch)
2012-07-26 02:34 PDT, Vsevolod Vlasov
gns: commit-queue-
Details | Formatted Diff | Diff
Patch (42.78 KB, patch)
2012-07-27 01:20 PDT, Vsevolod Vlasov
no flags Details | Formatted Diff | Diff
Patch (45.21 KB, patch)
2012-07-29 13:02 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 2012-07-24 06:02:22 PDT
Patch to follow
Comment 1 Vsevolod Vlasov 2012-07-24 07:07:04 PDT
Created attachment 154051 [details]
Patch
Comment 2 Pavel Feldman 2012-07-25 02:33:15 PDT
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.
Comment 3 Vsevolod Vlasov 2012-07-25 07:37:51 PDT
Created attachment 154351 [details]
Patch
Comment 4 Yury Semikhatsky 2012-07-25 08:14:47 PDT
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 5 Yury Semikhatsky 2012-07-25 08:20:09 PDT
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 6 Pavel Feldman 2012-07-25 08:24:41 PDT
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
Comment 7 Vsevolod Vlasov 2012-07-26 01:48:26 PDT
Committed r123715: <http://trac.webkit.org/changeset/123715>
Comment 8 WebKit Review Bot 2012-07-26 02:23:01 PDT
Re-opened since this is blocked by 92356
Comment 9 Vsevolod Vlasov 2012-07-26 02:34:42 PDT
Created attachment 154590 [details]
Patch
Comment 10 Gustavo Noronha (kov) 2012-07-26 06:35:17 PDT
Comment on attachment 154590 [details]
Patch

Attachment 154590 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13351684
Comment 11 Build Bot 2012-07-26 14:04:44 PDT
Comment on attachment 154590 [details]
Patch

Attachment 154590 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13349985
Comment 12 Vsevolod Vlasov 2012-07-27 01:20:31 PDT
Created attachment 154874 [details]
Patch
Comment 13 WebKit Review Bot 2012-07-27 07:32:09 PDT
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 14 Timothy Hatcher 2012-07-27 08:26:50 PDT
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 15 Build Bot 2012-07-27 14:52:51 PDT
Comment on attachment 154874 [details]
Patch

Attachment 154874 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13372702
Comment 16 Vsevolod Vlasov 2012-07-29 13:02:36 PDT
Created attachment 155178 [details]
Patch
Comment 17 Vsevolod Vlasov 2012-07-30 00:55:16 PDT
Committed r124000: <http://trac.webkit.org/changeset/124000>
Comment 18 Chris Dumez 2012-07-30 04:20:04 PDT
This patch appears to cause crashes at least on EFL port. I opened Bug 92628 to address it.
Comment 19 Chris Dumez 2012-07-30 04:21:28 PDT
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.
Comment 20 WebKit Review Bot 2012-07-30 04:42:06 PDT
Re-opened since this is blocked by 92632
Comment 21 Vsevolod Vlasov 2012-07-30 08:59:10 PDT
Committed r124032: <http://trac.webkit.org/changeset/124032>