core(css-usage): prevent late stylesheet additions (#15865)

core/gather/gatherers/css-usage.js

@@ -36,7 +36,7 @@ class CSSUsage extends BaseGatherer {
   /**
    * @param {LH.Crdp.CSS.StyleSheetAddedEvent} event
    */
-  async _onStylesheetAdded(event) {
+  _onStylesheetAdded(event) {
     if (!this._session) throw new Error('Session not initialized');
     const styleSheetId = event.header.styleSheetId;
     const sheetPromise = this._session.sendCommand('CSS.getStyleSheetText', {styleSheetId})
@@ -66,59 +66,60 @@ class CSSUsage extends BaseGatherer {
   /**
    * @param {LH.Gatherer.Context} context
    */
-  async startCSSUsageTracking(context) {
+  async startInstrumentation(context) {
     const session = context.driver.defaultSession;
     this._session = session;
-    session.on('CSS.styleSheetAdded', this._onStylesheetAdded);
+
+    // Calling `CSS.enable` will emit events for stylesheets currently on the page.
+    // We want to ignore these events in navigation mode because they are not relevant to the
+    // navigation that is about to happen. Adding the event listener *after* calling `CSS.enable`
+    // ensures that the events for pre-existing stylesheets are ignored.
+    const isNavigation = context.gatherMode === 'navigation';
+    if (!isNavigation) {
+      session.on('CSS.styleSheetAdded', this._onStylesheetAdded);
+    }
 
     await session.sendCommand('DOM.enable');
     await session.sendCommand('CSS.enable');
     await session.sendCommand('CSS.startRuleUsageTracking');
+
+    if (isNavigation) {
+      session.on('CSS.styleSheetAdded', this._onStylesheetAdded);
+    }
   }
 
 
   /**
    * @param {LH.Gatherer.Context} context
    */
-  async stopCSSUsageTracking(context) {
+  async stopInstrumentation(context) {
     const session = context.driver.defaultSession;
     const coverageResponse = await session.sendCommand('CSS.stopRuleUsageTracking');
     this._ruleUsage = coverageResponse.ruleUsage;
     session.off('CSS.styleSheetAdded', this._onStylesheetAdded);
-  }
 
-  /**
-   * @param {LH.Gatherer.Context} context
-   */
-  async startInstrumentation(context) {
-    if (context.gatherMode !== 'timespan') return;
-    await this.startCSSUsageTracking(context);
-  }
+    // Ensure we finish fetching all stylesheet contents before disabling the CSS domain
+    await Promise.all(this._sheetPromises.values());
 
-  /**
-   * @param {LH.Gatherer.Context} context
-   */
-  async stopInstrumentation(context) {
-    if (context.gatherMode !== 'timespan') return;
-    await this.stopCSSUsageTracking(context);
+    await session.sendCommand('CSS.disable');
+    await session.sendCommand('DOM.disable');
   }
 
   /**
    * @param {LH.Gatherer.Context} context
    * @return {Promise<LH.Artifacts['CSSUsage']>}
    */
   async getArtifact(context) {
-    const session = context.driver.defaultSession;
     const executionContext = context.driver.executionContext;
 
-    if (context.gatherMode !== 'timespan') {
-      await this.startCSSUsageTracking(context);
+    if (context.gatherMode === 'snapshot') {
+      await this.startInstrumentation(context);
 
       // Force style to recompute.
       // Doesn't appear to be necessary in newer versions of Chrome.
       await executionContext.evaluateAsync('getComputedStyle(document.body)');
 
-      await this.stopCSSUsageTracking(context);
+      await this.stopInstrumentation(context);
     }
 
     /** @type {Map<string, LH.Artifacts.CSSStyleSheetInfo>} */
@@ -140,9 +141,6 @@ class CSSUsage extends BaseGatherer {
       dedupedStylesheets.set(sheet.content, sheet);
     }
 
-    await session.sendCommand('CSS.disable');
-    await session.sendCommand('DOM.disable');
-
     if (!this._ruleUsage) throw new Error('Issue collecting rule usages');
 
     return {

core/gather/gatherers/root-causes.js

@@ -124,9 +124,7 @@ class RootCauses extends BaseGatherer {
       rootCauses.layoutShifts[layoutShiftEvents.indexOf(event)] = r;
     }
 
-    // Yeah this gatherer enabled it, and so you'd think it should disable it too...
-    // ...but we don't give gatherers their own session so this stomps on others.
-    // await driver.defaultSession.sendCommand('DOM.disable');
+    await driver.defaultSession.sendCommand('DOM.disable');
     await driver.defaultSession.sendCommand('CSS.disable');
 
     return rootCauses;

core/test/fixtures/user-flows/css-change/end.html

@@ -9,5 +9,14 @@
 <body>
   <style>h1 {color: red}</style>
   <h1>Should have different styles than the start page</h1>
+  <button>Add more styles</button>
+  <script>
+    const button = document.querySelector('button');
+    button.onclick = () => {
+      const style = document.createElement('style');
+      style.innerHTML = 'h1 {font-size: 30px}';
+      document.body.append(style);
+    }
+  </script>
 </body>
 </html>

core/test/scenarios/stylesheet-tracking.js

@@ -0,0 +1,77 @@
+/**
+ * @license
+ * Copyright 2024 Google LLC
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import * as api from '../../index.js';
+import {createTestState} from './pptr-test-utils.js';
+import {LH_ROOT} from '../../../shared/root.js';
+
+/* eslint-env browser */
+
+describe('User flow stylesheet tracking', function() {
+  // eslint-disable-next-line no-invalid-this
+  this.timeout(120_000);
+
+  const state = createTestState();
+
+  state.installSetupAndTeardownHooks();
+
+  before(() => {
+    state.server.baseDir = `${LH_ROOT}/core/test/fixtures/user-flows/css-change`;
+  });
+
+  it('should correctly scope stylesheets based on mode', async () => {
+    const pageUrl = `${state.serverBaseUrl}/start.html`;
+    await state.page.goto(pageUrl, {waitUntil: ['networkidle0']});
+
+    const flow = await api.startFlow(state.page);
+
+    await flow.navigate(`${state.serverBaseUrl}/start.html`);
+
+    await flow.navigate(async () => {
+      await state.page.click('a');
+    });
+
+    await flow.startTimespan();
+    await state.page.click('button');
+    await flow.endTimespan();
+
+    const flowArtifacts = flow.createArtifactsJson();
+
+    const artifacts0 = flowArtifacts.gatherSteps[0].artifacts;
+    const artifacts1 = flowArtifacts.gatherSteps[1].artifacts;
+    const artifacts2 = flowArtifacts.gatherSteps[2].artifacts;
+
+    state.saveTrace(artifacts1.Trace);
+
+    const stylesheets0 = artifacts0.CSSUsage.stylesheets
+      .map(s => s.content.trim())
+      .sort((a, b) => a.localeCompare(b));
+    const stylesheets1 = artifacts1.CSSUsage.stylesheets
+      .map(s => s.content.trim())
+      .sort((a, b) => a.localeCompare(b));
+    const stylesheets2 = artifacts2.CSSUsage.stylesheets
+      .map(s => s.content.trim())
+      .sort((a, b) => a.localeCompare(b));
+
+    expect(stylesheets0).toEqual([
+      'body {\n  border: 5px solid black;\n}',
+      'h1 {color: gray}',
+    ]);
+
+    // Some stylesheets are pre-existing but they are out of scope for navigation mode
+    expect(stylesheets1).toEqual([
+      'body {\n  border: 5px solid red;\n}',
+      'h1 {color: red}',
+    ]);
+
+    // Some stylesheets are pre-existing and they are in in scope for timespan mode
+    expect(stylesheets2).toEqual([
+      'body {\n  border: 5px solid red;\n}',
+      'h1 {color: red}',
+      'h1 {font-size: 30px}',
+    ]);
+  });
+});