-
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
Conversation
| node_search_list: Node.Search.List, | ||
|
|
||
| // Holds the data to be able recreate the fake isolated world, see: @isolated_world | ||
| fake_isolatedworld: ?@import("domains/runtime.zig").ExecutionContextCreated, |
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.zig with a Created struct inside. Also can keep as-is.
| 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); |
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.
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.
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.
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.
| // 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 }); |
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.
FYI: you can capture a reference to it:
if (bc.fake_isolatedworld) |*isolatedworld| { .... }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.
Good to know. Generally I don't worry about stack copies. I'd be surprised if this would end up in the final binary.
|
PR put back into draft as the fake isolated context seems like a rabbit hole |
Based on #524 as describeNode is needed to recreate the issue this solves.
When executionContextsCleared is called puppetteer drops all references to contexts it had.
When navigating we already send a executionContextCreated message for the main context, however we did not in the case the client had requested an isolated world.
Puppetteer seems to be using the isolated world to make sure that what it is doing does not interfere with the variables of the actual page. We however still us the main context to do everything, and send a fake context id 0.
This PR continues the hack by making sure we resend the isolated context right after send the executionContextsCleared event.
NOTE: Likely need to change the target before merging!