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
Changes from 1 commit
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
Prev Previous commit
Next Next commit
test horrorshow
paulirish committed Feb 22, 2024
commit e0f751c1b5dca872ffa1d0584282785eea87c5ef
2 changes: 2 additions & 0 deletions core/gather/session.js
Original file line number Diff line number Diff line change
@@ -134,7 +134,9 @@ class ProtocolSession extends CrdpEventEmitter {
* @return {void}
*/
listenForCrashes(crashRej) {
console.log("gonna listen")
this.on('Inspector.targetCrashed', async _ => {
console.error('DLFIDSF ON NO ITS A CRASH')
log.error('Session', 'Inspector.targetCrashed', this._targetInfo);
// Manually detach so no more CDP traffic is attempted.
this.dispose();
1 change: 0 additions & 1 deletion core/gather/snapshot-runner.js
Original file line number Diff line number Diff line change
@@ -40,7 +40,6 @@ async function snapshotGather(page, options = {}) {
baseArtifacts.URL = {
finalDisplayedUrl: url,
};

const artifactDefinitions = resolvedConfig.artifacts || [];
const artifactState = getEmptyArtifactState();
await collectPhaseArtifacts({
57 changes: 57 additions & 0 deletions core/test/gather/snapshot-runner-test.js
Original file line number Diff line number Diff line change
@@ -12,7 +12,11 @@ import {
createMockGathererInstance,
mockDriverModule,
mockRunnerModule,
createMockCdpSession,
} from './mock-driver.js';
import {fnAny} from '../test-utils.js';
import {getRejectionCallback} from '../../gather/runner-helpers.js';
import {Driver} from '../../gather/driver.js';

const mockRunner = await mockRunnerModule();

@@ -143,4 +147,57 @@ describe('Snapshot Runner', () => {
},
});
});

it.only('includes a crash runtimeError when there\'s a crash during gathering', async () => {
const puppeteerSession = createMockCdpSession();
puppeteerSession.send
.mockResponse('Page.enable')
.mockResponse('Page.getFrameTree', {frameTree: {frame: {id: 'mainFrameId'}}})
.mockResponse('Runtime.enable')
.mockResponse('Page.disable')
.mockResponse('Runtime.disable')
.mockResponse('Target.getTargetInfo', {targetInfo: {type: 'page', targetId: 'page'}})
.mockResponse('Network.enable')
.mockResponse('Target.setAutoAttach')
.mockResponse('Runtime.runIfWaitingForDebugger');

const pageTarget = {createCDPSession: () => puppeteerSession};

// @ts-expect-error - Individual mock functions are applied as necessary.
page = {target: () => pageTarget, url: fnAny()};
const driver = new Driver(page);

const runP = snapshotGather(page, {config});
// await expect(runP).rejects.toThrow(/TARGET_CRASHED/);
// const wait = (ms = 100) => new Promise(resolve => setTimeout(resolve, ms));
// const p1 = wait(100);
// debugger;
driver.defaultSession.emit('Inspector.targetCrashed', {});
await runP;

const gatherResult2 = await snapshotGather(page, {config});
expect(mockDriver.connect).toHaveBeenCalled();
expect(mockRunner.gather).toHaveBeenCalled();
expect(mockRunner.audit).not.toHaveBeenCalled();
const artifacts = await mockRunner.gather.mock.calls[0][0]();

expect(artifacts).toMatchObject({A: 'Artifact A', B: 'Artifact B'});

console.log({runP});

// const {resolvedConfig} = await initializeConfig('navigation');

// setTimeout(() => {
// driverMock.defaultSession.emit('Inspector.targetCrashed');
// });
// debugger;
// const {lhr} = await runGatherAndAudit(createGatherFn('https://example.com/'),
// {resolvedConfig, driverMock, computedCache: new Map()});

// // And it bubbled up to the runtimeError.
// expect(lhr.runtimeError.code).toEqual(LighthouseError.errors.TARGET_CRASHED.code);
// expect(lhr.runtimeError.message).toMatch(/crashed/);

// await expect(runP).rejects.toThrow(/TARGET_CRASHED/);
});
});
3 changes: 2 additions & 1 deletion core/test/gather/timespan-runner-test.js
Original file line number Diff line number Diff line change
@@ -185,8 +185,9 @@ describe('Timespan Runner', () => {
});
});

