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(gather): handle crash if CDP target crashes #11840

Merged
merged 49 commits into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
1655cd7
wip
paulirish Dec 16, 2020
5c27dbd
crash and detach
paulirish Dec 16, 2020
60b160a
just crash. simplify
paulirish Dec 16, 2020
fbf2ed1
add target.detached case
paulirish Dec 16, 2020
e7abf97
add a test.
paulirish Dec 16, 2020
6e257c8
lint / json
paulirish Dec 16, 2020
c53262e
avoid ctrl-c sigint => chromelauncher.kill() also triggering these re…
paulirish Dec 17, 2020
9bce036
Merge remote-tracking branch 'origin/master' into targetcrashed
paulirish Nov 3, 2021
9e95a65
move to driver. probably doesnt work - didnt test anything
paulirish Nov 3, 2021
de1fd10
Merge remote-tracking branch 'origin/master' into targetcrashed
connorjclark Jun 29, 2022
a2fdb48
Merge branch 'master' into targetcrashed
paulirish Aug 22, 2022
16fd334
Merge remote-tracking branch 'origin/targetcrashed' into target-crash…
paulirish Feb 21, 2024
fd59236
something. but.. 1) prot_timeout doesnt exit quickly, nor does this. …
paulirish Feb 21, 2024
50597de
rejection working as intended
paulirish Feb 22, 2024
4eaf8a9
Merge remote-tracking branch 'origin/main' into target-crashed-with-11.5
paulirish Feb 22, 2024
4603756
drop extra stuff
paulirish Feb 22, 2024
6798ff4
all to race
paulirish Feb 22, 2024
f22e334
test on the way
paulirish Feb 22, 2024
e0f751c
test horrorshow
paulirish Feb 22, 2024
db454e1
Revert "test horrorshow"
paulirish Feb 22, 2024
970cb32
types and test cleanup
paulirish Feb 22, 2024
f442a80
WIP changes from a while ago. Last modified on:
paulirish Feb 25, 2024
c34bcd8
moved from session to driver. definitely better.
paulirish Feb 25, 2024
e972872
tempthing
paulirish Feb 26, 2024
b41feb9
innerGather, but the rejection is unhandled
paulirish Feb 26, 2024
039a215
Revert "innerGather, but the rejection is unhandled"
paulirish Feb 26, 2024
c556d6d
fix
paulirish Feb 26, 2024
b0ca79e
finish cleanup. the wrappedpromise in nav-runner was the key to every…
paulirish Feb 26, 2024
bc7d77c
cleanup after more testing with a crashy browser
paulirish Feb 26, 2024
a030f98
split page_hung tweaks for sep pr
paulirish Feb 26, 2024
6cb351f
Merge remote-tracking branch 'origin/main' into targetcrashed
paulirish Feb 26, 2024
56b0011
promise<never> and dropping the getRejectionCallback method that had …
paulirish Feb 27, 2024
8815fbf
tests
paulirish Feb 27, 2024
240fa7c
WIP changes from a while ago. Last modified on:
paulirish Mar 5, 2024
c3b1da2
Merge remote-tracking branch 'origin/main' into targetcrashed
paulirish Apr 2, 2024
31b7b64
stop trying for a unittest and do a smoketest instead. doesnt work ye…
paulirish Apr 3, 2024
694f67d
seems good. but missing some artifacty things
paulirish Apr 3, 2024
ce021f5
now its handled and we get an LHR.. but given that we should probably…
paulirish Apr 3, 2024
6d79adf
changed things up to handle the rejection in navigation gather stuff.…
paulirish Apr 3, 2024
e45f4df
sendCommandAndIgnore
paulirish Apr 3, 2024
c0db08f
cleanup now that it works great. and test. and yay
paulirish Apr 3, 2024
512ec69
driver driver roger
paulirish Apr 3, 2024
9fd788e
types in mockdriver
paulirish Apr 3, 2024
0fcfa4f
mocksendcommand
paulirish Apr 3, 2024
ef4bb7d
Update core/test/gather/mock-driver.js
paulirish Apr 3, 2024
bc6ca69
fix mock. an impl isn't needed
paulirish Apr 3, 2024
043bda5
skip crash smoketest for devtools
paulirish Apr 9, 2024
537718d
Merge remote-tracking branch 'origin/main' into targetcrashed
paulirish Apr 9, 2024
fdd2f69
Merge remote-tracking branch 'origin/main' into targetcrashed
paulirish Apr 16, 2024
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
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};
}
paulirish marked this conversation as resolved.
Show resolved Hide resolved

/** @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
Loading