-
Notifications
You must be signed in to change notification settings - Fork 287
Recreate fake isolated world when cleared #533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,35 +98,46 @@ fn addScriptToEvaluateOnNewDocument(cmd: anytype) !void { | |
| } | ||
|
|
||
| // TODO: hard coded method | ||
| /// @isolated_world The current understanding is that an isolated world should be a separate isolate and context | ||
| /// that would live in the BrowserContext. We think Puppetee creates this to be able to create variables | ||
| /// that are not interfering with the normal namespace of he webpage. | ||
| /// Similar to the main context we need to pretend to recreate it after a executionContextsCleared event | ||
| /// which happens when navigating to a new page. | ||
| /// Since we do not actually create an isolated context operations on this context are still performed | ||
| /// in the main context, we suspect this may lead to unexpected variables and value overwrites. | ||
| fn createIsolatedWorld(cmd: anytype) !void { | ||
| _ = cmd.browser_context orelse return error.BrowserContextNotLoaded; | ||
|
|
||
| const session_id = cmd.input.session_id orelse return error.SessionIdRequired; | ||
|
|
||
| const params = (try cmd.params(struct { | ||
| frameId: []const u8, | ||
| worldName: []const u8, | ||
| grantUniveralAccess: bool, | ||
| })) orelse return error.InvalidParams; | ||
|
|
||
| // noop executionContextCreated event | ||
| try cmd.sendEvent("Runtime.executionContextCreated", .{ | ||
| .context = runtime.ExecutionContextCreated{ | ||
| .id = 0, | ||
| .origin = "", | ||
| .name = params.worldName, | ||
| // TODO: hard coded ID | ||
| .uniqueId = "7102379147004877974.3265385113993241162", | ||
| .auxData = .{ | ||
| .isDefault = false, | ||
| .type = "isolated", | ||
| .frameId = params.frameId, | ||
| }, | ||
| const bc = cmd.browser_context orelse return error.BrowserContextNotLoaded; | ||
| const session_id = cmd.input.session_id orelse return error.SessionIdRequired; | ||
|
|
||
| const name_copy = try bc.session.arena.allocator().dupe(u8, params.worldName); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could also give the bc its own arena. My only concern with using the Session's arena is that it's easy to forget the BrowserContext is using it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. One more push to making me think that we should stop with the OO separation of these data structs, especially when we start sending notifications from one to the other, while they are just next to each other. |
||
| const frame_id_copy = try bc.session.arena.allocator().dupe(u8, params.frameId); | ||
| const fake_id = 0; | ||
|
|
||
| bc.fake_isolatedworld = .{ | ||
| .id = fake_id, | ||
| .origin = "", // The 2nd time chrome sends this it is "://" | ||
| .name = name_copy, | ||
| // TODO: hard coded ID, should change when context is recreated | ||
| .uniqueId = "7102379147004877974.3265385113993241162", | ||
| .auxData = .{ | ||
| .isDefault = false, | ||
| .type = "isolated", | ||
| .frameId = frame_id_copy, | ||
| }, | ||
| }, .{ .session_id = session_id }); | ||
| }; | ||
|
|
||
| return cmd.sendResult(.{ | ||
| .executionContextId = 0, | ||
| // Inform the client of the creation of the isolated world and its ID. | ||
| // Note: Puppeteer uses the ID from this event and actually ignores the executionContextId return value | ||
| try cmd.sendEvent("Runtime.executionContextCreated", .{ .context = bc.fake_isolatedworld }, .{ .session_id = session_id }); | ||
|
|
||
| try cmd.sendResult(.{ | ||
| .executionContextId = fake_id, | ||
| }, .{}); | ||
| } | ||
|
|
||
|
|
@@ -202,9 +213,14 @@ pub fn pageNavigate(bc: anytype, event: *const Notification.PageEvent) !void { | |
| }, .{ .session_id = session_id }); | ||
| } | ||
|
|
||
| // Send Runtime.executionContextsCleared event | ||
| // TODO: noop event, we have no env context at this point, is it necesarry? | ||
| // Sending this events will tell make the client drop its contexts and wait for new ones to be created. | ||
| try cdp.sendEvent("Runtime.executionContextsCleared", null, .{ .session_id = session_id }); | ||
|
|
||
| // When the execution contexts are cleared the client expect us to send executionContextCreated events with the new ID for each context. | ||
| // Since we do not actually maintain an isolated context we just send the same message as we did initially. | ||
| // The contextCreated message is send for the main context by the session by calling the inspector.contextCreated when navigating. | ||
| if (bc.fake_isolatedworld) |isolatedworld| try cdp.sendEvent("Runtime.executionContextCreated", .{ .context = isolatedworld }, .{ .session_id = session_id }); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI: you can capture a reference to it: if (bc.fake_isolatedworld) |*isolatedworld| { .... }
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good to know. Generally I don't worry about stack copies. I'd be surprised if this would end up in the final binary. |
||
| } | ||
|
|
||
| pub fn pageNavigated(bc: anytype, event: *const Notification.PageEvent) !void { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not against having a
src/cdp/execution_context.zigwith aCreatedstruct inside. Also can keep as-is.