<rdar://68207416>
Created attachment 436698 [details] Patch v1.0
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.
Created attachment 436705 [details] Patch v1.1 - Fix erroneous assertion change
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 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`
(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 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);
Created attachment 436803 [details] Patch v1.2 - Address review notes
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.
Created attachment 436832 [details] Patch v1.3 - Review nits, add reviewer to Changelog
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].