Bug 92108 – Web Inspector: Resource agent's reference to cached resources should be weak.

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

[Vsevolod Vlasov]

Patch to follow

[Vsevolod Vlasov]

Created attachment 154051 [details]
Patch

[Pavel Feldman]

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.

[Vsevolod Vlasov]

Created attachment 154351 [details]
Patch

[Yury Semikhatsky]

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?

[Yury Semikhatsky]

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.

[Pavel Feldman]

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

[Vsevolod Vlasov]

Committed r123715: <http://trac.webkit.org/changeset/123715>

[WebKit Review Bot]

Re-opened since this is blocked by 92356

[Vsevolod Vlasov]

Created attachment 154590 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 154590 [details]
Patch

Attachment 154590 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13351684

[Build Bot]

Comment on attachment 154590 [details]
Patch

Attachment 154590 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13349985

[Vsevolod Vlasov]

Created attachment 154874 [details]
Patch

[WebKit Review Bot]

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.

[Timothy Hatcher]

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.

[Build Bot]

Comment on attachment 154874 [details]
Patch

Attachment 154874 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13372702

[Vsevolod Vlasov]

Created attachment 155178 [details]
Patch

[Vsevolod Vlasov]

Committed r124000: <http://trac.webkit.org/changeset/124000>

[Chris Dumez]

This patch appears to cause crashes at least on EFL port. I opened Bug 92628 to address it.

[Chris Dumez]

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.

[WebKit Review Bot]

Re-opened since this is blocked by 92632

[Vsevolod Vlasov]

Committed r124032: <http://trac.webkit.org/changeset/124032>