Skip to content

Commit

Permalink
fix: don't wait for activation if the connection is disconnected (#13591
Browse files Browse the repository at this point in the history
)
  • Loading branch information
Lightning00Blade authored Feb 7, 2025
1 parent 26e98ba commit 6b20ac1
Show file tree
Hide file tree
Showing 13 changed files with 96 additions and 67 deletions.
7 changes: 0 additions & 7 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -347,13 +347,6 @@ jobs:
PUPPETEER_SKIP_DOWNLOAD: true
# Set up GitHub Actions caching for Wireit.
- uses: google/wireit@eea3c9f0385a39e6eb4ff6a6daa273311381d436 # setup-github-actions-caching/v2.0.2
- name: Setup cache for browsers
uses: actions/cache@1bd1e32a3bdc45362d1e726936510720a7c30a57 # v4.2.0
with:
path: ~/.cache/puppeteer/
key: Browsers-${{ runner.os }}-${{ hashFiles('packages/puppeteer-core/src/revisions.ts') }}-${{ hashFiles('packages/puppeteer/src/node/install.ts') }}
- name: Install browsers
run: npm run postinstall
- name: Test install
env:
PKG_MANAGER: ${{ matrix.pkg_manager }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ export class CdpCDPSession extends CDPSession {
#sessionId: string;
#targetType: string;
#callbacks = new CallbackRegistry();
#connection?: Connection;
#connection: Connection;
#parentSessionId?: string;
#target?: CdpTarget;
#rawErrors = false;

#detached = false;
/**
* @internal
*/
Expand All @@ -56,7 +56,7 @@ export class CdpCDPSession extends CDPSession {
*
* @internal
*/
_setTarget(target: CdpTarget): void {
setTarget(target: CdpTarget): void {
this.#target = target;
}

Expand All @@ -65,7 +65,7 @@ export class CdpCDPSession extends CDPSession {
*
* @internal
*/
_target(): CdpTarget {
target(): CdpTarget {
assert(this.#target, 'Target must exist');
return this.#target;
}
Expand All @@ -74,6 +74,10 @@ export class CdpCDPSession extends CDPSession {
return this.#connection;
}

get #closed(): boolean {
return this.#connection._closed || this.#detached;
}

override parentSession(): CDPSession | undefined {
if (!this.#parentSessionId) {
// In some cases, e.g., DevTools pages there is no parent session. In this
Expand All @@ -89,7 +93,7 @@ export class CdpCDPSession extends CDPSession {
params?: ProtocolMapping.Commands[T]['paramsType'][0],
options?: CommandOptions,
): Promise<ProtocolMapping.Commands[T]['returnType']> {
if (!this.#connection) {
if (this.#closed) {
return Promise.reject(
new TargetCloseError(
`Protocol error (${method}): Session closed. Most likely the ${this.#targetType} has been closed.`,
Expand All @@ -108,7 +112,7 @@ export class CdpCDPSession extends CDPSession {
/**
* @internal
*/
_onMessage(object: {
onMessage(object: {
id?: number;
method: keyof CDPEvents;
params: CDPEvents[keyof CDPEvents];
Expand Down Expand Up @@ -140,22 +144,23 @@ export class CdpCDPSession extends CDPSession {
* won't emit any events and can't be used to send messages.
*/
override async detach(): Promise<void> {
if (!this.#connection) {
if (this.#closed) {
throw new Error(
`Session already detached. Most likely the ${this.#targetType} has been closed.`,
);
}
await this.#connection.send('Target.detachFromTarget', {
sessionId: this.#sessionId,
});
this.#detached = true;
}

/**
* @internal
*/
_onClosed(): void {
onClosed(): void {
this.#callbacks.clear();
this.#connection = undefined;
this.#detached = true;
this.emit(CDPSessionEvent.Disconnected, undefined);
}

Expand Down
8 changes: 4 additions & 4 deletions packages/puppeteer-core/src/cdp/Connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {TargetCloseError} from '../common/Errors.js';
import {EventEmitter} from '../common/EventEmitter.js';
import {createProtocolErrorMessage} from '../util/ErrorLike.js';

import {CdpCDPSession} from './CDPSession.js';
import {CdpCDPSession} from './CdpSession.js';

const debugProtocolSend = debug('puppeteer:protocol:SEND ►');
const debugProtocolReceive = debug('puppeteer:protocol:RECV ◀');
Expand Down Expand Up @@ -174,7 +174,7 @@ export class Connection extends EventEmitter<CDPSessionEvents> {
} else if (object.method === 'Target.detachedFromTarget') {
const session = this.#sessions.get(object.params.sessionId);
if (session) {
session._onClosed();
session.onClosed();
this.#sessions.delete(object.params.sessionId);
this.emit(CDPSessionEvent.SessionDetached, session);
const parentSession = this.#sessions.get(object.sessionId);
Expand All @@ -186,7 +186,7 @@ export class Connection extends EventEmitter<CDPSessionEvents> {
if (object.sessionId) {
const session = this.#sessions.get(object.sessionId);
if (session) {
session._onMessage(object);
session.onMessage(object);
}
} else if (object.id) {
if (object.error) {
Expand Down Expand Up @@ -216,7 +216,7 @@ export class Connection extends EventEmitter<CDPSessionEvents> {
this.#transport.onclose = undefined;
this.#callbacks.clear();
for (const session of this.#sessions.values()) {
session._onClosed();
session.onClosed();
}
this.#sessions.clear();
this.emit(CDPSessionEvent.Disconnected, undefined);
Expand Down
13 changes: 10 additions & 3 deletions packages/puppeteer-core/src/cdp/FrameManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {isErrorLike} from '../util/ErrorLike.js';

import type {Binding} from './Binding.js';
import {CdpPreloadScript} from './CdpPreloadScript.js';
import {CdpCDPSession} from './CDPSession.js';
import {CdpCDPSession} from './CdpSession.js';
import {isTargetClosedError} from './Connection.js';
import {DeviceRequestPromptManager} from './DeviceRequestPrompt.js';
import {ExecutionContext} from './ExecutionContext.js';
Expand Down Expand Up @@ -103,6 +103,13 @@ export class FrameManager extends EventEmitter<FrameManagerEvents> {
if (!mainFrame) {
return;
}

if (this.client.connection()?._closed) {
// On connection disconnected remove all frames
this.#removeFramesRecursively(mainFrame);
return;
}

for (const child of mainFrame.childFrames()) {
this.#removeFramesRecursively(child);
}
Expand Down Expand Up @@ -133,9 +140,9 @@ export class FrameManager extends EventEmitter<FrameManagerEvents> {
);
const frame = this._frameTree.getMainFrame();
if (frame) {
this.#frameNavigatedReceived.add(this.#client._target()._targetId);
this.#frameNavigatedReceived.add(this.#client.target()._targetId);
this._frameTree.removeFrame(frame);
frame.updateId(this.#client._target()._targetId);
frame.updateId(this.#client.target()._targetId);
this._frameTree.addFrame(frame);
frame.updateClient(client);
}
Expand Down
18 changes: 9 additions & 9 deletions packages/puppeteer-core/src/cdp/Page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ import {AsyncDisposableStack} from '../util/disposable.js';
import {isErrorLike} from '../util/ErrorLike.js';

import {Binding} from './Binding.js';
import {CdpCDPSession} from './CDPSession.js';
import {CdpCDPSession} from './CdpSession.js';
import {isTargetClosedError} from './Connection.js';
import {Coverage} from './Coverage.js';
import type {DeviceRequestPrompt} from './DeviceRequestPrompt.js';
Expand Down Expand Up @@ -145,7 +145,7 @@ export class CdpPage extends Page {
this.#primaryTargetClient = client;
this.#tabTargetClient = client.parentSession()!;
assert(this.#tabTargetClient, 'Tab target session is not defined.');
this.#tabTarget = (this.#tabTargetClient as CdpCDPSession)._target();
this.#tabTarget = (this.#tabTargetClient as CdpCDPSession).target();
assert(this.#tabTarget, 'Tab target is not defined.');
this.#primaryTarget = target;
this.#targetManager = target._targetManager();
Expand Down Expand Up @@ -262,7 +262,7 @@ export class CdpPage extends Page {
this.#primaryTargetClient instanceof CdpCDPSession,
'CDPSession is not instance of CDPSessionImpl',
);
this.#primaryTarget = this.#primaryTargetClient._target();
this.#primaryTarget = this.#primaryTargetClient.target();
assert(this.#primaryTarget, 'Missing target on swap');
this.#keyboard.updateClient(newSession);
this.#mouse.updateClient(newSession);
Expand All @@ -276,7 +276,7 @@ export class CdpPage extends Page {

async #onSecondaryTarget(session: CDPSession): Promise<void> {
assert(session instanceof CdpCDPSession);
if (session._target()._subtype() !== 'prerender') {
if (session.target()._subtype() !== 'prerender') {
return;
}
this.#frameManager.registerSpeculativeSession(session).catch(debugError);
Expand Down Expand Up @@ -327,13 +327,13 @@ export class CdpPage extends Page {

#onAttachedToTarget = (session: CDPSession) => {
assert(session instanceof CdpCDPSession);
this.#frameManager.onAttachedToTarget(session._target());
if (session._target()._getTargetInfo().type === 'worker') {
this.#frameManager.onAttachedToTarget(session.target());
if (session.target()._getTargetInfo().type === 'worker') {
const worker = new CdpWebWorker(
session,
session._target().url(),
session._target()._targetId,
session._target().type(),
session.target().url(),
session.target()._targetId,
session.target().type(),
this.#addConsoleMessage.bind(this),
this.#handleException.bind(this),
);
Expand Down
6 changes: 3 additions & 3 deletions packages/puppeteer-core/src/cdp/Target.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {debugError} from '../common/util.js';
import type {Viewport} from '../common/Viewport.js';
import {Deferred} from '../util/Deferred.js';

import {CdpCDPSession} from './CDPSession.js';
import {CdpCDPSession} from './CdpSession.js';
import {CdpPage} from './Page.js';
import type {TargetManager} from './TargetManager.js';
import {CdpWebWorker} from './WebWorker.js';
Expand Down Expand Up @@ -66,7 +66,7 @@ export class CdpTarget extends Target {
this._targetId = targetInfo.targetId;
this.#sessionFactory = sessionFactory;
if (this.#session && this.#session instanceof CdpCDPSession) {
this.#session._setTarget(this);
this.#session.setTarget(this);
}
}

Expand Down Expand Up @@ -114,7 +114,7 @@ export class CdpTarget extends Target {
throw new Error('sessionFactory is not initialized');
}
return this.#sessionFactory(false).then(session => {
(session as CdpCDPSession)._setTarget(this);
(session as CdpCDPSession).setTarget(this);
return session;
});
}
Expand Down
8 changes: 4 additions & 4 deletions packages/puppeteer-core/src/cdp/TargetManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {debugError} from '../common/util.js';
import {assert} from '../util/assert.js';
import {Deferred} from '../util/Deferred.js';

import type {CdpCDPSession} from './CDPSession.js';
import type {CdpCDPSession} from './CdpSession.js';
import type {Connection} from './Connection.js';
import {CdpTarget, InitializationStatus} from './Target.js';
import type {TargetManagerEvents} from './TargetManageEvents.js';
Expand Down Expand Up @@ -380,7 +380,7 @@ export class TargetManager
this.#setupAttachmentListeners(session);

if (isExistingTarget) {
(session as CdpCDPSession)._setTarget(target);
(session as CdpCDPSession).setTarget(target);
this.#attachedTargetsBySessionId.set(
session.id(),
this.#attachedTargetsByTargetId.get(targetInfo.targetId)!,
Expand All @@ -393,7 +393,7 @@ export class TargetManager

const parentTarget =
parentSession instanceof CDPSession
? (parentSession as CdpCDPSession)._target()
? (parentSession as CdpCDPSession).target()
: null;
parentTarget?._addChildTarget(target);

Expand Down Expand Up @@ -440,7 +440,7 @@ export class TargetManager
}

if (parentSession instanceof CDPSession) {
(parentSession as CdpCDPSession)._target()._removeChildTarget(target);
(parentSession as CdpCDPSession).target()._removeChildTarget(target);
}
this.#attachedTargetsByTargetId.delete(target._targetId);
this.emit(TargetManagerEvent.TargetGone, target);
Expand Down
2 changes: 1 addition & 1 deletion packages/puppeteer-core/src/cdp/cdp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export * from './Binding.js';
export * from './Browser.js';
export * from './BrowserContext.js';
export * from './BrowserConnector.js';
export * from './CDPSession.js';
export * from './CdpSession.js';
export * from './Connection.js';
export * from './Coverage.js';
export * from './CdpPreloadScript.js';
Expand Down
7 changes: 6 additions & 1 deletion packages/puppeteer/install.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,12 @@
async function importInstaller() {
try {
return await import('puppeteer/internal/node/install.js');
} catch {
} catch (error) {
// Add more logging when the GitHub Action Debugging option is set
// https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables
if (process.env['RUNNER_DEBUG']) {
console.error(error);
}
console.warn(
'Skipping browser installation because the Puppeteer build is not available. Run `npm install` again after you have re-built Puppeteer.',
);
Expand Down
9 changes: 6 additions & 3 deletions packages/puppeteer/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,18 @@
"build": {
"dependencies": [
"build:tsc",
"build:types"
"build:types",
"generate:package-json"
]
},
"generate:package-json": {
"command": "node --experimental-strip-types ../../tools/generate_module_package_json.ts lib/esm/package.json",
"files": [
"../../tools/generate_module_package_json.ts"
],
"dependencies": [
"build:tsc"
],
"output": [
"lib/esm/package.json"
]
Expand All @@ -82,8 +86,7 @@
"clean": "if-file-deleted",
"dependencies": [
"../puppeteer-core:build",
"../browsers:build",
"generate:package-json"
"../browsers:build"
],
"files": [
"src/**"
Expand Down
Loading

0 comments on commit 6b20ac1

Please sign in to comment.