core(crc): redirects leading to iframe shouldn't be marked as critical (

#6704)
GoogleChrome · Apr 17, 2019 · 4b1a28f · 4b1a28f
1 parent 53cea6d
commit 4b1a28f
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 2 deletions.
diff --git a/lighthouse-core/computed/critical-request-chains.js b/lighthouse-core/computed/critical-request-chains.js
@@ -28,6 +28,14 @@ class CriticalRequestChains {
       return false;
     }
 
+    // Whenever a request is a redirect, we don't know if it's critical until we resolve the final
+    // destination. At that point we can assign all the properties (priority, resourceType) of the
+    // final request back to the redirect(s) that led to it.
+    // See https://github.com/GoogleChrome/lighthouse/pull/6704
+    while (request.redirectDestination) {
+      request = request.redirectDestination;
+    }
+
     // Iframes are considered High Priority but they are not render blocking
     const isIframe = request.resourceType === NetworkRequest.TYPES.Document
       && request.frameId !== mainResource.frameId;

diff --git a/lighthouse-core/test/computed/critical-request-chains-test.js b/lighthouse-core/test/computed/critical-request-chains-test.js
@@ -27,6 +27,7 @@ function mockTracingData(prioritiesList, edges) {
       finished: true,
       priority,
       initiatorRequest: null,
+      statusCode: 200,
     }));
 
   // add mock initiator information
@@ -250,13 +251,24 @@ describe('CriticalRequestChain gatherer: extractChain function', () => {
   });
 
   it('handles redirects', () => {
-    const networkRecords = mockTracingData([HIGH, HIGH, HIGH], [[0, 1], [1, 2]]);
+    const networkRecords = mockTracingData([HIGH, HIGH, HIGH, HIGH], [[0, 1], [1, 2], [1, 3]]);
     const mainResource = networkRecords[0];
 
     // Make a fake redirect
     networkRecords[1].requestId = '1:redirected.0';
     networkRecords[2].requestId = '1';
 
+    networkRecords[3].requestId = '2';
+    networkRecords[3].url = 'https://example.com/redirect-stylesheet';
+    networkRecords[3].resourceType = undefined;
+    networkRecords[3].statusCode = 302;
+    networkRecords[3].redirectDestination = {
+      redirectDestination: {
+        resourceType: NetworkRequest.TYPES.Stylesheet,
+        priority: 'High',
+      },
+    };
+
     const criticalChains = CriticalRequestChains.extractChain(networkRecords, mainResource);
     assert.deepEqual(criticalChains, {
       0: {
@@ -269,6 +281,10 @@ describe('CriticalRequestChain gatherer: extractChain function', () => {
                 request: networkRecords[2],
                 children: {},
               },
+              2: {
+                request: networkRecords[3],
+                children: {},
+              },
             },
           },
         },
@@ -309,7 +325,7 @@ describe('CriticalRequestChain gatherer: extractChain function', () => {
   });
 
   it('discards iframes as non-critical', () => {
-    const networkRecords = mockTracingData([HIGH, HIGH, HIGH], [[0, 1], [0, 2]]);
+    const networkRecords = mockTracingData([HIGH, HIGH, HIGH, HIGH], [[0, 1], [0, 2], [0, 3]]);
     const mainResource = networkRecords[0];
 
     // 1th record is the root document
@@ -326,6 +342,15 @@ describe('CriticalRequestChain gatherer: extractChain function', () => {
     networkRecords[2].mimeType = 'text/html';
     networkRecords[2].resourceType = NetworkRequest.TYPES.Document;
     networkRecords[2].frameId = '3';
+    // 4rd record is an iframe in the page with a redirect https://github.com/GoogleChrome/lighthouse/issues/6675
+    networkRecords[3].url = 'https://example.com/redirect-iframe';
+    networkRecords[3].resourceType = undefined;
+    networkRecords[3].statusCode = 302;
+    networkRecords[3].redirectDestination = {
+      resourceType: NetworkRequest.TYPES.Document,
+      priority: 'Low',
+    };
+    networkRecords[3].frameId = '4';
 
     const criticalChains = CriticalRequestChains.extractChain(networkRecords, mainResource);
     assert.deepEqual(criticalChains, {