Reorganize v8 contexts and scope#578
Conversation
- Pages within the same session have proper isolation - they have their own window - they have their own SessionState - they have their own v8.Context - Move inspector to CDP browser context - Browser now knows nothing about the inspector - Use notification to emit a context-created message - This is still a bit hacky, but again, it decouples browser from CDP
emit contextCreated when it's needed, not when it actually happens. I thought we could make this sync-up, but we'd need to create 3 contexts to satisfy both puppeteer and chromedp. So rather than having it partially driven by notifications from Browser, I rather just fake it all for now.
|
I initially wanted to have the It turns out that: No doubt this is how chrome actually behaves. A context for the tab. A context for the page (and then one per frame) and, if you create an isolated world, a context for that. Since we aren't currently creating 3 contexts, I decided to go back to fully faking this. However thanks to (1) from the PR details above, this is less of a lie than it was before and thanks to (2) this mess is completely removed from Browser - it's purely a CDP concern. Now, whether we actually need 3+ distinct contexts created, and whether they need to be created at those exact points in time, is still TDB. |
karlseguin
force-pushed
the
scope_tightening
branch
from
e22ec72 to
c31e2d9
Compare
| _ = page_arena.reset(.{ .retain_with_limit = 1 * 1024 * 1024 }); | ||
|
|
||
| self.page = Page.init(self); | ||
| self.page = @as(Page, undefined); |
There was a problem hiding this comment.
Is this OK?
In Page.init we are using the values of the current state as input for the new state!
There was a problem hiding this comment.
I'm not sure what you mean by "the values of the current state". Do you mean things like:
self.* = .{
.url = URL.empty,
.state = .{
.url = &self.url,
}
};Isn't state.url being given a valid/correct address? The value might not be initialized, but it will be by the time the statement end.
| var world = &self.isolated_world.?; | ||
|
|
||
| // TODO: can we do something better than passing `undefined` for the state? | ||
| world.scope = try world.executor.startScope(&world.global, undefined, {}); |
There was a problem hiding this comment.
Just a note, doesn't need to be fixed in this PR.
(When grantUniversalAccess is true)
I'm fairly certain we should put in the page's Window, such that they share the same DOM tree.
I expect we keep running into issues if we combine steps like creating a scope and a context.
3 participants
PR does 2 cleanups and 1 hack.
1 - Moves a lot of the
ExecutorandSessionlogic/data down to theScopeand thePage. This provides correct isolation when navigating from page-to-page and minimizes some of the v8 scoping. A Page/Scope now owns thewindow(globla),session_state(ooopps, should rename this topage_state?) and its own v8.Context.2 - Moves the inspector out of the
Sessionand into theBrowserContext. The inspector, it's callback hookup, it'saux_dataand everything else, is fully managed by the CDP layer. Browser/Session are now inspector-unaware.3 - To fix the isolated world crash, I force-create the isolated world before the page is created and always shutdown the page before shutting down the isolated world.
(3) is a hack, but I'm still not sure what the correct long term solution is, but I think (1) and (2) move us in a better direction.