Bug 229632 – Web Inspector: Refactor `WorkerInspectorAgent` to use weak pointers for `WorkerInspectorProxy`s

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Patrick Angle]

<rdar://68207416>

[Patrick Angle]

Created attachment 436698 [details]
Patch v1.0

[Patrick Angle]

Comment on attachment 436698 [details]
Patch v1.0

View in context: https://bugs.webkit.org/attachment.cgi?id=436698&action=review

> Source/WebCore/inspector/agents/InspectorWorkerAgent.cpp:136
> +    ASSERT(!m_connectedProxies.isEmpty());

Oops. Not sure why or how I managed to do this.

[Patrick Angle]

Created attachment 436705 [details]
Patch v1.1 - Fix erroneous assertion change

[Chris Dumez]

Comment on attachment 436705 [details]
Patch v1.1 - Fix erroneous assertion change

View in context: https://bugs.webkit.org/attachment.cgi?id=436705&action=review

> Source/WebCore/inspector/agents/InspectorWorkerAgent.cpp:142
> +        Document& document = downcast<Document>(*proxy.scriptExecutionContext());

auto&

> Source/WebCore/inspector/agents/InspectorWorkerAgent.cpp:152
> +    for (auto proxy : copyToVector(m_connectedProxies.values())) {

auto& ?

> Source/WebCore/workers/WorkerInspectorProxy.h:44
> +class WorkerInspectorProxy : public RefCounted<WorkerInspectorProxy>, public CanMakeWeakPtr<WorkerInspectorProxy> {

Why did you make this RefCounted? I didn't see an obvious reason from this patch.

[Devin Rousso]

Comment on attachment 436705 [details]
Patch v1.1 - Fix erroneous assertion change

View in context: https://bugs.webkit.org/attachment.cgi?id=436705&action=review

> Source/WebCore/inspector/agents/InspectorWorkerAgent.cpp:85
> +    auto proxy = m_connectedProxies.get(workerId);

I think the new rules/guidelines around referencing are that we need to `ref` this so that it's kept alive in case something inside `resumeWorkerIfPaused` were to cause it to be destroyed.
```
    auto proxy = makeRefPtr(m_connectedProxies.get(workerId).get());
```

> Source/WebCore/inspector/agents/InspectorWorkerAgent.cpp:99
> +    auto proxy = m_connectedProxies.get(workerId);

ditto (:85)

> Source/WebCore/workers/WorkerInspectorProxy.cpp:42
> +WeakHashSet<WorkerInspectorProxy>& WorkerInspectorProxy::allWorkerInspectorProxies()

this is a bit odd IMO since we already have code to add/remove instances inside the constructor and destructor, so we're not really taking advantage of the functionality of `WeakPtr`

[Chris Dumez]

(In reply to Devin Rousso from comment #5)
> Comment on attachment 436705 [details]
> Patch v1.1 - Fix erroneous assertion change
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436705&action=review
> 
> > Source/WebCore/inspector/agents/InspectorWorkerAgent.cpp:85
> > +    auto proxy = m_connectedProxies.get(workerId);
> 
> I think the new rules/guidelines around referencing are that we need to
> `ref` this so that it's kept alive in case something inside
> `resumeWorkerIfPaused` were to cause it to be destroyed.
> ```
>     auto proxy = makeRefPtr(m_connectedProxies.get(workerId).get());
> ```
> 
> > Source/WebCore/inspector/agents/InspectorWorkerAgent.cpp:99
> > +    auto proxy = m_connectedProxies.get(workerId);
> 
> ditto (:85)
> 
> > Source/WebCore/workers/WorkerInspectorProxy.cpp:42
> > +WeakHashSet<WorkerInspectorProxy>& WorkerInspectorProxy::allWorkerInspectorProxies()
> 
> this is a bit odd IMO since we already have code to add/remove instances
> inside the constructor and destructor, so we're not really taking advantage
> of the functionality of `WeakPtr`

I don't think this is odd, this is exactly how we're supposed to use WeakHashSet.
We don't want to store raw pointers for security reasons. We still need to remove from the set on destruction because WeakHashSet doesn't do that for you.

[Chris Dumez]

Comment on attachment 436705 [details]
Patch v1.1 - Fix erroneous assertion change

View in context: https://bugs.webkit.org/attachment.cgi?id=436705&action=review

>>> Source/WebCore/inspector/agents/InspectorWorkerAgent.cpp:85
>>> +    auto proxy = m_connectedProxies.get(workerId);
>> 
>> I think the new rules/guidelines around referencing are that we need to `ref` this so that it's kept alive in case something inside `resumeWorkerIfPaused` were to cause it to be destroyed.
>> ```
>>     auto proxy = makeRefPtr(m_connectedProxies.get(workerId).get());
>> ```
> 
> I don't think this is odd, this is exactly how we're supposed to use WeakHashSet.
> We don't want to store raw pointers for security reasons. We still need to remove from the set on destruction because WeakHashSet doesn't do that for you.

Also, please don't use makeRef() / makeRefPtr() as they are deprecated. You could use this if you wanted to ref:
RefPtr proxy  = m_connectedProxies.get(workerId);

[Patrick Angle]

Created attachment 436803 [details]
Patch v1.2 - Address review notes

[Chris Dumez]

Comment on attachment 436803 [details]
Patch v1.2 - Address review notes

View in context: https://bugs.webkit.org/attachment.cgi?id=436803&action=review

r=me with nits.

> Source/WebCore/inspector/agents/InspectorWorkerAgent.cpp:155
> +        if (!proxyWeakPtr)

It looks better do do the null check after assigning to a strong pointer:
```
RefPtr proxy = proxyWeakPtr.get();
if (!proxy)
    continue;
```

> Source/WebCore/workers/WorkerInspectorProxy.h:80
> +    WorkerInspectorProxy(const String& identifier);

Should be marked explicit.

[Patrick Angle]

Created attachment 436832 [details]
Patch v1.3 - Review nits, add reviewer to Changelog

[EWS]

Committed r281787 (241124@main): <https://commits.webkit.org/241124@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436832 [details].