Repository: GoogleChrome/lighthouse
Pull request #12830 (2021-07-27 20:39:33)

core(fr): add snapshot and timespan support to no-unload-listeners audit
`JsUsage` was missing some source URL -> script id mappings in timespan mode, so this PR fills in the gaps with empty usages.

I'm concerned that empty usages won't be interpreted the same way as no usages, in which case a new gatherer specifically for source URL -> script id mappings should be considered.


Patch:
diff --git a/lighthouse-core/gather/gatherers/global-listeners.js b/lighthouse-core/gather/gatherers/global-listeners.js
index 41db2cc10..e14fe5446 100644
--- a/lighthouse-core/gather/gatherers/global-listeners.js
+++ b/lighthouse-core/gather/gatherers/global-listeners.js
@@ -17,7 +17,7 @@ const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
 class GlobalListeners extends FRGatherer {
   /** @type {LH.Gatherer.GathererMeta} */
   meta = {
-    supportedModes: ['snapshot', 'navigation'],
+    supportedModes: ['snapshot', 'timespan', 'navigation'],
   }
 
   /**
diff --git a/lighthouse-core/gather/gatherers/js-usage.js b/lighthouse-core/gather/gatherers/js-usage.js
index 62b3f7359..368870dfa 100644
--- a/lighthouse-core/gather/gatherers/js-usage.js
+++ b/lighthouse-core/gather/gatherers/js-usage.js
@@ -13,8 +13,7 @@ const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');
 class JsUsage extends FRGatherer {
   /** @type {LH.Gatherer.GathererMeta} */
   meta = {
-    // TODO(FR-COMPAT): special snapshot case for scriptId -> URL mappings.
-    supportedModes: ['timespan', 'navigation'],
+    supportedModes: ['snapshot', 'timespan', 'navigation'],
   }
 
   constructor() {
@@ -60,8 +59,8 @@ class JsUsage extends FRGatherer {
    */
   async startSensitiveInstrumentation(context) {
     const session = context.driver.defaultSession;
+    session.on('Debugger.scriptParsed', this.onScriptParsed);
     await session.sendCommand('Debugger.enable');
-    await session.on('Debugger.scriptParsed', this.onScriptParsed);
   }
 
   /**
@@ -69,16 +68,47 @@ class JsUsage extends FRGatherer {
    */
   async stopSensitiveInstrumentation(context) {
     const session = context.driver.defaultSession;
-    await session.off('Debugger.scriptParsed', this.onScriptParsed);
     await session.sendCommand('Debugger.disable');
+    session.off('Debugger.scriptParsed', this.onScriptParsed);
   }
 
   /**
+   * Usages alone do not always generate an exhaustive list of scripts in timespan and snapshot.
+   * For audits which use this for url/scriptId mappings, we can include an empty usage object.
+   *
+   * @param {Record<string, Array<LH.Crdp.Profiler.ScriptCoverage>>} usageByUrl
+   */
+  _addMissingScriptIds(usageByUrl) {
+    for (const scriptParsedEvent of this._scriptParsedEvents) {
+      const url = scriptParsedEvent.embedderName;
+      if (!url) continue;
+
+      const scripts = usageByUrl[url] || [];
+      if (!scripts.find(s => s.scriptId === scriptParsedEvent.scriptId)) {
+        scripts.push({
+          url,
+          scriptId: scriptParsedEvent.scriptId,
+          functions: [],
+        });
+      }
+      usageByUrl[url] = scripts;
+    }
+  }
+
+  /**
+   * @param {LH.Gatherer.FRTransitionalContext} context
    * @return {Promise<LH.Artifacts['JsUsage']>}
    */
-  async getArtifact() {
+  async getArtifact(context) {
     /** @type {Record<string, Array<LH.Crdp.Profiler.ScriptCoverage>>} */
     const usageByUrl = {};
+
+    // Force `Debugger.scriptParsed` events for url to scriptId mappings in snapshot mode.
+    if (context.gatherMode === 'snapshot') {
+      await this.startSensitiveInstrumentation(context);
+      await this.stopSensitiveInstrumentation(context);
+    }
+
     for (const scriptUsage of this._scriptUsages) {
       // `ScriptCoverage.url` can be overridden by a magic sourceURL comment.
       // Get the associated ScriptParsedEvent and use embedderName, which is the original url.
@@ -104,6 +134,11 @@ class JsUsage extends FRGatherer {
       usageByUrl[url] = scripts;
     }
 
+    // TODO(FR-COMPAT): Enable this logic for legacy and navigation or make a separate artifact for url/scriptId mappings.
+    if (context.gatherMode !== 'navigation') {
+      this._addMissingScriptIds(usageByUrl);
+    }
+
     return usageByUrl;
   }
 }


Test patch:
diff --git a/lighthouse-core/test/fraggle-rock/api-test-pptr.js b/lighthouse-core/test/fraggle-rock/api-test-pptr.js
index 6114a8717..55c68b311 100644
--- a/lighthouse-core/test/fraggle-rock/api-test-pptr.js
+++ b/lighthouse-core/test/fraggle-rock/api-test-pptr.js
@@ -99,7 +99,7 @@ describe('Fraggle Rock API', () => {
 
       const {auditResults, erroredAudits, failedAudits} = getAuditsBreakdown(lhr);
       // TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached.
-      expect(auditResults.length).toMatchInlineSnapshot(`79`);
+      expect(auditResults.length).toMatchInlineSnapshot(`80`);
 
       expect(erroredAudits).toHaveLength(0);
       expect(failedAudits.map(audit => audit.id)).toContain('label');
@@ -130,7 +130,7 @@ describe('Fraggle Rock API', () => {
         notApplicableAudits,
       } = getAuditsBreakdown(lhr);
       // TODO(FR-COMPAT): This assertion can be removed when full compatibility is reached.
-      expect(auditResults.length).toMatchInlineSnapshot(`49`);
+      expect(auditResults.length).toMatchInlineSnapshot(`50`);
 
       expect(notApplicableAudits.length).toMatchInlineSnapshot(`7`);
       expect(notApplicableAudits.map(audit => audit.id)).not.toContain('server-response-time');
@@ -172,7 +172,7 @@ describe('Fraggle Rock API', () => {
       if (!result) throw new Error('Lighthouse failed to produce a result');
 
       const {auditResults, erroredAudits, notApplicableAudits} = getAuditsBreakdown(result.lhr);
-      expect(auditResults.length).toMatchInlineSnapshot(`49`);
+      expect(auditResults.length).toMatchInlineSnapshot(`50`);
 
       expect(notApplicableAudits.length).toMatchInlineSnapshot(`9`);
       expect(notApplicableAudits.map(audit => audit.id)).toContain('server-response-time');
diff --git a/lighthouse-core/test/gather/gatherers/js-usage-test.js b/lighthouse-core/test/gather/gatherers/js-usage-test.js
index c2a7af6d5..e8f598073 100644
--- a/lighthouse-core/test/gather/gatherers/js-usage-test.js
+++ b/lighthouse-core/test/gather/gatherers/js-usage-test.js
@@ -13,6 +13,7 @@ const Driver = require('../../../gather/driver.js');
 const Connection = require('../../../gather/connections/connection.js');
 const JsUsage = require('../../../gather/gatherers/js-usage.js');
 const {createMockSendCommandFn, createMockOnFn} = require('../mock-commands.js');
+const {createMockContext} = require('../../fraggle-rock/gather/mock-driver.js');
 const {flushAllTimersAndMicrotasks} = require('../../test-utils.js');
 
 describe('JsUsage gatherer', () => {
@@ -59,7 +60,7 @@ describe('JsUsage gatherer', () => {
 
     expect(gatherer._scriptUsages).toEqual(coverage);
 
-    return gatherer.getArtifact();
+    return gatherer.getArtifact({gatherMode: 'navigation'});
   }
 
   it('combines coverage data by url', async () => {
@@ -105,4 +106,87 @@ describe('JsUsage gatherer', () => {
       }
     `);
   });
+
+  it('just establishes url to script id mappings in snapshot mode', async () => {
+    const context = createMockContext();
+    context.gatherMode = 'snapshot';
+    context.driver._session.on
+      .mockEvent('Debugger.scriptParsed', {
+        scriptId: '1',
+        embedderName: 'https://www.example.com',
+      });
+    context.driver._session.sendCommand
+      // Events are flushed on domain enable.
+      .mockResponse('Debugger.enable', flushAllTimersAndMicrotasks)
+      .mockResponse('Debugger.disable', {});
+
+    const artifact = await new JsUsage().getArtifact(context.asContext());
+
+    expect(artifact).toEqual({
+      'https://www.example.com': [
+        {
+          scriptId: '1',
+          url: 'https://www.example.com',
+          functions: [],
+        },
+      ],
+    });
+  });
+
+  it('adds script coverages without coverage in timespan', async () => {
+    const context = createMockContext();
+    context.gatherMode = 'timespan';
+    context.driver._session.on
+      .mockEvent('Debugger.scriptParsed', {
+        scriptId: '1',
+        embedderName: 'https://www.example.com',
+      })
+      .mockEvent('Debugger.scriptParsed', {
+        scriptId: '2',
+        embedderName: 'https://www.example.com/script.js',
+      });
+    context.driver._session.sendCommand
+      .mockResponse('Profiler.enable', {})
+      .mockResponse('Profiler.disable', {})
+      .mockResponse('Debugger.enable', {})
+      .mockResponse('Debugger.disable', {})
+      .mockResponse('Profiler.startPreciseCoverage', {})
+      .mockResponse('Profiler.takePreciseCoverage', {
+        result: [{
+          scriptId: '1',
+          url: 'https://www.example.com',
+          functions: [],
+        }],
+      })
+      .mockResponse('Profiler.stopPreciseCoverage', {});
+
+    const gatherer = new JsUsage();
+    await gatherer.startInstrumentation(context);
+    await gatherer.startSensitiveInstrumentation(context);
+
+    // Needed for protocol events to emit.
+    await flushAllTimersAndMicrotasks(1);
+
+    await gatherer.stopSensitiveInstrumentation(context);
+    await gatherer.stopInstrumentation(context);
+
+    const artifact = await gatherer.getArtifact(context);
+
+    expect(artifact).toEqual({
+      'https://www.example.com': [
+        {
+          scriptId: '1',
+          url: 'https://www.example.com',
+          functions: [],
+        },
+      ],
+      'https://www.example.com/script.js': [
+        {
+          scriptId: '2',
+          url: 'https://www.example.com/script.js',
+          functions: [],
+        },
+      ],
+    });
+  });
 });