core(fps): make lhId less dependent on chrome internals (#14272)

connorjclark · web-flow · commit e9b8c47f00a7 · 2022-08-16T12:12:23.000-07:00
diff --git a/core/gather/driver/execution-context.js b/core/gather/driver/execution-context.js
@@ -16,6 +16,13 @@ class ExecutionContext {
 
     /** @type {number|undefined} */
     this._executionContextId = undefined;
+    /**
+     * 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.
@@ -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.
@@ -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;
+
     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})
diff --git a/core/lib/page-functions.js b/core/lib/page-functions.js
@@ -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.
@@ -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('-');
diff --git a/core/test/gather/driver/execution-context-test.js b/core/test/gather/driver/execution-context-test.js
@@ -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(_ => (() => {
diff --git a/types/externs.d.ts b/types/externs.d.ts
@@ -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;
