Bug 276167 - Audit all uses of IsStrongCallback
Summary: Audit all uses of IsStrongCallback
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 276191 276267 276277 276322 276332 276344
Blocks:
  Show dependency treegraph
 
Reported: 2024-07-02 23:26 PDT by Ryan Reno
Modified: 2024-07-16 10:49 PDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Reno 2024-07-02 23:26:21 PDT
As of 280611@main callbacks are now Weak by default. The owning JS wrapper/C++ object needs to manage the callback lifetime. In the interim, the prior behavior of callbacks being Strong GC roots is maintained via the IsStrongCallback attribute. We should audit all of these callbacks and properly manage the lifetime of the object via visitAdditionalChildren/WebCore Opaque Roots, and any other lifetime management scheme we have available to us.

Strong callbacks are a bit of a memory leak trap so ideally we can remove all uses of IsStrongCallback in time.
Comment 1 Radar WebKit Bug Importer 2024-07-02 23:26:29 PDT
<rdar://problem/131026926>
Comment 2 Ryan Reno 2024-07-05 14:24:29 PDT
I'm learning that it's probably not ideal to remove ALL uses of IsStrongCallback. I've found two legitimate uses so far. One is when the callback is "ephemeral". In other words, if there is no owning object and it is scheduled to run in the next run loop/upon promise resolution (examples are requestAnimationFrame's callback and Notification.requestPermission, respectively).

Additionally, if the owning object is an ActiveDOMObject, we can remove the Ref/RefPtr to the callback in ActiveDOMObject::stop which greatly simplifies lifetime management over using JSC::Weak and visitAdditionalChildren.
Comment 3 Ryan Reno 2024-07-16 10:49:53 PDT
After discussing with rniwa we settled on a design which allows us to remove IsStrongCallback entirely which landed in 280975@main