core(uses-preload): prevent infinite loop (#5184)

lighthouse-core/audits/uses-rel-preload.js

@@ -62,18 +62,16 @@ class UsesRelPreloadAudit extends Audit {
    * @param {Set<string>} urls The array of byte savings results per resource
    * @param {LH.Gatherer.Simulation.GraphNode} graph
    * @param {LH.Gatherer.Simulation.Simulator} simulator
-   * @param {LH.WebInspector.NetworkRequest} mainResource
    * @return {{wastedMs: number, results: Array<{url: string, wastedMs: number}>}}
    */
-  static computeWasteWithGraph(urls, graph, simulator, mainResource) {
+  static computeWasteWithGraph(urls, graph, simulator) {
     if (!urls.size) {
       return {wastedMs: 0, results: []};
     }
 
     // Preload changes the ordering of requests, simulate the original graph with flexible ordering
     // to have a reasonable baseline for comparison.
     const simulationBeforeChanges = simulator.simulate(graph, {flexibleOrdering: true});
-
     const modifiedGraph = graph.cloneWithRelationships();
 
     /** @type {Array<LH.Gatherer.Simulation.GraphNetworkNode>} */
@@ -84,12 +82,10 @@ class UsesRelPreloadAudit extends Audit {
       if (node.type !== 'network') return;
 
       const networkNode = /** @type {LH.Gatherer.Simulation.GraphNetworkNode} */ (node);
-      if (networkNode.record && urls.has(networkNode.record.url)) {
-        nodesToPreload.push(networkNode);
-      }
-
-      if (networkNode.record && networkNode.record.url === mainResource.url) {
+      if (node.isMainDocument()) {
         mainDocumentNode = networkNode;
+      } else if (networkNode.record && urls.has(networkNode.record.url)) {
+        nodesToPreload.push(networkNode);
       }
     });
 
@@ -167,8 +163,7 @@ class UsesRelPreloadAudit extends Audit {
       }
     }
 
-    const {results, wastedMs} = UsesRelPreloadAudit.computeWasteWithGraph(urls, graph, simulator,
-        mainResource);
+    const {results, wastedMs} = UsesRelPreloadAudit.computeWasteWithGraph(urls, graph, simulator);
     // sort results by wastedTime DESC
     results.sort((a, b) => b.wastedMs - a.wastedMs);
 

lighthouse-core/lib/dependency-graph/network-node.js

@@ -68,7 +68,9 @@ class NetworkNode extends Node {
    * @return {NetworkNode}
    */
   cloneWithoutRelationships() {
-    return new NetworkNode(this._record);
+    const node = new NetworkNode(this._record);
+    node.setIsMainDocument(this._isMainDocument);
+    return node;
   }
 }
 

lighthouse-core/lib/dependency-graph/node.js

@@ -155,7 +155,9 @@ class Node {
    * @return {Node}
    */
   cloneWithoutRelationships() {
-    return new Node(this.id);
+    const node = new Node(this.id);
+    node.setIsMainDocument(this._isMainDocument);
+    return node;
   }
 
   /**
@@ -256,9 +258,15 @@ class Node {
   /**
    * Returns whether the given node has a cycle in its dependent graph by performing a DFS.
    * @param {Node} node
+   * @param {'dependents'|'dependencies'|'both'} [direction]
    * @return {boolean}
    */
-  static hasCycle(node) {
+  static hasCycle(node, direction = 'both') {
+    // Checking 'both' is the default entrypoint to recursively check both directions
+    if (direction === 'both') {
+      return Node.hasCycle(node, 'dependents') || Node.hasCycle(node, 'dependencies');
+    }
+
     const visited = new Set();
     /** @type {Node[]} */
     const currentPath = [];
@@ -286,10 +294,13 @@ class Node {
       currentPath.push(currentNode);
 
       // Add all of its dependents to our toVisit stack
-      for (const dependent of currentNode._dependents) {
-        if (toVisit.includes(dependent)) continue;
-        toVisit.push(dependent);
-        depthAdded.set(dependent, currentPath.length);
+      const nodesToExplore = direction === 'dependents' ?
+        currentNode._dependents :
+        currentNode._dependencies;
+      for (const nextNode of nodesToExplore) {
+        if (toVisit.includes(nextNode)) continue;
+        toVisit.push(nextNode);
+        depthAdded.set(nextNode, currentPath.length);
       }
     }
 

lighthouse-core/lib/dependency-graph/simulator/simulator.js

@@ -315,6 +315,10 @@ class Simulator {
    * @return {LH.Gatherer.Simulation.Result}
    */
   simulate(graph, options) {
+    if (Node.hasCycle(graph)) {
+      throw new Error('Cannot simulate graph with cycle');
+    }
+
     options = Object.assign({flexibleOrdering: false}, options);
     // initialize the necessary data containers
     this._flexibleOrdering = options.flexibleOrdering;
@@ -327,7 +331,6 @@ class Simulator {
 
     const rootNode = graph.getRootNode();
     rootNode.traverse(node => nodesNotReadyToStart.add(node));
-
     let totalElapsedTime = 0;
     let iteration = 0;
 

lighthouse-core/test/audits/uses-rel-preload-test.js

@@ -54,6 +54,7 @@ describe('Performance: uses-rel-preload audit', () => {
     const scriptNode = buildNode(3, 'http://www.example.com/script.js');
     const scriptAddedNode = buildNode(4, 'http://www.example.com/script-added.js');
 
+    mainDocumentNode.setIsMainDocument(true);
     mainDocumentNode.addDependency(rootNode);
     scriptNode.addDependency(mainDocumentNode);
     scriptAddedNode.addDependency(scriptNode);
@@ -195,7 +196,7 @@ describe('Performance: uses-rel-preload audit', () => {
     });
   });
 
-  it('does no throw on a real trace/devtools log', async () => {
+  it('does not throw on a real trace/devtools log', async () => {
     const artifacts = Object.assign({
       URL: {finalUrl: 'https://pwa.rocks/'},
       traces: {

lighthouse-core/test/lib/dependency-graph/node-test.js

@@ -6,6 +6,7 @@
 'use strict';
 
 const Node = require('../../../lib/dependency-graph/node');
+const NetworkNode = require('../../../lib/dependency-graph/network-node');
 
 const assert = require('assert');
 
@@ -103,6 +104,16 @@ describe('DependencyGraph/Node', () => {
       assert.notEqual(node, clone);
       assert.equal(clone.getDependencies().length, 0);
     });
+
+    it('should copy isMainDocument', () => {
+      const node = new Node(1);
+      node.setIsMainDocument(true);
+      const networkNode = new NetworkNode({});
+      networkNode.setIsMainDocument(true);
+
+      assert.ok(node.cloneWithoutRelationships().isMainDocument());
+      assert.ok(networkNode.cloneWithoutRelationships().isMainDocument());
+    });
   });
 
   describe('.cloneWithRelationships', () => {
@@ -241,6 +252,7 @@ describe('DependencyGraph/Node', () => {
     });
 
     it('should return true for basic cycles', () => {
+      // A - B - C - A!
       const nodeA = new Node('A');
       const nodeB = new Node('B');
       const nodeC = new Node('C');
@@ -252,6 +264,21 @@ describe('DependencyGraph/Node', () => {
       assert.equal(Node.hasCycle(nodeA), true);
     });
 
+    it('should return true for children', () => {
+      //       A!
+      //      /
+      // A - B - C
+      const nodeA = new Node('A');
+      const nodeB = new Node('B');
+      const nodeC = new Node('C');
+
+      nodeA.addDependent(nodeB);
+      nodeB.addDependent(nodeC);
+      nodeB.addDependent(nodeA);
+
+      assert.equal(Node.hasCycle(nodeC), true);
+    });
+
     it('should return true for complex cycles', () => {
       //   B - D - F - G - C!
       //  /      /
@@ -276,6 +303,13 @@ describe('DependencyGraph/Node', () => {
       nodeG.addDependent(nodeC);
 
       assert.equal(Node.hasCycle(nodeA), true);
+      assert.equal(Node.hasCycle(nodeB), true);
+      assert.equal(Node.hasCycle(nodeC), true);
+      assert.equal(Node.hasCycle(nodeD), true);
+      assert.equal(Node.hasCycle(nodeE), true);
+      assert.equal(Node.hasCycle(nodeF), true);
+      assert.equal(Node.hasCycle(nodeG), true);
+      assert.equal(Node.hasCycle(nodeH), true);
     });
 
     it('works for very large graphs', () => {

lighthouse-core/test/lib/dependency-graph/simulator/simulator-test.js

@@ -212,5 +212,15 @@ describe('DependencyGraph/Simulator', () => {
       // should be 800ms for E and 800ms for F/G
       assert.equal(resultB.timeInMs, 800 + 800);
     });
+
+    it('should throw (not hang) on graphs with cycles', () => {
+      const rootNode = new NetworkNode(request({}));
+      const depNode = new NetworkNode(request({}));
+      rootNode.addDependency(depNode);
+      depNode.addDependency(rootNode);
+
+      const simulator = new Simulator({serverResponseTimeByOrigin});
+      assert.throws(() => simulator.simulate(rootNode), /cycle/);
+    });
   });
 });