core(config): prevent custom gatherer interference (#14756)

core/config/config.js

@@ -37,6 +37,18 @@ const defaultConfigPath = path.join(
   '../../config/default-config.js'
 );
 
+/**
+ * Certain gatherers are destructive to the page state.
+ * We should ensure that these gatherers run after any custom gatherers.
+ * The default priority should be 0.
+ * TODO: Make this an official part of the config or design a different solution.
+ * @type {Record<string, number|undefined>}
+ */
+const internalArtifactPriorities = {
+  FullPageScreenshot: 1,
+  BFCacheFailures: 1,
+};
+
 /**
  * @param {LH.Config|undefined} config
  * @param {{configPath?: string}} context
@@ -129,12 +141,19 @@ async function resolveArtifactsToDefns(artifacts, configDir) {
   const status = {msg: 'Resolve artifact definitions', id: 'lh:config:resolveArtifactsToDefns'};
   log.time(status, 'verbose');
 
+  const sortedArtifacts = [...artifacts];
+  sortedArtifacts.sort((a, b) => {
+    const aPriority = internalArtifactPriorities[a.id] || 0;
+    const bPriority = internalArtifactPriorities[b.id] || 0;
+    return aPriority - bPriority;
+  });
+
   /** @type {Map<Symbol, LH.Config.AnyArtifactDefn>} */
   const artifactDefnsBySymbol = new Map();
 
   const coreGathererList = Runner.getGathererList();
   const artifactDefns = [];
-  for (const artifactJson of artifacts) {
+  for (const artifactJson of sortedArtifacts) {
     /** @type {LH.Config.GathererJson} */
     // @ts-expect-error - remove when legacy runner path is removed.
     const gathererJson = artifactJson.gatherer;

core/legacy/config/config.js

@@ -60,6 +60,18 @@ const alwaysRunArtifactIds = [
   'FullPageScreenshot',
 ];
 
+/**
+ * Certain gatherers are destructive to the page state.
+ * We should ensure that these gatherers run after any custom gatherers.
+ * The default priority should be 0.
+ * TODO: Make this an official part of the config or design a different solution.
+ * @type {Record<string, number|undefined>}
+ */
+const internalGathererPriorities = {
+  FullPageScreenshot: 1,
+  BFCacheFailures: 1,
+};
+
 /**
  * @param {LegacyResolvedConfig['passes']} passes
  * @param {LegacyResolvedConfig['audits']} audits
@@ -536,6 +548,12 @@ class LegacyResolvedConfig {
       );
       uniqueDefns.forEach(gatherer => assertValidGatherer(gatherer.instance, gatherer.path));
 
+      uniqueDefns.sort((a, b) => {
+        const aPriority = internalGathererPriorities[a.instance.name] || 0;
+        const bPriority = internalGathererPriorities[b.instance.name] || 0;
+        return aPriority - bPriority;
+      });
+
       return Object.assign(pass, {gatherers: uniqueDefns});
     });
     const fullPasses = await Promise.all(fullPassesPromises);

core/test/config/config-test.js

@@ -383,6 +383,20 @@ describe('Fraggle Rock Config', () => {
       if (!hasExtraArtifact) expect(resolvedConfig.artifacts).toContain('ExtraArtifact');
     });
 
+    it('should sort artifacts by internal priority', async () => {
+      const {resolvedConfig} = await initializeConfig('navigation', extensionConfig);
+      if (!resolvedConfig.artifacts) throw new Error(`No artifacts created`);
+
+      const last5 = resolvedConfig.artifacts.reverse().slice(0, 5).map(a => a.id);
+      expect(last5).toEqual([
+        'BFCacheFailures', // Has internal priority of 1
+        'FullPageScreenshot', // Has internal priority of 1
+        'ExtraArtifact', // Has default priority of 0
+        'traces', // Has default priority of 0
+        'devtoolsLogs', // Has default priority of 0
+      ]);
+    });
+
     it('should merge in navigations', async () => {
       const {resolvedConfig} = await initializeConfig('navigation', extensionConfig);
       if (!resolvedConfig.navigations) throw new Error(`No navigations created`);

core/test/legacy/config/config-test.js

@@ -1380,6 +1380,19 @@ describe('Config', () => {
         [{path: 'viewport-dimensions', instance: expectedInstance}]);
     });
 
+    it('should sort gatherers by internal priority', async () => {
+      const gatherers = [
+        'bf-cache-failures', // Has internal priority of 1
+        'viewport-dimensions', // Has default internal priority of 0
+      ];
+
+      const merged = await LegacyResolvedConfig.requireGatherers([{gatherers}]);
+
+      assert.deepEqual(
+        merged[0].gatherers.map(g => g.path),
+        ['viewport-dimensions', 'bf-cache-failures']);
+    });
+
     async function loadGatherer(gathererEntry) {
       const resolvedConfig =
         await LegacyResolvedConfig.fromJson({passes: [{gatherers: [gathererEntry]}]});