Skip to content

refactor: remove gin patch to notify microtask runner disposal - #53759

Draft
deepak1556 wants to merge 12 commits into
robo/cppgc_web_contents_cleanupfrom
robo/cppgc_microtasks_runner_cleanup
Draft

refactor: remove gin patch to notify microtask runner disposal#53759
deepak1556 wants to merge 12 commits into
robo/cppgc_web_contents_cleanupfrom
robo/cppgc_microtasks_runner_cleanup

Conversation

@deepak1556

Copy link
Copy Markdown
Member

Description of Change

Refs #47922

Release Notes

Notes: none

deepak1556 and others added 12 commits September 7, 2026 22:59
Following CL https://crrev.com/c/8279909. Remove the worker specific
holder registry and let unified heap tracing manage callback lifetimes.

Move function and object template caches from isolate scope to gin context
data so discarded contexts can be collected. Install lifecycle owned gin
context data for Node workers without requiring gin isolate data, and
detach it during environment cleanup.
Move the native resources owned by api::WebContents and every native
registration that routes callbacks back into it (WebContentsObserver,
WebContentsDelegate, InputEventObserver, the InspectableWebContents
delegates, BackgroundThrottlingSource and ExclusiveAccessContext) into a
nested NativeLifecycle helper that forwards through a weak link.

Isolating the native teardown from the wrapper is a prerequisite for
managing api::WebContents with cppgc, where the wrapper is reclaimed by
the collector instead of being deleted at a well defined point. No
behavior change, the wrapper is still a DeprecatedWrappable that is
pinned and deleted explicitly.
Move api::WebContents onto the cppgc managed gin::Wrappable, so the
wrapper participates in unified heap tracing instead of being pinned and
deleted by hand. NativeLifecycle now holds a weak link into the GC
heap, registers a dispose observer to drain native resources before
isolate shutdown, and is deleted in a follow up task so native teardown
never runs during sweeping.
The agent host keeps a raw client pointer. If the Debugger was still
attached when cppgc finalized it, a later ForceDetachAllSessions() at
quit called AgentHostClosed() on freed memory.
content makes the new host current and can run embedder callbacks
(focus/blur from CommitPending) before RenderFrameHostChanged re-keys the
existing WebFrameMain, so touching webContents.mainFrame in that window
constructed a second instance for the same FrameTreeNode and CHECKed.
Also dispose WebFrameMains of a WebContents that api::WebContents stops
observing without owning (webview guests, background pages), and replace
a stale FrameTreeNode entry instead of CHECKing.
api::WebContents was the last user of gin_helper::DeprecatedWrappable and
gin_helper::Pinnable, so drop both along with the generic helper
overloads and template bookkeeping that only existed to keep the
deprecated wrapper info working.

The gin patch that restored the deprecated wrapper utilities can go with
them, but the gin::PerIsolateData hook that notifies dispose observers
before the microtasks runner is disposed is still needed, so reduce the
patch to just that hook.
@deepak1556 deepak1556 added the semver/patch backwards-compatible bug fixes label Sep 9, 2026
@deepak1556
deepak1556 added this pull request to stack #53756 September 9, 2026 04:43
@electron-cation electron-cation Bot added the new-pr 🌱 PR opened recently label Sep 9, 2026
@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently label Sep 10, 2026
Comment on lines 83 to 86
NotificationDelegateProxy(v8::Isolate* isolate, Notification* notification)
: isolate_(isolate), notification_(notification) {
gin::PerIsolateData::From(isolate_)->AddDisposeObserver(this);
MicrotasksRunner::AddObserver(this);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: nothing reads isolate_ anymore now that both sides go through MicrotasksRunner, so the member and the v8::Isolate* ctor param can both go.

@ckerr
ckerr self-requested a review September 10, 2026 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-backport semver/patch backwards-compatible bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants