Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
17 changes: 16 additions & 1 deletion lighthouse-core/gather/driver.js
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

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,10 +834,11 @@ 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;
Expand All @@ -843,6 +854,10 @@ class Driver {
this.setNextProtocolTimeout(30 * 1000);
this.sendCommand('Page.navigate', {url});

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

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

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

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

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

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
9 changes: 3 additions & 6 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,18 +68,15 @@ 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

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

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

url = constants.defaultPassConfig.blankPage
Copy link
Contributor

Choose a reason for hiding this comment

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

can this go back to being one line now :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep! 🎉

) {
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 +455,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