From f9a3d8f789864b3c045e5052d685954f7b248ff2 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 10 May 2018 15:44:11 -0700 Subject: [PATCH 1/3] core(uses-preload): prevent infinite loop --- lighthouse-core/audits/uses-rel-preload.js | 9 ++--- .../lib/dependency-graph/network-node.js | 4 ++- lighthouse-core/lib/dependency-graph/node.js | 16 +++++++-- .../dependency-graph/simulator/simulator.js | 5 ++- .../test/audits/uses-rel-preload-test.js | 2 +- .../test/lib/dependency-graph/node-test.js | 34 +++++++++++++++++++ .../simulator/simulator-test.js | 10 ++++++ 7 files changed, 68 insertions(+), 12 deletions(-) diff --git a/lighthouse-core/audits/uses-rel-preload.js b/lighthouse-core/audits/uses-rel-preload.js index 1221e42371dd..8c2b19641b14 100644 --- a/lighthouse-core/audits/uses-rel-preload.js +++ b/lighthouse-core/audits/uses-rel-preload.js @@ -73,7 +73,6 @@ class UsesRelPreloadAudit extends Audit { // 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} */ @@ -84,12 +83,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); } }); diff --git a/lighthouse-core/lib/dependency-graph/network-node.js b/lighthouse-core/lib/dependency-graph/network-node.js index 20fa3796990c..c5f9bdf1ea7f 100644 --- a/lighthouse-core/lib/dependency-graph/network-node.js +++ b/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; } } diff --git a/lighthouse-core/lib/dependency-graph/node.js b/lighthouse-core/lib/dependency-graph/node.js index 1fa2c1eb3538..b703ce9b427c 100644 --- a/lighthouse-core/lib/dependency-graph/node.js +++ b/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,14 @@ class Node { /** * Returns whether the given node has a cycle in its dependent graph by performing a DFS. * @param {Node} node + * @param {'dependents'|'dependencies'|'all'} [direction] * @return {boolean} */ - static hasCycle(node) { + static hasCycle(node, direction = 'all') { + if (direction === 'all') { + return Node.hasCycle(node, 'dependents') || Node.hasCycle(node, 'dependencies'); + } + const visited = new Set(); /** @type {Node[]} */ const currentPath = []; @@ -286,7 +293,10 @@ class Node { currentPath.push(currentNode); // Add all of its dependents to our toVisit stack - for (const dependent of currentNode._dependents) { + const nodesToExplore = direction === 'dependents' ? + currentNode._dependents : + currentNode._dependencies; + for (const dependent of nodesToExplore) { if (toVisit.includes(dependent)) continue; toVisit.push(dependent); depthAdded.set(dependent, currentPath.length); diff --git a/lighthouse-core/lib/dependency-graph/simulator/simulator.js b/lighthouse-core/lib/dependency-graph/simulator/simulator.js index ac103275eeaa..c4fea57d50ef 100644 --- a/lighthouse-core/lib/dependency-graph/simulator/simulator.js +++ b/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; diff --git a/lighthouse-core/test/audits/uses-rel-preload-test.js b/lighthouse-core/test/audits/uses-rel-preload-test.js index d96db4e75cb4..d164ea8ead9a 100644 --- a/lighthouse-core/test/audits/uses-rel-preload-test.js +++ b/lighthouse-core/test/audits/uses-rel-preload-test.js @@ -195,7 +195,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: { diff --git a/lighthouse-core/test/lib/dependency-graph/node-test.js b/lighthouse-core/test/lib/dependency-graph/node-test.js index 6fcc0d9c70d3..6dc216ebb2ca 100644 --- a/lighthouse-core/test/lib/dependency-graph/node-test.js +++ b/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 Node(1); + 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', () => { diff --git a/lighthouse-core/test/lib/dependency-graph/simulator/simulator-test.js b/lighthouse-core/test/lib/dependency-graph/simulator/simulator-test.js index cc5478a1fd5c..20ecb5a29fcf 100644 --- a/lighthouse-core/test/lib/dependency-graph/simulator/simulator-test.js +++ b/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/); + }); }); }); From f21a852cdec1275a891f493073e14fa18bab9c9e Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Thu, 10 May 2018 16:50:40 -0700 Subject: [PATCH 2/3] lint --- lighthouse-core/audits/uses-rel-preload.js | 6 ++---- lighthouse-core/test/audits/uses-rel-preload-test.js | 1 + lighthouse-core/test/lib/dependency-graph/node-test.js | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/audits/uses-rel-preload.js b/lighthouse-core/audits/uses-rel-preload.js index 8c2b19641b14..e6f6c2600bc0 100644 --- a/lighthouse-core/audits/uses-rel-preload.js +++ b/lighthouse-core/audits/uses-rel-preload.js @@ -62,10 +62,9 @@ class UsesRelPreloadAudit extends Audit { * @param {Set} 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: []}; } @@ -164,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); diff --git a/lighthouse-core/test/audits/uses-rel-preload-test.js b/lighthouse-core/test/audits/uses-rel-preload-test.js index d164ea8ead9a..2eec167b175e 100644 --- a/lighthouse-core/test/audits/uses-rel-preload-test.js +++ b/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); diff --git a/lighthouse-core/test/lib/dependency-graph/node-test.js b/lighthouse-core/test/lib/dependency-graph/node-test.js index 6dc216ebb2ca..65d82c64aac8 100644 --- a/lighthouse-core/test/lib/dependency-graph/node-test.js +++ b/lighthouse-core/test/lib/dependency-graph/node-test.js @@ -108,7 +108,7 @@ describe('DependencyGraph/Node', () => { it('should copy isMainDocument', () => { const node = new Node(1); node.setIsMainDocument(true); - const networkNode = new Node(1); + const networkNode = new NetworkNode({}); networkNode.setIsMainDocument(true); assert.ok(node.cloneWithoutRelationships().isMainDocument()); From b2c2fdad54798e0c9e66d39eb3dbb1791be867b1 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Fri, 11 May 2018 16:57:05 -0700 Subject: [PATCH 3/3] feedback --- lighthouse-core/lib/dependency-graph/node.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lighthouse-core/lib/dependency-graph/node.js b/lighthouse-core/lib/dependency-graph/node.js index b703ce9b427c..925aaa5aad52 100644 --- a/lighthouse-core/lib/dependency-graph/node.js +++ b/lighthouse-core/lib/dependency-graph/node.js @@ -258,11 +258,12 @@ class Node { /** * Returns whether the given node has a cycle in its dependent graph by performing a DFS. * @param {Node} node - * @param {'dependents'|'dependencies'|'all'} [direction] + * @param {'dependents'|'dependencies'|'both'} [direction] * @return {boolean} */ - static hasCycle(node, direction = 'all') { - if (direction === 'all') { + 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'); } @@ -296,10 +297,10 @@ class Node { const nodesToExplore = direction === 'dependents' ? currentNode._dependents : currentNode._dependencies; - for (const dependent of nodesToExplore) { - if (toVisit.includes(dependent)) continue; - toVisit.push(dependent); - depthAdded.set(dependent, currentPath.length); + for (const nextNode of nodesToExplore) { + if (toVisit.includes(nextNode)) continue; + toVisit.push(nextNode); + depthAdded.set(nextNode, currentPath.length); } }