core(dependency-graph): add cycle sanity check

lighthouse-core/gather/computed/page-dependency-graph.js

@@ -231,6 +231,10 @@ class PageDependencyGraphArtifact extends ComputedArtifact {
     PageDependencyGraphArtifact.linkNetworkNodes(rootNode, networkNodeOutput, networkRecords);
     PageDependencyGraphArtifact.linkCPUNodes(rootNode, networkNodeOutput, cpuNodes);
 
+    if (NetworkNode.hasCycle(rootNode)) {
+      throw new Error('Invalid dependency graph created, cycle detected');
+    }
+
     return rootNode;
   }
 

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

@@ -81,14 +81,8 @@ class Node {
    */
   getRootNode() {
     let rootNode = this;
-    let maxDepth = 1000;
-    while (rootNode._dependencies.length && maxDepth) {
+    while (rootNode._dependencies.length) {
       rootNode = rootNode._dependencies[0];
-      maxDepth--;
-    }
-
-    if (!maxDepth) {
-      throw new Error('Maximum depth exceeded: getRootNode');
     }
 
     return rootNode;
@@ -211,6 +205,45 @@ class Node {
 
     this._traversePaths(iterator, getNext);
   }
+
+  /**
+   * Returns whether the given node has a cycle in its dependent graph by performing a DFS.
+   * @param {!Node} node
+   * @return {boolean}
+   */
+  static hasCycle(node) {
+    const visited = new Set();
+    const currentPath = [];
+    const toVisit = [node];
+    const depthAdded = new Map([[node, 0]]);
+
+    // Keep going while we have nodes to visit in the stack
+    while (toVisit.length) {
+      // Get the last node in the stack (DFS uses stack, not queue)
+      const currentNode = toVisit.pop();
+
+      // We've hit a cycle if the node we're visiting is in our current dependency path
+      if (currentPath.includes(currentNode)) return true;
+      // If we've already visited the node, no need to revisit it
+      if (visited.has(currentNode)) continue;
+
+      // Since we're visiting this node, clear out any nodes in our path that we had to backtrack
+      while (currentPath.length > depthAdded.get(currentNode)) currentPath.pop();
+
+      // Update our data structures to reflect that we're adding this node to our path
+      visited.add(currentNode);
+      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);
+      }
+    }
+
+    return false;
+  }
 }
 
 Node.TYPES = {

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

@@ -365,16 +365,13 @@ class Simulator {
     const rootNode = this._graph.getRootNode();
     rootNode.traverse(node => nodesNotReadyToStart.add(node));
 
-    let depth = 0;
     let totalElapsedTime = 0;
 
     // root node is always ready to start
     this._markNodeAsReadyToStart(rootNode, totalElapsedTime);
 
     // loop as long as we have nodes in the queue or currently in progress
     while (nodesReadyToStart.size || nodesInProgress.size) {
-      depth++;
-
       // move all possible queued nodes to in progress
       for (const node of nodesReadyToStart) {
         this._startNodeIfPossible(node, totalElapsedTime);
@@ -391,14 +388,6 @@ class Simulator {
       for (const node of nodesInProgress) {
         this._updateProgressMadeInTimePeriod(node, minimumTime, totalElapsedTime);
       }
-
-      if (depth > 10000) {
-        throw new Error('Maximum depth exceeded: estimate');
-      }
-    }
-
-    if (nodesNotReadyToStart.size !== 0) {
-      throw new Error(`Cycle detected: ${nodesNotReadyToStart.size} unused nodes`);
     }
 
     return {

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

@@ -218,4 +218,78 @@ describe('DependencyGraph/Node', () => {
       assert.deepEqual(ids, ['F', 'E', 'D', 'B', 'C', 'A']);
     });
   });
+
+  describe('#hasCycle', () => {
+    it('should return false for DAGs', () => {
+      const graph = createComplexGraph();
+      assert.equal(Node.hasCycle(graph.nodeA), false);
+    });
+
+    it('should return false for triangular DAGs', () => {
+      //   B
+      //  / \
+      // A - C
+      const nodeA = new Node('A');
+      const nodeB = new Node('B');
+      const nodeC = new Node('C');
+
+      nodeA.addDependent(nodeC);
+      nodeA.addDependent(nodeB);
+      nodeB.addDependent(nodeC);
+
+      assert.equal(Node.hasCycle(nodeA), false);
+    });
+
+    it('should return true for basic cycles', () => {
+      const nodeA = new Node('A');
+      const nodeB = new Node('B');
+      const nodeC = new Node('C');
+
+      nodeA.addDependent(nodeB);
+      nodeB.addDependent(nodeC);
+      nodeC.addDependent(nodeA);
+
+      assert.equal(Node.hasCycle(nodeA), true);
+    });
+
+    it('should return true for complex cycles', () => {
+      //   B - D - F - G - C!
+      //  /      /
+      // A - - C - E - H
+      const nodeA = new Node('A');
+      const nodeB = new Node('B');
+      const nodeC = new Node('C');
+      const nodeD = new Node('D');
+      const nodeE = new Node('E');
+      const nodeF = new Node('F');
+      const nodeG = new Node('G');
+      const nodeH = new Node('H');
+
+      nodeA.addDependent(nodeB);
+      nodeA.addDependent(nodeC);
+      nodeB.addDependent(nodeD);
+      nodeC.addDependent(nodeE);
+      nodeC.addDependent(nodeF);
+      nodeD.addDependent(nodeF);
+      nodeE.addDependent(nodeH);
+      nodeF.addDependent(nodeG);
+      nodeG.addDependent(nodeC);
+
+      assert.equal(Node.hasCycle(nodeA), true);
+    });
+
+    it('works for very large graphs', () => {
+      const root = new Node('root');
+
+      let lastNode = root;
+      for (let i = 0; i < 10000; i++) {
+        const nextNode = new Node(`child${i}`);
+        lastNode.addDependent(nextNode);
+        lastNode = nextNode;
+      }
+
+      lastNode.addDependent(root);
+      assert.equal(Node.hasCycle(root), true);
+    });
+  });
 });