Skip to content

core(driver): wait for Page.frameNavigated for about:blank #6446

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.

Sign up for GitHub

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

Merged
paulirish merged 3 commits into from
Nov 2, 2018
Merged

core(driver): wait for Page.frameNavigated for about:blank #6446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aaaf9fb
core(driver): wait for Page.frameNavigated for about:blank
patrickhulce Nov 1, 2018
38797ae
comma dangle
patrickhulce Nov 1, 2018
2d09609
more cleanup and feedback
patrickhulce Nov 1, 2018
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
4 changes: 0 additions & 4 deletions lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,7 +1029,6 @@ Object {
},
"passes": Array [
Object {
"blankDuration": 300,
"blankPage": "about:blank",
"blockedUrlPatterns": Array [],
"cpuQuietThresholdMs": 1000,
Expand Down Expand Up @@ -1123,7 +1122,6 @@ Object {
"useThrottling": true,
},
Object {
"blankDuration": 300,
"blankPage": "about:blank",
"blockedUrlPatterns": Array [],
"cpuQuietThresholdMs": 0,
Expand All @@ -1145,7 +1143,6 @@ Object {
"useThrottling": false,
},
Object {
"blankDuration": 300,
"blankPage": "about:blank",
"blockedUrlPatterns": Array [
"*.css",
Expand Down Expand Up @@ -1290,7 +1287,6 @@ Object {
},
"passes": Array [
Object {
"blankDuration": 300,
"blankPage": "about:blank",
"blockedUrlPatterns": Array [],
"cpuQuietThresholdMs": 1000,
Expand Down
1 change: 0 additions & 1 deletion lighthouse-core/config/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ const defaultPassConfig = {
cpuQuietThresholdMs: 0,
blockedUrlPatterns: [],
blankPage: 'about:blank',
blankDuration: 300,
gatherers: [],
};

Expand Down
21 changes: 20 additions & 1 deletion lighthouse-core/gather/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,16 @@ class Driver {
});
}

/**
* Returns a promise that resolve when a frame has been navigated.
* Used for detecting that our about:blank reset has been completed.
*/
_waitForFrameNavigated() {
return new Promise(resolve => {
Copy link
Contributor

@brendankenny brendankenny Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at some point we should add a driver method that wraps once with a promise. Seems like a lot of our uses of it end up like this

this.once('Page.frameNavigated', resolve);
});
}

/**
* Returns a promise that resolves when the network has been idle (after DCL) for
* `networkQuietThresholdMs` ms and a method to cancel internal network listeners/timeout.
Expand Down Expand Up @@ -824,14 +834,19 @@ class Driver {
* possible workaround.
* Resolves on the url of the loaded page, taking into account any redirects.
* @param {string} url
* @param {{waitForLoad?: boolean, passContext?: LH.Gatherer.PassContext}} options
* @param {{waitForLoad?: boolean, waitForNavigated?: boolean, passContext?: LH.Gatherer.PassContext}} options
* @return {Promise<string>}
*/
async gotoURL(url, options = {}) {
const waitForNavigated = options.waitForNavigated || false;
const waitForLoad = options.waitForLoad || false;
const passContext = /** @type {Partial<LH.Gatherer.PassContext>} */ (options.passContext || {});
const disableJS = passContext.disableJavaScript || false;

if (waitForNavigated && waitForLoad) {
throw new Error('Cannot use both waitForNavigated and waitForLoad, pick just one');
Copy link
Contributor

@brendankenny brendankenny Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

}

await this._beginNetworkStatusMonitoring(url);
await this._clearIsolatedContextId();

Expand All @@ -843,6 +858,10 @@ class Driver {
this.setNextProtocolTimeout(30 * 1000);
this.sendCommand('Page.navigate', {url});

if (waitForNavigated) {
await this._waitForFrameNavigated();
Copy link
Contributor

@brendankenny brendankenny Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any concerns about racing here? i.e. should we get the promise before Page.navigate and then await it here?

Copy link
Contributor

@brendankenny brendankenny Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, any concerns about delaying _waitForFullyLoaded until after Page.frameNavigated fires?

Copy link
Collaborator Author

@patrickhulce patrickhulce Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dgozman convinced me that so long as the listener is attached synchronously (which it is inside this._waitForFrameNavigated), it is impossible for the event to be received too early.

Longer explanation was that the earliest possible resolution of the promise would be the next microtask and because the listener is installed before that microtask is executed there's no way to miss the event

Copy link
Collaborator Author

@patrickhulce patrickhulce Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, any concerns about delaying _waitForFullyLoaded until after Page.frameNavigated fires?

definitely, they should not be used together I'll add a throw

Copy link
Contributor

@brendankenny brendankenny Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dgozman convinced me that so long as the listener is attached synchronously (which it is inside this._waitForFrameNavigated), it is impossible for the event to be received too early.

yeahhhh, good point. As long as no one sticks an await in front of that sendCommand, I guess?

}

if (waitForLoad) {
const passConfig = /** @type {Partial<LH.Config.Pass>} */ (passContext.passConfig || {});
let {pauseAfterLoadMs, networkQuietThresholdMs, cpuQuietThresholdMs} = passConfig;
Expand Down
12 changes: 3 additions & 9 deletions lighthouse-core/gather/gather-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,18 +68,12 @@ class GatherRunner {
* never fired on it.
* @param {Driver} driver
* @param {string=} url
* @param {number=} duration
* @return {Promise<void>}
*/
static async loadBlank(
driver,
url = constants.defaultPassConfig.blankPage,
duration = constants.defaultPassConfig.blankDuration
Copy link
Contributor

@brendankenny brendankenny Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forget when we've actually used a different blankDuration. Was it just for tests?

Copy link
Collaborator Author

@patrickhulce patrickhulce Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests and once upon a time WPT used it for video recording, but for past year or so he's just run LH straight outta the box

https://github.com/WPO-Foundation/wptagent/blob/14d8c49e3139625fd31fa12efd6a3ab7632dd323/internal/devtools_browser.py#L466-L476

) {
static async loadBlank(driver, url = constants.defaultPassConfig.blankPage) {
const status = {msg: 'Resetting state with about:blank', id: 'lh:gather:loadBlank'};
log.time(status);
await driver.gotoURL(url);
await new Promise(resolve => setTimeout(resolve, duration));
await driver.gotoURL(url, {waitForNavigated: true});
log.timeEnd(status);
}

Expand Down Expand Up @@ -458,7 +452,7 @@ class GatherRunner {
await driver.setThrottling(options.settings, passConfig);
if (!isFirstPass) {
// Already on blank page if driver was just set up.
await GatherRunner.loadBlank(driver, passConfig.blankPage, passConfig.blankDuration);
await GatherRunner.loadBlank(driver, passConfig.blankPage);
}
await GatherRunner.beforePass(passContext, gathererResults);
await GatherRunner.pass(passContext, gathererResults);
Expand Down
20 changes: 7 additions & 13 deletions lighthouse-core/test/gather/gather-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,14 +560,12 @@ describe('GatherRunner', function() {
const settings = {};

const passes = [{
blankDuration: 0,
recordTrace: true,
passName: 'firstPass',
gatherers: [
{instance: t1},
],
}, {
blankDuration: 0,
passName: 'secondPass',
gatherers: [
{instance: t2},
Expand All @@ -587,12 +585,10 @@ describe('GatherRunner', function() {

it('respects trace names', () => {
const passes = [{
blankDuration: 0,
recordTrace: true,
passName: 'firstPass',
gatherers: [{instance: new TestGatherer()}],
}, {
blankDuration: 0,
recordTrace: true,
passName: 'secondPass',
gatherers: [{instance: new TestGatherer()}],
Expand All @@ -610,12 +606,10 @@ describe('GatherRunner', function() {

it('doesn\'t leave networkRecords as an artifact', () => {
const passes = [{
blankDuration: 0,
recordTrace: true,
passName: 'firstPass',
gatherers: [{instance: new TestGatherer()}],
}, {
blankDuration: 0,
recordTrace: true,
passName: 'secondPass',
gatherers: [{instance: new TestGatherer()}],
Expand Down Expand Up @@ -790,7 +784,7 @@ describe('GatherRunner', function() {
},
];
const passes = [{
blankDuration: 0,

gatherers: gatherers.map(G => ({instance: new G()})),
}];

Expand Down Expand Up @@ -844,7 +838,7 @@ describe('GatherRunner', function() {
].map(instance => ({instance}));
const gathererNames = gatherers.map(gatherer => gatherer.instance.name);
const passes = [{
blankDuration: 0,

gatherers,
}];

Expand Down Expand Up @@ -881,7 +875,7 @@ describe('GatherRunner', function() {
{instance: new class EavesdropGatherer3 extends EavesdropGatherer {}()},
];

const passes = [{blankDuration: 0, gatherers}];
const passes = [{gatherers}];
return GatherRunner.run(passes, {
driver: fakeDriver,
requestedUrl: 'https://example.com',
Expand Down Expand Up @@ -1002,7 +996,7 @@ describe('GatherRunner', function() {
].map(instance => ({instance}));
const gathererNames = gatherers.map(gatherer => gatherer.instance.name);
const passes = [{
blankDuration: 0,

gatherers,
}];

Expand All @@ -1022,7 +1016,7 @@ describe('GatherRunner', function() {

it('rejects if a gatherer does not provide an artifact', () => {
const passes = [{
blankDuration: 0,

recordTrace: true,
passName: 'firstPass',
gatherers: [
Expand All @@ -1040,7 +1034,7 @@ describe('GatherRunner', function() {

it('rejects when domain name can\'t be resolved', () => {
const passes = [{
blankDuration: 0,

recordTrace: true,
passName: 'firstPass',
gatherers: [],
Expand Down Expand Up @@ -1071,7 +1065,7 @@ describe('GatherRunner', function() {

it('resolves when domain name can\'t be resolved but is offline', () => {
const passes = [{
blankDuration: 0,

recordTrace: true,
passName: 'firstPass',
gatherers: [],
Expand Down
1 change: 0 additions & 1 deletion typings/config.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ declare global {
cpuQuietThresholdMs?: number;
blockedUrlPatterns?: string[];
blankPage?: string;
blankDuration?: number;
gatherers?: GathererJson[];
}

Expand Down