Skip to content

Commit

Permalink
core(gather): handle crash if CDP target crashes (#11840)
Browse files Browse the repository at this point in the history
  • Loading branch information
paulirish authored Apr 17, 2024
1 parent 5ad54df commit c220a33
Show file tree
Hide file tree
Showing 18 changed files with 121 additions and 8 deletions.
2 changes: 2 additions & 0 deletions cli/test/smokehouse/config/exclusions.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ const exclusions = {
'metrics-tricky-tti', 'metrics-tricky-tti-late-fcp', 'screenshot',
// Disabled because of differences that need further investigation
'byte-efficiency', 'byte-gzip', 'perf-preload',
// Disabled because a renderer crash also breaks devtools frontend
'crash',
],
};

Expand Down
2 changes: 2 additions & 0 deletions cli/test/smokehouse/core-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import a11y from './test-definitions/a11y.js';
import byteEfficiency from './test-definitions/byte-efficiency.js';
import byteGzip from './test-definitions/byte-gzip.js';
import crash from './test-definitions/crash.js';
import cspAllowAll from './test-definitions/csp-allow-all.js';
import cspBlockAll from './test-definitions/csp-block-all.js';
import dbw from './test-definitions/dobetterweb.js';
Expand Down Expand Up @@ -65,6 +66,7 @@ const smokeTests = [
a11y,
byteEfficiency,
byteGzip,
crash,
cspAllowAll,
cspBlockAll,
dbw,
Expand Down
41 changes: 41 additions & 0 deletions cli/test/smokehouse/test-definitions/crash.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/**
* @license
* Copyright 2024 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

const config = {
extends: 'lighthouse:default',
settings: {
maxWaitForLoad: 5000,
onlyAudits: [
'first-contentful-paint',
],
},
};

/**
* @type {Smokehouse.ExpectedRunnerResult}
*/
const expectations = {
lhr: {
requestedUrl: 'chrome://crash',
finalDisplayedUrl: 'about:blank',
runtimeError: {code: 'TARGET_CRASHED'},
runWarnings: [
'Browser tab has unexpectedly crashed.',
],
audits: {
'first-contentful-paint': {
scoreDisplayMode: 'error',
errorMessage: 'Browser tab has unexpectedly crashed.',
},
},
},
};

export default {
id: 'crash',
expectations,
config,
};
26 changes: 26 additions & 0 deletions core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ 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 Down Expand Up @@ -47,6 +48,12 @@ 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 @@ -86,11 +93,30 @@ 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
5 changes: 4 additions & 1 deletion core/gather/driver/navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ async function gotoURL(driver, requestor, options) {
throw new Error('Cannot wait for FCP without waiting for page load');
}

const waitConditions = await Promise.all(waitConditionPromises);
const waitConditions = await Promise.race([
driver.fatalRejection.promise,
Promise.all(waitConditionPromises),
]);
const timedOut = waitConditions.some(condition => condition.timedOut);
const navigationUrls = await networkMonitor.getNavigationUrls();

Expand Down
2 changes: 1 addition & 1 deletion core/gather/driver/prepare.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ async function enableAsyncStacks(session) {
await enable();

return async () => {
await session.sendCommand('Debugger.disable');
await session.sendCommandAndIgnore('Debugger.disable');
session.off('Debugger.paused', onDebuggerPaused);
session.off('Page.frameNavigated', onFrameNavigated);
};
Expand Down
5 changes: 3 additions & 2 deletions core/gather/driver/target-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,9 @@ class TargetManager extends ProtocolEventEmitter {
cdpSession.off('sessionattached', this._onSessionAttached);
}

await this._rootCdpSession.send('Page.disable');
await this._rootCdpSession.send('Runtime.disable');
// Ignore failures on these in case the tab has crashed.
await this._rootCdpSession.send('Page.disable').catch(_ => {});
await this._rootCdpSession.send('Runtime.disable').catch(_ => {});

this._enabled = false;
this._targetIdToTargets = new Map();
Expand Down
4 changes: 3 additions & 1 deletion core/gather/navigation-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ async function _navigate(navigationContext) {
return {requestedUrl, mainDocumentUrl, navigationError: undefined};
} catch (err) {
if (!(err instanceof LighthouseError)) throw err;
if (err.code !== 'NO_FCP' && err.code !== 'PAGE_HUNG') throw err;
if (err.code !== 'NO_FCP' && err.code !== 'PAGE_HUNG' && err.code !== 'TARGET_CRASHED') {
throw err;
}
if (typeof requestor !== 'string') throw err;

// TODO: Make the urls optional here so we don't need to throw an error with a callback requestor.
Expand Down
3 changes: 2 additions & 1 deletion core/gather/session.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ class ProtocolSession extends CrdpEventEmitter {
/** @type {NodeJS.Timer|undefined} */
let timeout;
const timeoutPromise = new Promise((resolve, reject) => {
// Unexpected setTimeout invocation to preserve the error stack. https://github.com/GoogleChrome/lighthouse/issues/13332
// eslint-disable-next-line max-len
timeout = setTimeout(reject, timeoutMs, new LighthouseError(LighthouseError.errors.PROTOCOL_TIMEOUT, {
protocolMethod: method,
Expand Down Expand Up @@ -139,7 +140,7 @@ class ProtocolSession extends CrdpEventEmitter {
async dispose() {
// @ts-expect-error Puppeteer expects the handler params to be type `unknown`
this._cdpSession.off('*', this._handleProtocolEvent);
await this._cdpSession.detach();
await this._cdpSession.detach().catch(e => log.verbose('session', 'detach failed', e.message));
}
}

Expand Down
4 changes: 2 additions & 2 deletions core/lib/emulation.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ function enableNetworkThrottling(session, throttlingSettings) {
* @return {Promise<void>}
*/
function clearNetworkThrottling(session) {
return session.sendCommand('Network.emulateNetworkConditions', NO_THROTTLING_METRICS);
return session.sendCommandAndIgnore('Network.emulateNetworkConditions', NO_THROTTLING_METRICS);
}

/**
Expand All @@ -146,7 +146,7 @@ function enableCPUThrottling(session, throttlingSettings) {
* @return {Promise<void>}
*/
function clearCPUThrottling(session) {
return session.sendCommand('Emulation.setCPUThrottlingRate', NO_CPU_THROTTLE_METRICS);
return session.sendCommandAndIgnore('Emulation.setCPUThrottlingRate', NO_CPU_THROTTLE_METRICS);
}

export {
Expand Down
10 changes: 10 additions & 0 deletions core/lib/lh-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ const UIStrings = {
* @example {Largest Contentful Paint} featureName
* */
oldChromeDoesNotSupportFeature: 'This version of Chrome is too old to support \'{featureName}\'. Use a newer version to see full results.',

/** Error message explaining that the browser tab that Lighthouse is inspecting has crashed. */
targetCrashed: 'Browser tab has unexpectedly crashed.',
};

const str_ = i18n.createIcuMessageFn(import.meta.url, UIStrings);
Expand Down Expand Up @@ -423,6 +426,13 @@ const ERRORS = {
message: UIStrings.erroredRequiredArtifact,
},

/** The page has crashed and will no longer respond to 99% of CDP commmands. */
TARGET_CRASHED: {
code: 'TARGET_CRASHED',
message: UIStrings.targetCrashed,
lhrRuntimeError: true,
},

// Hey! When adding a new error type, update lighthouse-result.proto too.
// Only necessary for runtime errors, which come from artifacts or pageLoadErrors.
};
Expand Down
3 changes: 3 additions & 0 deletions core/lib/navigation-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,9 @@ function getPageLoadError(navigationError, context) {
let finalRecord;
if (mainRecord) {
finalRecord = NetworkAnalyzer.resolveRedirects(mainRecord);
} else {
// We have no network requests to process, use the navError
return navigationError;
}

if (finalRecord?.mimeType === XHTML_MIME_TYPE) {
Expand Down
9 changes: 9 additions & 0 deletions core/test/gather/mock-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,13 @@ 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 @@ -172,6 +179,8 @@ function createMockDriver() {
fetchResource: fnAny(),
},
networkMonitor: new NetworkMonitor(targetManager.asTargetManager()),
listenForCrashes: fnAny(),
fatalRejection: {promise, rej},

/** @return {Driver} */
asDriver() {
Expand Down
1 change: 1 addition & 0 deletions core/test/gather/session-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ describe('ProtocolSession', () => {
describe('.dispose', () => {
it('should detach from the session', async () => {
const detach = fnAny();
detach.mockResolvedValue(undefined);
class MockCdpSession extends EventEmitter {
constructor() {
super();
Expand Down
4 changes: 4 additions & 0 deletions proto/lighthouse-result.proto
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ enum LighthouseError {
NOT_HTML = 21;
// The trace did not contain a ResourceSendRequest event.
NO_RESOURCE_REQUEST = 22;
// Used when any Chrome interstitial error prevents page load.
CHROME_INTERSTITIAL_ERROR = 23;
// The page has crashed and will no longer respond to 99% of CDP commmands.
TARGET_CRASHED = 24;
}

// The overarching Lighthouse Response object (LHR)
Expand Down
3 changes: 3 additions & 0 deletions shared/localization/locales/en-US.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions shared/localization/locales/en-XL.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions types/gatherer.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ 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 c220a33

Please sign in to comment.