fix: do not throw synchronously when a CDP session is gone (#2466)
`PuppeteerDevToolsConnection.send()` is declared to return a promise of
a result or an error, but it throws synchronously when the session id no
longer resolves. The generated agent code does not catch that, so it
escapes as an uncaught exception and ends the process.
A session going away while commands for it are in flight is normal
rather than exceptional. The way I kept hitting it is closing a page
while its source maps are still loading:
```
Error: Unknown session 5C39E3164EE6C2C4AB1D3788F5AE03DA
at PuppeteerDevToolsConnection.send (build/src/devtools/DevToolsConnectionAdapter.js:31:19)
at AgentPrototype.invoke [as invoke_close]
at IOModel.close
at PageResourceLoader.loadFromTarget
at async PageResourceLoader.dispatchLoad
at async PageResourceLoader.loadResource
at async loadSourceMap
```
Source maps are just the most likely way to be holding a load open long
enough to notice; any in-flight command to a session that goes away does
the same thing. In the server this means closing a tab at the wrong
moment can take everything down.
This returns an error response instead, which is how a failed command is
already reported a few lines below, so callers see it through the path
they already handle.
The other synchronous throw in that method, for `sessionId ===
undefined`, is left alone: it guards an invariant the callers are
supposed to uphold, so failing loudly there seems right. Happy to change
that too if you would rather have both consistent.
The test fails on `main` with `Error: Unknown session
session-that-went-away` and passes with the change.
Found while working on #2431, but unrelated to it and independent of
#2463.
This commit is contained in:
@@ -56,7 +56,17 @@ export class PuppeteerDevToolsConnection
|
||||
}
|
||||
const session = this.#connection.session(sessionId);
|
||||
if (!session) {
|
||||
throw new Error('Unknown session ' + sessionId);
|
||||
// A session can go away while commands for it are still in flight, for
|
||||
// example when a page is closed while its source maps are loading. That
|
||||
// is normal, so report it the same way a failed command is reported.
|
||||
// Throwing here would escape synchronously out of a method declared to
|
||||
// return a promise, past the generated agent code, and end the process.
|
||||
return Promise.resolve({
|
||||
error: {
|
||||
code: -32000,
|
||||
message: 'Unknown session ' + sessionId,
|
||||
} as DevTools.CDPConnection.CDPError,
|
||||
});
|
||||
}
|
||||
// Rolled protocol version between puppeteer and DevTools doesn't necessarily match
|
||||
/* eslint-disable @typescript-eslint/no-explicit-any */
|
||||
|
||||
@@ -0,0 +1,68 @@
|
||||
/**
|
||||
* @license
|
||||
* Copyright 2026 Google LLC
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
import assert from 'node:assert';
|
||||
import {describe, it} from 'node:test';
|
||||
|
||||
import {PuppeteerDevToolsConnection} from '../../src/devtools/DevToolsConnectionAdapter.js';
|
||||
import type {CDPSession} from '../../src/third_party/index.js';
|
||||
import {mockListener} from '../utils.js';
|
||||
|
||||
function getMockSession(sessionsById: Record<string, unknown>): CDPSession {
|
||||
const connection = {
|
||||
session(id: string) {
|
||||
return sessionsById[id];
|
||||
},
|
||||
};
|
||||
return {
|
||||
...mockListener(),
|
||||
id() {
|
||||
return 'root-session';
|
||||
},
|
||||
connection() {
|
||||
return connection;
|
||||
},
|
||||
} as unknown as CDPSession;
|
||||
}
|
||||
|
||||
describe('PuppeteerDevToolsConnection', () => {
|
||||
it('reports an unknown session as an error instead of throwing', async () => {
|
||||
const session = getMockSession({'root-session': {}});
|
||||
const connection = new PuppeteerDevToolsConnection(session);
|
||||
|
||||
// The page this session belonged to is gone, which is what happens when a
|
||||
// tab closes while commands for it are still in flight.
|
||||
const response = await connection.send(
|
||||
'Network.loadNetworkResource' as never,
|
||||
{} as never,
|
||||
'session-that-went-away',
|
||||
);
|
||||
|
||||
assert.ok('error' in response, 'expected an error response');
|
||||
assert.match(response.error.message, /Unknown session/);
|
||||
});
|
||||
|
||||
it('forwards commands to the session it belongs to', async () => {
|
||||
const calls: string[] = [];
|
||||
const target = {
|
||||
send(method: string) {
|
||||
calls.push(method);
|
||||
return Promise.resolve({ok: true});
|
||||
},
|
||||
};
|
||||
const session = getMockSession({'a-session': target});
|
||||
const connection = new PuppeteerDevToolsConnection(session);
|
||||
|
||||
const response = await connection.send(
|
||||
'Page.enable' as never,
|
||||
{} as never,
|
||||
'a-session',
|
||||
);
|
||||
|
||||
assert.deepStrictEqual(calls, ['Page.enable']);
|
||||
assert.deepStrictEqual(response, {result: {ok: true}});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user