Bug 229632 - Web Inspector: Refactor `WorkerInspectorAgent` to use weak pointers for `WorkerInspectorProxy`s
Summary: Web Inspector: Refactor `WorkerInspectorAgent` to use weak pointers for `Work...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Patrick Angle
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-27 15:15 PDT by Patrick Angle
Modified: 2021-08-30 18:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch v1.0 (9.86 KB, patch)
2021-08-27 17:54 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.1 - Fix erroneous assertion change (9.74 KB, patch)
2021-08-27 22:57 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.2 - Address review notes (9.86 KB, patch)
2021-08-30 13:12 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.3 - Review nits, add reviewer to Changelog (9.86 KB, patch)
2021-08-30 17:24 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2021-08-27 15:15:52 PDT
<rdar://68207416>
Comment 1 Patrick Angle 2021-08-27 17:54:52 PDT
Created attachment 436698 [details]
Patch v1.0
Comment 2 Patrick Angle 2021-08-27 22:54:41 PDT
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.
Comment 3 Patrick Angle 2021-08-27 22:57:37 PDT
Created attachment 436705 [details]
Patch v1.1 - Fix erroneous assertion change
Comment 4 Chris Dumez 2021-08-30 09:35:25 PDT
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.
Comment 5 Devin Rousso 2021-08-30 11:56:48 PDT
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`
Comment 6 Chris Dumez 2021-08-30 12:04:22 PDT
(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.
Comment 7 Chris Dumez 2021-08-30 12:05:48 PDT
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);
Comment 8 Patrick Angle 2021-08-30 13:12:40 PDT
Created attachment 436803 [details]
Patch v1.2 - Address review notes
Comment 9 Chris Dumez 2021-08-30 13:34:48 PDT
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.
Comment 10 Patrick Angle 2021-08-30 17:24:45 PDT
Created attachment 436832 [details]
Patch v1.3 - Review nits, add reviewer to Changelog
Comment 11 EWS 2021-08-30 18:42:02 PDT
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].