Fix executor used in resolveNode

Author: sjorsdonkers

src/browser/browser.zig

@@ -192,7 +192,7 @@ pub const Session = struct {
         errdefer self.arena.deinit();
 
         self.executor = try browser.env.startExecutor(Window, &self.state, self);
-        errdefer browser.env.stopExecutor(self.executor);
+        errdefer browser.env.stopExecutor(self.executor, true);
         self.inspector = try Env.Inspector.init(self.arena.allocator(), self.executor, ctx);
 
         self.microtaskLoop();
@@ -207,7 +207,7 @@ pub const Session = struct {
         self.arena.deinit();
         self.cookie_jar.deinit();
         self.storage_shed.deinit();
-        self.browser.env.stopExecutor(self.executor);
+        self.browser.env.stopExecutor(self.executor, true);
     }
 
     fn microtaskLoop(self: *Session) void {

src/cdp/cdp.zig

@@ -308,24 +308,6 @@ pub fn BrowserContext(comptime CDP_T: type) type {
 
         isolated_world: ?IsolatedWorld,
 
-        pub fn createIsolatedWorld(
-            self: *Self,
-            world_name: []const u8,
-            grant_universal_access: bool,
-        ) !void {
-            if (self.isolated_world != null) return error.AlreadyExists;
-
-            const executor = try self.cdp.browser.env.startExecutor(@import("../browser/html/window.zig").Window, &self.session.state, self.session);
-            errdefer self.cdp.browser.env.stopExecutor(executor);
-            executor.context.exit();
-
-            self.isolated_world = .{
-                .name = try self.session.arena.allocator().dupe(u8, world_name), // TODO allocator
-                .grant_universal_access = grant_universal_access,
-                .executor = executor,
-            };
-        }
-
         const Self = @This();
 
         fn init(self: *Self, id: []const u8, cdp: *CDP_T) !void {
@@ -352,6 +334,11 @@ pub fn BrowserContext(comptime CDP_T: type) type {
         }
 
         pub fn deinit(self: *Self) void {
+            if (self.isolated_world) |isolated_world| {
+                isolated_world.executor.endScope();
+                self.cdp.browser.env.stopExecutor(isolated_world.executor, false);
+                self.isolated_world = null;
+            }
             self.node_registry.deinit();
             self.node_search_list.deinit();
         }
@@ -361,6 +348,29 @@ pub fn BrowserContext(comptime CDP_T: type) type {
             self.node_search_list.reset();
         }
 
+        pub fn createIsolatedWorld(
+            self: *Self,
+            world_name: []const u8,
+            grant_universal_access: bool,
+        ) !void {
+            if (self.isolated_world != null) return error.CurrentlyOnly1IsolatedWorldSupported;
+
+            const executor = try self.cdp.browser.env.startExecutor(@import("../browser/html/window.zig").Window, &self.session.state, self.session);
+            errdefer self.cdp.browser.env.stopExecutor(executor, true);
+
+            // TBD should we endScope on removePage and re-startScope on createPage?
+            // Window will be refactored into the executor so we leave it ugly here for now as a reminder.
+            try executor.startScope(@import("../browser/html/window.zig").Window{});
+
+            executor.context.exit(); // The default context should remain open
+
+            self.isolated_world = .{
+                .name = try self.session.arena.allocator().dupe(u8, world_name), // TODO allocator
+                .grant_universal_access = grant_universal_access,
+                .executor = executor,
+            };
+        }
+
         pub fn nodeWriter(self: *Self, node: *const Node, opts: Node.Writer.Opts) Node.Writer {
             return .{
                 .node = node,

src/cdp/domains/dom.zig

@@ -134,6 +134,7 @@ fn resolveNode(cmd: anytype) !void {
         if (executor.context.debugContextId() != context_id) {
             const isolated_world = bc.isolated_world orelse return error.ContextNotFound;
             executor = isolated_world.executor;
+
             if (executor.context.debugContextId() != context_id) return error.ContextNotFound;
         }
     }
@@ -143,7 +144,7 @@ fn resolveNode(cmd: anytype) !void {
     // node._node is a *parser.Node we need this to be able to find its most derived type e.g. Node -> Element -> HTMLElement
     // So we use the Node.Union when retrieve the value from the environment
     const remote_object = try bc.session.inspector.getRemoteObject(
-        bc.session.executor,
+        executor,
         params.objectGroup orelse "",
         try dom_node.Node.toInterface(node._node),
     );

src/cdp/domains/page.zig

@@ -84,6 +84,8 @@ fn setLifecycleEventsEnabled(cmd: anytype) !void {
 }
 
 // TODO: hard coded method
+// With the command we receive a script we need to store and run for each new document.
+// Note that the worldName refers to the name given to the isolated world.
 fn addScriptToEvaluateOnNewDocument(cmd: anytype) !void {
     // const params = (try cmd.params(struct {
     //     source: []const u8,
@@ -110,9 +112,10 @@ fn createIsolatedWorld(cmd: anytype) !void {
     }
     const bc = cmd.browser_context orelse return error.BrowserContextNotLoaded;
 
-    try bc.createIsolatedWorld(params.worldName, params.grantUniveralAccess); // orelse return error.IsolatedWorldAlreadyExists;
+    try bc.createIsolatedWorld(params.worldName, params.grantUniveralAccess);
 
-    // Create the auxdata json from
+    // Create the auxdata json for the contextCreated event
+    // Calling contextCreated will assign a Id to the context and send the contextCreated event
     const aux_json = try std.fmt.allocPrint(cmd.arena, "{{\"isDefault\":false,\"type\":\"isolated\",\"frameId\":\"{s}\"}}", .{params.frameId});
     bc.session.inspector.contextCreated(bc.isolated_world.?.executor, bc.isolated_world.?.name, "", aux_json, false);
 
@@ -213,16 +216,19 @@ pub fn pageNavigate(bc: anytype, event: *const Notification.PageNavigate) !void
 
     // Send Runtime.executionContextsCleared event
     // TODO: noop event, we have no env context at this point, is it necesarry?
-    // When we actually recreated the context we should have the inspector send this event, see:  resetContextGroup
+    // When we actually recreated the context we should have the inspector send this event, see: resetContextGroup
+    // Sending this event will tell the client that the context ids they had are invalid and the context shouls be dropped
+    // The client will expect us to send new contextCreated events, such that the client has new id's for the active contexts.
     try cdp.sendEvent("Runtime.executionContextsCleared", null, .{ .session_id = session_id });
 
     if (bc.isolated_world != null) {
         const aux_json = try std.fmt.allocPrint(
             bc.session.arena.allocator(), // TODO change this
             "{{\"isDefault\":false,\"type\":\"isolated\",\"frameId\":\"{s}\"}}",
-            .{bc.target_id.?}, // TODO check this
+            .{bc.target_id.?},
         );
 
+        // Calling contextCreated will assign a new Id to the context and send the contextCreated event
         bc.session.inspector.contextCreated(
             bc.isolated_world.?.executor,
             bc.isolated_world.?.name,

src/runtime/js.zig

@@ -348,7 +348,7 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
                 },
             };
 
-            errdefer self.stopExecutor(executor);
+            errdefer self.stopExecutor(executor, true); // Note: This likely has issues as context.exit() is errdefered as well
 
             // Custom exception
             // NOTE: there is no way in v8 to subclass the Error built-in type
@@ -369,8 +369,8 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
         // a Context, it's managed by the garbage collector. So, when the
         // `gc_hints` option is enabled, we'll use the `lowMemoryNotification`
         // call on the isolate to encourage v8 to free the context.
-        pub fn stopExecutor(self: *Self, executor: *Executor) void {
-            executor.deinit();
+        pub fn stopExecutor(self: *Self, executor: *Executor, exit_context: bool) void {
+            executor.deinit(exit_context);
             self.executor_pool.destroy(executor);
             if (self.gc_hints) {
                 self.isolate.lowMemoryNotification();
@@ -789,11 +789,11 @@ pub fn Env(comptime S: type, comptime types: anytype) type {
             // no init, must be initialized via env.startExecutor()
 
             // not public, must be destroyed via env.stopExecutor()
-            fn deinit(self: *Executor) void {
+            fn deinit(self: *Executor, exit_context: bool) void {
                 if (self.scope) |*s| {
                     s.deinit();
                 }
-                self.context.exit();
+                if (exit_context) self.context.exit();
                 self.handle_scope.deinit();
                 self.call_arena.deinit();
                 self.scope_arena.deinit();