it('should warn if a navigation was detected', async () => {
it.only('should warn if a navigation was detected', async () => {
mockDriver._session.on.mockEvent('Page.frameNavigated', {});
mockDriver._session.on.mockEvent('Inspector.targetCrashed', {});

const timespan = await startTimespanGather(page, {config});
await flushAllTimersAndMicrotasks();
82 changes: 29 additions & 53 deletions core/test/runner-test.js
Original file line number Diff line number Diff line change
@@ -872,41 +872,41 @@ describe('Runner', () => {
});
});

describe.only('lhr.runtimeError', () => {
it('includes a top-level runtimeError when a gatherer throws one', async () => {
const NO_FCP = LighthouseError.errors.NO_FCP;
class RuntimeErrorGatherer extends Gatherer {
meta = {
supportedModes: ['navigation'],
};
describe('lhr.runtimeError', () => {
const NO_FCP = LighthouseError.errors.NO_FCP;
class RuntimeErrorGatherer extends Gatherer {
meta = {
supportedModes: ['navigation'],
};

getArtifact() {
throw new LighthouseError(NO_FCP);
}
getArtifact() {
throw new LighthouseError(NO_FCP);
}
}

class WarningAudit extends Audit {
static get meta() {
return {
id: 'test-audit',
title: 'A test audit',
failureTitle: 'A test audit',
description: 'An audit for testing',
requiredArtifacts: ['RuntimeErrorGatherer'],
};
}
static audit() {
throw new Error('Should not get here');
}
class WarningAudit extends Audit {
static get meta() {
return {
id: 'test-audit',
title: 'A test audit',
failureTitle: 'A test audit',
description: 'An audit for testing',
requiredArtifacts: ['RuntimeErrorGatherer'],
};
}
static audit() {
throw new Error('Should not get here');
}
}

const config = {
artifacts: [
{id: 'RuntimeErrorGatherer', gatherer: RuntimeErrorGatherer},
],
audits: [WarningAudit],
};
const config = {
artifacts: [
{id: 'RuntimeErrorGatherer', gatherer: RuntimeErrorGatherer},
],
audits: [WarningAudit],
};

it('includes a top-level runtimeError when a gatherer throws one', async () => {
const {resolvedConfig} = await initializeConfig('navigation', config);
const {lhr} = await runGatherAndAudit(createGatherFn('https://example.com/'),
{resolvedConfig, driverMock, computedCache: new Map()});
@@ -919,30 +919,6 @@ describe('Runner', () => {
expect(lhr.runtimeError.code).toEqual(NO_FCP.code);
expect(lhr.runtimeError.message).toMatch(/did not paint any content.*\(NO_FCP\)/);
});


it.only('includes a crash runtimeError when there\'s a crash during gathering', async () => {
// We need to simulate an expected browser crash. Basic plan is to crash the page on the FIRST use of sendCommand.
// A little odd, but it works.
// driverMock.defaultSession.sendCommand
// .mockResponse('Page.navigate', {data: 'Hello ', eof: false, base64Encoded: false});


const {resolvedConfig} = await initializeConfig('navigation');

setTimeout(() => {
driverMock.defaultSession.emit('Inspector.targetCrashed');
});
debugger;
const {lhr} = await runGatherAndAudit(createGatherFn('https://example.com/'),
{resolvedConfig, driverMock, computedCache: new Map()});

// And it bubbled up to the runtimeError.
expect(lhr.runtimeError.code).toEqual(LighthouseError.errors.TARGET_CRASHED.code);
expect(lhr.runtimeError.message).toMatch(/crashed/);

// await expect(runP).rejects.toThrow(/TARGET_CRASHED/);
});
});

it('localized errors thrown in gather fn', async () => {
1 change: 1 addition & 0 deletions core/test/scripts/run-mocha-tests.js
Original file line number Diff line number Diff line change
@@ -283,6 +283,7 @@ async function runMocha(tests, mochaArgs, invocationNumber) {
rootHooks,
timeout: 20_000,
bail: mochaArgs.bail,
color: true,
grep: mochaArgs.grep,
forbidOnly: mochaArgs.forbidOnly,
// TODO: not working