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 5 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
24 changes: 23 additions & 1 deletion core/gather/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,23 @@
this._handleProtocolEvent = this._handleProtocolEvent.bind(this);
// @ts-expect-error Puppeteer expects the handler params to be type `unknown`
this._cdpSession.on('*', this._handleProtocolEvent);

// Poor man's Promise.withResolvers()
/** @param {Error} _ */
let rej = _ => {};
const promise = /** @type {Promise<never>} */ (new Promise((_, theRej) => rej = theRej));
this._targetCrashedResolver = {promise, rej};
connorjclark marked this conversation as resolved.
Show resolved Hide resolved

// 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.
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();
this._targetCrashedResolver.rej(new LighthouseError(LighthouseError.errors.TARGET_CRASHED));

Check warning on line 62 in core/gather/session.js

View check run for this annotation

Codecov / codecov/patch

core/gather/session.js#L58-L62

Added lines #L58 - L62 were not covered by tests
});
}

id() {
Expand Down Expand Up @@ -114,7 +131,8 @@
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._targetCrashedResolver.promise]);

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

onCrashPromise() {
return this._targetCrashedResolver.promise;
}
}

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