Merge pull request #3664 from lightpanda-io/always-detach-global

v8: always detach global

src/browser/Session.zig

@@ -743,7 +743,6 @@ fn _processFrameNavigation(self: *Session, frame: *Frame, qn: *QueuedNavigation)
     const frame_id = frame._frame_id;
     const reuse_window = frame.window;
     const page = frame.page;
-    frame.js.detachGlobal();
     frame.deinit();
     frame.* = undefined;
 
@@ -794,7 +793,6 @@ fn processPopupNavigation(_: *Session, frame: *Frame, qn: *QueuedNavigation) !vo
     const frame_id = frame._frame_id;
     const page = frame.page;
 
-    frame.js.detachGlobal();
     frame.deinit();
     frame.* = undefined;
 

src/browser/js/Context.zig

@@ -216,6 +216,14 @@ pub fn deinit(self: *Context) void {
     // have a dangling pointer to our freed Context struct.
     v8.v8__Context__SetAlignedPointerInEmbedderData(entered.handle, 1, null);
 
+    // Detach the global so that a navigation can attach the reused Window to
+    // the frame's next context. This also nulls v8's pointer to our
+    // microtask_queue, which we free below. The v8 context can outlive us when
+    // another realm holds one of our functions, e.g. as a promise handler, and
+    // resolving that promise would enqueue onto the freed queue. With the
+    // pointer null, v8 drops the job instead.
+    v8.v8__Context__DetachGlobal(entered.handle);
+
     v8.v8__Global__Reset(&self.handle);
     env.isolate.notifyContextDisposed();
     // There can be other tasks associated with this context that we need to
@@ -224,24 +232,6 @@ pub fn deinit(self: *Context) void {
     v8.v8__MicrotaskQueue__DELETE(self.microtask_queue);
 }
 
-// The global (e.g. Window) can be reused across contexts. If you do:
-//
-// var w = iframe.contentWindow;
-// iframe.src = 'two.html';
-// w === iframe.contentWindow  (must be true)
-//
-// so when we navigate, the Window/Global is re-used. That's fine with v8, but
-// we need to explicitly detach it from the original before we can safely attach
-// it to the new
-pub fn detachGlobal(self: *Context) void {
-    var hs: js.HandleScope = undefined;
-    hs.init(self.isolate);
-    defer hs.deinit();
-
-    const local_v8_context: *const v8.Context = @ptrCast(v8.v8__Global__Get(&self.handle, self.isolate.handle));
-    v8.v8__Context__DetachGlobal(local_v8_context);
-}
-
 // setOrigin is called at navigation (opaque -> real origin) and again when a
 // script sets document.domain (real origin -> '!'-marked effective domain).
 pub fn setOrigin(self: *Context, key: ?[]const u8) !void {

src/browser/tests/frames/detached_realm_promise_handler.html

@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<head></head>
+<body>
+<script src="../testing.js"></script>
+
+<!--
+Navigating a frame frees the contexts of its child frames, but their v8
+contexts live on while another realm holds one of their functions. Resolving
+a promise whose handler is such a function used to enqueue the job on the
+freed microtask queue (SIGFPE / SIGSEGV in MicrotaskQueue::EnqueueMicrotask).
+-->
+
+<script id=detached_realm_promise_handler type=module>
+  const state = await testing.async();
+
+  const iframe = document.createElement('iframe');
+  const onceLoad = (f) => new Promise((r) => { f.onload = () => r(); });
+
+  let loaded = onceLoad(iframe);
+  iframe.src = 'support/win_a.html';
+  document.body.appendChild(iframe);
+  await loaded;
+
+  const inner = iframe.contentDocument.createElement('iframe');
+  iframe.contentDocument.body.appendChild(inner);
+
+  let ran = false;
+  const grandchild = inner.contentWindow;
+  const handler = new grandchild.Function('cb', 'return () => cb()')(() => { ran = true; });
+
+  loaded = onceLoad(iframe);
+  iframe.src = 'support/win_b.html';
+  await loaded;
+
+  // fetch() resolves natively, which enqueues the handler's job directly.
+  await Promise.all(Array.from({ length: 50 }, (_, i) => {
+    const p = fetch('support/win_a.html?' + i);
+    p.then(handler);
+    return p;
+  }));
+
+  state.resolve();
+  await state.done(() => {
+    // The grandchild's document is gone, so its job is dropped.
+    testing.expectEqual(false, ran);
+  });
+</script>
+</body>

src/server/cdp/CDP.zig

@@ -1417,10 +1417,6 @@ pub const IsolatedWorld = struct {
     }
 
     fn destroyFrameContext(self: *IsolatedWorld, fc: FrameContext) void {
-        // A re-navigating child frame keeps its Window, and the identity map
-        // keeps the window's global proxy; detach it from this context so the
-        // frame's next context can reattach it (as the main world does).
-        fc.context.detachGlobal();
         self.browser.env.destroyContext(fc.context);
         fc.call_arena.release();
         fc.local_arena.release();