Skip to content

Commit a3e2b52

Browse files
karlseguinkrichprollsch
authored andcommitted
Make CDP server more authoritative with respect to IDs
The TL;DR is that this commit enforces the use of correct IDs, introduces a BrowserContext, and adds some CDP tests. These are the ids we need to be aware of when talking about CDP: - id - browserContextId - targetId - sessionId - loaderId - frameId The `id` is the only one that _should_ originate from the driver. It's attached to most messages and it's how we maintain a request -> response flow: when the server responds to a specific message, it echo's back the id from the requested message. (As opposed to out-of-band events sent from the server which won't have an `id`). When I say "id" from this point forward, I mean every id except for this req->res id. Every other id is created by the browser. Prior to this commit, we didn't really check incoming ids from the driver. If the driver said "attachToTarget" and included a targetId, we just assumed that this was the current targetId. This was aided by the fact that we only used hard-coded IDS. If _we_ only "create" a frameId of "FRAME-1", then it's tempting to think the driver will only ever send a frameId of "FRAME-1". The issue with this approach is that _if_ the browser and driver fall out of sync and there's only ever 1 browserContextId, 1 sessionId and 1 frameId, it's not impossible to imagine cases where we behave on the thing. Imagine this flow: - Driver asks for a new BrowserContext - Browser says OK, your browserContextId is 1 - Driver, for whatever reason, says close browserContextId 2 - Browser says, OK, but it doesn't check the id and just closes the only BrowserContext it knows about (which is 1) By both re-using the same hard-coded ids, and not verifying that the ids sent from the client correspond to the correct ids, any issues are going to be hard to debug. Currently LOADER_ID and FRAEM_ID are still hard-coded. Baby steps.
1 parent ccacac0 commit a3e2b52

File tree

17 files changed

+1097
-560
lines changed

17 files changed

+1097
-560
lines changed

src/browser/browser.zig

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,6 @@ pub const Browser = struct {
9898
self.session = null;
9999
}
100100
}
101-
102-
pub fn currentPage(self: *Browser) ?*Page {
103-
if (self.session.page == null) return null;
104-
105-
return &self.session.page.?;
106-
}
107101
};
108102

109103
// Session is like a browser's tab.

src/cdp/browser.zig

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ pub fn processMessage(cmd: anytype) !void {
3333
setDownloadBehavior,
3434
getWindowForTarget,
3535
setWindowBounds,
36-
}, cmd.action) orelse return error.UnknownMethod;
36+
}, cmd.input.action) orelse return error.UnknownMethod;
3737

3838
switch (action) {
3939
.getVersion => return getVersion(cmd),
@@ -88,7 +88,6 @@ test "cdp.browser: getVersion" {
8888

8989
try ctx.processMessage(.{
9090
.id = 32,
91-
.sessionID = "leto",
9291
.method = "Browser.getVersion",
9392
});
9493

@@ -99,7 +98,7 @@ test "cdp.browser: getVersion" {
9998
.revision = REVISION,
10099
.userAgent = USER_AGENT,
101100
.jsVersion = JS_VERSION,
102-
}, .{ .id = 32, .index = 0 });
101+
}, .{ .id = 32, .index = 0, .session_id = null });
103102
}
104103

105104
test "cdp.browser: getWindowForTarget" {
@@ -108,13 +107,12 @@ test "cdp.browser: getWindowForTarget" {
108107

109108
try ctx.processMessage(.{
110109
.id = 33,
111-
.sessionId = "leto",
112110
.method = "Browser.getWindowForTarget",
113111
});
114112

115113
try ctx.expectSentCount(1);
116114
try ctx.expectSentResult(.{
117115
.windowId = DEV_TOOLS_WINDOW_ID,
118116
.bounds = .{ .windowState = "normal" },
119-
}, .{ .id = 33, .index = 0, .session_id = "leto" });
117+
}, .{ .id = 33, .index = 0, .session_id = null });
120118
}

0 commit comments

Comments
 (0)