diff --git a/lighthouse-core/computed/page-dependency-graph.js b/lighthouse-core/computed/page-dependency-graph.js index 44e1fdf42eda..e575940c8660 100644 --- a/lighthouse-core/computed/page-dependency-graph.js +++ b/lighthouse-core/computed/page-dependency-graph.js @@ -269,8 +269,11 @@ class PageDependencyGraph { const networkNodeOutput = PageDependencyGraph.getNetworkNodeOutput(networkRecords); const cpuNodes = PageDependencyGraph.getCPUNodes(traceOfTab); - const rootRequest = networkRecords.reduce((min, r) => (min.startTime < r.startTime ? min : r)); + // The root request is the earliest network request, using position in networkRecords array to break ties. + const rootRequest = networkRecords.reduce((min, r) => (r.startTime < min.startTime ? r : min)); const rootNode = networkNodeOutput.idToNodeMap.get(rootRequest.requestId); + // The main document request is the earliest network request *of type document*. + // This will be different from the root request when there are server redirects. const mainDocumentRequest = NetworkAnalyzer.findMainDocument(networkRecords); const mainDocumentNode = networkNodeOutput.idToNodeMap.get(mainDocumentRequest.requestId); @@ -279,6 +282,12 @@ class PageDependencyGraph { throw new Error(`${rootNode ? 'mainDocument' : 'root'}Node not found.`); } + if (mainDocumentNode !== rootNode && + (!mainDocumentNode.record.redirects || + !mainDocumentNode.record.redirects.includes(rootNode.record))) { + throw new Error('Root node was not in redirect chain of mainDocument'); + } + PageDependencyGraph.linkNetworkNodes(rootNode, networkNodeOutput); PageDependencyGraph.linkCPUNodes(rootNode, networkNodeOutput, cpuNodes); mainDocumentNode.setIsMainDocument(true); diff --git a/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js b/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js index 6b45bbebf76d..ef2187df5f9e 100644 --- a/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js +++ b/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js @@ -451,7 +451,8 @@ class NetworkAnalyzer { // TODO(phulce): handle more edge cases like client redirects, or plumb through finalUrl const documentRequests = records.filter(record => record.resourceType === NetworkRequest.TYPES.Document); - return documentRequests.sort((a, b) => a.startTime - b.startTime)[0]; + // The main document is the earliest document request, using position in networkRecords array to break ties. + return documentRequests.reduce((min, r) => (r.startTime < min.startTime ? r : min)); } } diff --git a/lighthouse-core/lib/network-request.js b/lighthouse-core/lib/network-request.js index dd157c868f36..883a851dfc30 100644 --- a/lighthouse-core/lib/network-request.js +++ b/lighthouse-core/lib/network-request.js @@ -272,6 +272,9 @@ module.exports = class NetworkRequest { * @param {LH.Crdp.Network.ResourceTiming} timing */ _recomputeTimesWithResourceTiming(timing) { + // Don't recompute times if the data is invalid. RequestTime should always be a thread timestamp. + // If we don't have receiveHeadersEnd, we really don't have more accurate data. + if (timing.requestTime === 0 || timing.receiveHeadersEnd === -1) return; // Take startTime and responseReceivedTime from timing data for better accuracy. // Timing's requestTime is a baseline in seconds, rest of the numbers there are ticks in millis. this.startTime = timing.requestTime; diff --git a/lighthouse-core/test/computed/page-dependency-graph-test.js b/lighthouse-core/test/computed/page-dependency-graph-test.js index c0cda7d0d707..814378405d55 100644 --- a/lighthouse-core/test/computed/page-dependency-graph-test.js +++ b/lighthouse-core/test/computed/page-dependency-graph-test.js @@ -261,9 +261,12 @@ describe('PageDependencyGraph computed artifact:', () => { }); it('should set isMainDocument on first document request', () => { - const request1 = createRequest(1, '1', 0, null, NetworkRequest.TYPES.Image); - const request2 = createRequest(2, '2', 5); - const networkRecords = [request1, request2]; + const request1 = createRequest(1, '1', 0, null, NetworkRequest.TYPES.Other); + const request2 = createRequest(2, '2', 5, null, NetworkRequest.TYPES.Document); + // Add in another unrelated + early request to make sure we pick the correct chain + const request3 = createRequest(3, '3', 0, null, NetworkRequest.TYPES.Other); + request2.redirects = [request1]; + const networkRecords = [request1, request2, request3]; addTaskEvents(0, 0, []); @@ -271,9 +274,22 @@ describe('PageDependencyGraph computed artifact:', () => { const nodes = []; graph.traverse(node => nodes.push(node)); - assert.equal(nodes.length, 2); + assert.equal(nodes.length, 3); + assert.equal(nodes[0].id, 1); assert.equal(nodes[0].isMainDocument(), false); assert.equal(nodes[1].isMainDocument(), true); + assert.equal(nodes[2].isMainDocument(), false); + }); + + it('should throw when root node is not related to main document', () => { + const request1 = createRequest(1, '1', 0, null, NetworkRequest.TYPES.Other); + const request2 = createRequest(2, '2', 5, null, NetworkRequest.TYPES.Document); + const networkRecords = [request1, request2]; + + addTaskEvents(0, 0, []); + + const fn = () => PageDependencyGraph.createGraph(traceOfTab, networkRecords); + expect(fn).toThrow(/root node.*document/i); }); }); }); diff --git a/lighthouse-core/test/lib/dependency-graph/simulator/network-analyzer-test.js b/lighthouse-core/test/lib/dependency-graph/simulator/network-analyzer-test.js index 4b42606cbcc2..af091f5278b5 100644 --- a/lighthouse-core/test/lib/dependency-graph/simulator/network-analyzer-test.js +++ b/lighthouse-core/test/lib/dependency-graph/simulator/network-analyzer-test.js @@ -374,5 +374,16 @@ describe('DependencyGraph/Simulator/NetworkAnalyzer', () => { const mainDocument = NetworkAnalyzer.findMainDocument(records); assert.equal(mainDocument.url, 'https://pwa.rocks/'); }); + + it('should break ties using position in array', async () => { + const records = [ + {url: 'http://example.com', resourceType: 'Other'}, + {url: 'https://example.com', resourceType: 'Other'}, + {url: 'https://www.example.com', resourceType: 'Document', startTime: 0}, + {url: 'https://www.iframe.com', resourceType: 'Document', startTime: 0}, + ]; + const mainDocument = NetworkAnalyzer.findMainDocument(records); + assert.equal(mainDocument.url, 'https://www.example.com'); + }); }); }); diff --git a/lighthouse-core/test/lib/network-recorder-test.js b/lighthouse-core/test/lib/network-recorder-test.js index 0a409ef3dd6d..0d839bd22cc2 100644 --- a/lighthouse-core/test/lib/network-recorder-test.js +++ b/lighthouse-core/test/lib/network-recorder-test.js @@ -6,6 +6,7 @@ 'use strict'; const NetworkRecorder = require('../../lib/network-recorder'); +const networkRecordsToDevtoolsLog = require('../network-records-to-devtools-log.js'); const assert = require('assert'); const devtoolsLogItems = require('../fixtures/artifacts/perflog/defaultPass.devtoolslog.json'); const redirectsDevtoolsLog = require('../fixtures/wikipedia-redirect.devtoolslog.json'); @@ -45,7 +46,7 @@ describe('network recorder', function() { assert.equal(mainDocument.resourceType, 'Document'); }); - it('recordsFromLogs ignores invalid records', function() { + it('recordsFromLogs ignores records with an invalid URL', function() { const logs = [ { // valid request 'method': 'Network.requestWillBeSent', @@ -55,6 +56,7 @@ describe('network recorder', function() { 'loaderId': '1', 'documentURL': 'https://www.example.com', 'request': { + // This URL is valid 'url': 'https://www.example.com', 'method': 'GET', 'headers': { @@ -80,6 +82,7 @@ describe('network recorder', function() { 'loaderId': '2', 'documentURL': 'https://www.example.com', 'request': { + // This URL is invalid 'url': 'https:', 'method': 'GET', 'headers': { @@ -105,6 +108,15 @@ describe('network recorder', function() { assert.equal(records.length, 1); }); + it('should ignore invalid `timing` data', () => { + const inputRecords = [{url: 'http://example.com', startTime: 1, endTime: 2}]; + const devtoolsLogs = networkRecordsToDevtoolsLog(inputRecords); + const responseReceived = devtoolsLogs.find(item => item.method === 'Network.responseReceived'); + responseReceived.params.response.timing = {requestTime: 0, receiveHeadersEnd: -1}; + const records = NetworkRecorder.recordsFromLogs(devtoolsLogs); + expect(records).toMatchObject([{url: 'http://example.com', startTime: 1, endTime: 2}]); + }); + describe('#findNetworkQuietPeriods', () => { function record(data) { const url = data.url || 'https://example.com';