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(fps): make lhId less dependent on chrome internals #14272

Merged
merged 12 commits into from
Aug 16, 2022
21 changes: 19 additions & 2 deletions core/gather/driver/execution-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ class ExecutionContext {

/** @type {number|undefined} */
this._executionContextId = undefined;
/**
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
* Marks how many execution context ids have been created, for purposes of having a unique
* value (that doesn't expose the actual execution context id) to
* use for __lighthouseExecutionContextUniqueIdentifier.
* @type {number}
*/
this._executionContextIdentifiersCreated = 0;

// We use isolated execution contexts for `evaluateAsync` that can be destroyed through navigation
// and other page actions. Cleanup our relevant bookkeeping as we see those events.
Expand Down Expand Up @@ -64,10 +71,10 @@ class ExecutionContext {
});

this._executionContextId = isolatedWorldResponse.executionContextId;
this._executionContextIdentifiersCreated++;
return isolatedWorldResponse.executionContextId;
}


/**
* Evaluate an expression in the given execution context; an undefined contextId implies the main
* page without isolation.
Expand All @@ -82,15 +89,25 @@ class ExecutionContext {
this._session.getNextProtocolTimeout() :
60000;

// `__lighthouseExecutionContextUniqueIdentifier` is only used by the FullPageScreenshot gatherer.
// See `getNodeDetails` in page-functions.
const uniqueExecutionContextIdentifier = contextId === undefined ?
undefined :
this._executionContextIdentifiersCreated;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@brendankenny not sure if this is what you had in mind, but we need to be mindful of the "undefined"/"page" context here.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, more or less, though I actually think the previous undefined (instead of 0) here is kind of cleaner--there is no isolated context for that case, it makes it clearer that it's separate from the numbering of the contexts, and it eliminates the need for the '?' case below.

const evaluationParams = {
// We need to explicitly wrap the raw expression for several purposes:
// 1. Ensure that the expression will be a native Promise and not a polyfill/non-Promise.
// 2. Ensure that errors in the expression are captured by the Promise.
// 3. Ensure that errors captured in the Promise are converted into plain-old JS Objects
// so that they can be serialized properly b/c JSON.stringify(new Error('foo')) === '{}'
//
// `__lighthouseExecutionContextUniqueIdentifier` is only used by the FullPageScreenshot gatherer.
// See `getNodeDetails` in page-functions.
expression: `(function wrapInNativePromise() {
${ExecutionContext._cachedNativesPreamble};
globalThis.__lighthouseExecutionContextId = ${contextId};
globalThis.__lighthouseExecutionContextUniqueIdentifier =
${uniqueExecutionContextIdentifier};
return new Promise(function (resolve) {
return Promise.resolve()
.then(_ => ${expression})
Expand Down
18 changes: 15 additions & 3 deletions core/lib/page-functions.js
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,18 @@ function getNodeDetails(element) {
const selector = getNodeSelector(element);

// Create an id that will be unique across all execution contexts.
//
// Made up of 3 components:
// - prefix unique to specific execution context
// - nth unique node seen by this function for this execution context
// - node tagName
//
// Every page load only has up to two associated contexts - the page context
// (denoted as `__lighthouseExecutionContextUniqueIdentifier` being undefined)
// and the isolated context. The id must be unique to distinguish gatherers running
// on different page loads that identify the same logical element, for purposes
// of the full page screenshot node lookup; hence the prefix.
//
// The id could be any arbitrary string, the exact value is not important.
// For example, tagName is added only because it might be useful for debugging.
// But execution id and map size are added to ensure uniqueness.
Expand All @@ -507,9 +519,9 @@ function getNodeDetails(element) {
let lhId = window.__lighthouseNodesDontTouchOrAllVarianceGoesAway.get(element);
if (!lhId) {
lhId = [
window.__lighthouseExecutionContextId !== undefined ?
window.__lighthouseExecutionContextId :
'page',
window.__lighthouseExecutionContextUniqueIdentifier === undefined ?
'page' :
window.__lighthouseExecutionContextUniqueIdentifier,
window.__lighthouseNodesDontTouchOrAllVarianceGoesAway.size,
element.tagName,
].join('-');
Expand Down
3 changes: 2 additions & 1 deletion core/test/gather/driver/execution-context-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,8 @@ describe('.evaluate', () => {
const URL = globalThis.__nativeURL || globalThis.URL;
const performance = globalThis.__nativePerformance || globalThis.performance;
const fetch = globalThis.__nativeFetch || globalThis.fetch;
globalThis.__lighthouseExecutionContextId = undefined;
globalThis.__lighthouseExecutionContextUniqueIdentifier =
undefined;
return new Promise(function (resolve) {
return Promise.resolve()
.then(_ => (() => {
Expand Down
2 changes: 1 addition & 1 deletion types/externs.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ declare global {

/** Used by FullPageScreenshot gatherer. */
__lighthouseNodesDontTouchOrAllVarianceGoesAway: Map<Element, string>;
__lighthouseExecutionContextId?: number;
__lighthouseExecutionContextUniqueIdentifier?: number;

/** Injected into the page when the `--debug` flag is used. */
continueLighthouseRun(): void;
Expand Down