Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: handle target crash at any point #15985

Merged
merged 7 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading