Skip to content

Commit

Permalink
core: handle target crash at any point (#15985)
Browse files Browse the repository at this point in the history
  • Loading branch information
connorjclark authored May 14, 2024
1 parent f4331fb commit 9c33bf5
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 39 deletions.
27 changes: 1 addition & 26 deletions core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import log from 'lighthouse-logger';

import {ExecutionContext} from './driver/execution-context.js';
import {TargetManager} from './driver/target-manager.js';
import {LighthouseError} from '../lib/lh-error.js';
import {Fetcher} from './fetcher.js';
import {NetworkMonitor} from './driver/network-monitor.js';

Expand All @@ -29,6 +28,7 @@ const throwingSession = {
sendCommand: throwNotConnectedFn,
sendCommandAndIgnore: throwNotConnectedFn,
dispose: throwNotConnectedFn,
onCrashPromise: throwNotConnectedFn,
};

/** @implements {LH.Gatherer.Driver} */
Expand All @@ -48,12 +48,6 @@ class Driver {
this._fetcher = undefined;

this.defaultSession = throwingSession;

// Poor man's Promise.withResolvers()
/** @param {Error} _ */
let rej = _ => {};
const promise = /** @type {Promise<never>} */ (new Promise((_, theRej) => rej = theRej));
this.fatalRejection = {promise, rej};
}

/** @return {LH.Gatherer.Driver['executionContext']} */
Expand Down Expand Up @@ -93,30 +87,11 @@ class Driver {
this._networkMonitor = new NetworkMonitor(this._targetManager);
await this._networkMonitor.enable();
this.defaultSession = this._targetManager.rootSession();
this.listenForCrashes();
this._executionContext = new ExecutionContext(this.defaultSession);
this._fetcher = new Fetcher(this.defaultSession);
log.timeEnd(status);
}

/**
* If the target crashes, we can't continue gathering.
*
* FWIW, if the target unexpectedly detaches (eg the user closed the tab), pptr will
* catch that and reject into our this._cdpSession.send, which we'll alrady handle appropriately
* @return {void}
*/
listenForCrashes() {
this.defaultSession.on('Inspector.targetCrashed', async _ => {
log.error('Driver', 'Inspector.targetCrashed');
// Manually detach so no more CDP traffic is attempted.
// Don't await, else our rejection will be a 'Target closed' protocol error on cross-talk CDP calls.
void this.defaultSession.dispose();
this.fatalRejection.rej(new LighthouseError(LighthouseError.errors.TARGET_CRASHED));
});
}


/** @return {Promise<void>} */
async disconnect() {
if (this.defaultSession === throwingSession) return;
Expand Down
2 changes: 1 addition & 1 deletion core/gather/driver/navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ async function gotoURL(driver, requestor, options) {
}

const waitConditions = await Promise.race([
driver.fatalRejection.promise,
session.onCrashPromise(),
Promise.all(waitConditionPromises),
]);
const timedOut = waitConditions.some(condition => condition.timedOut);
Expand Down
23 changes: 22 additions & 1 deletion core/gather/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,22 @@ class ProtocolSession extends CrdpEventEmitter {
this._handleProtocolEvent = this._handleProtocolEvent.bind(this);
// @ts-expect-error Puppeteer expects the handler params to be type `unknown`
this._cdpSession.on('*', this._handleProtocolEvent);

// If the target crashes, we can't continue gathering.
// FWIW, if the target unexpectedly detaches (eg the user closed the tab), pptr will
// catch that and reject in this._cdpSession.send, which is caught by us.
/** @param {Error} _ */
let rej = _ => {}; // Poor man's Promise.withResolvers()
this._targetCrashedPromise = /** @type {Promise<never>} */ (
new Promise((_, theRej) => rej = theRej));
this.on('Inspector.targetCrashed', async () => {
log.error('TargetManager', 'Inspector.targetCrashed');
// Manually detach so no more CDP traffic is attempted.
// Don't await, else our rejection will be a 'Target closed' protocol error on cross-talk
// CDP calls.
void this.dispose();
rej(new LighthouseError(LighthouseError.errors.TARGET_CRASHED));
});
}

id() {
Expand Down Expand Up @@ -114,7 +130,8 @@ class ProtocolSession extends CrdpEventEmitter {
log.formatProtocol('method <= browser ERR', {method}, 'error');
throw LighthouseError.fromProtocolMessage(method, error);
});
const resultWithTimeoutPromise = Promise.race([resultPromise, timeoutPromise]);
const resultWithTimeoutPromise =
Promise.race([resultPromise, timeoutPromise, this._targetCrashedPromise]);

return resultWithTimeoutPromise.finally(() => {
if (timeout) clearTimeout(timeout);
Expand Down Expand Up @@ -142,6 +159,10 @@ class ProtocolSession extends CrdpEventEmitter {
this._cdpSession.off('*', this._handleProtocolEvent);
await this._cdpSession.detach().catch(e => log.verbose('session', 'detach failed', e.message));
}

onCrashPromise() {
return this._targetCrashedPromise;
}
}

export {ProtocolSession};
10 changes: 1 addition & 9 deletions core/test/gather/mock-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ function createMockSession() {
addProtocolMessageListener: createMockOnFn(),
removeProtocolMessageListener: fnAny(),
dispose: fnAny(),
onCrashPromise: fnAny().mockReturnValue(new Promise(() => {})),

/** @return {LH.Gatherer.ProtocolSession} */
asSession() {
Expand Down Expand Up @@ -158,13 +159,6 @@ function createMockDriver() {
const context = createMockExecutionContext();
const targetManager = createMockTargetManager(session);

// The `fatalRejection`
/** @param {Error} _ */
let rej = _ => {};
const promise = new Promise((_, theRej) => {
rej = theRej;
});

return {
_page: page,
_executionContext: context,
Expand All @@ -179,8 +173,6 @@ function createMockDriver() {
fetchResource: fnAny(),
},
networkMonitor: new NetworkMonitor(targetManager.asTargetManager()),
listenForCrashes: fnAny(),
fatalRejection: {promise, rej},

/** @return {Driver} */
asDriver() {
Expand Down
3 changes: 1 addition & 2 deletions types/gatherer.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ declare module Gatherer {
sendCommand<TMethod extends keyof CrdpCommands>(method: TMethod, ...params: CrdpCommands[TMethod]['paramsType']): Promise<CrdpCommands[TMethod]['returnType']>;
sendCommandAndIgnore<TMethod extends keyof CrdpCommands>(method: TMethod, ...params: CrdpCommands[TMethod]['paramsType']): Promise<void>;
dispose(): Promise<void>;
onCrashPromise(): Promise<never>;
}

interface Driver {
Expand All @@ -49,8 +50,6 @@ declare module Gatherer {
off(event: 'protocolevent', callback: (payload: Protocol.RawEventMessage) => void): void
};
networkMonitor: NetworkMonitor;
listenForCrashes: (() => void);
fatalRejection: {promise: Promise<never>, rej: (reason: Error) => void}
}

interface Context<TDependencies extends DependencyKey = DefaultDependenciesKey> {
Expand Down

0 comments on commit 9c33bf5

Please sign in to comment.