Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(audit): iframe with redirect shouldn't be marked as critical #6704

Merged
merged 6 commits into from
Apr 17, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lighthouse-core/computed/critical-request-chains.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ class CriticalRequestChains {
return false;
}

// Whenever a request is a redirect not all props are filled in correctly (resourceType is undefined)
// so we loop until we find the final request so we use the correct data #6675
wardpeet marked this conversation as resolved.
Show resolved Hide resolved
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;
Expand Down
29 changes: 27 additions & 2 deletions lighthouse-core/test/computed/critical-request-chains-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ function mockTracingData(prioritiesList, edges) {
finished: true,
priority,
initiatorRequest: null,
statusCode: 200,
}));

// add mock initiator information
Expand Down Expand Up @@ -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: {
Expand All @@ -269,6 +281,10 @@ describe('CriticalRequestChain gatherer: extractChain function', () => {
request: networkRecords[2],
children: {},
},
2: {
request: networkRecords[3],
children: {},
},
},
},
},
Expand Down Expand Up @@ -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
Expand All @@ -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 #6675
wardpeet marked this conversation as resolved.
Show resolved Hide resolved
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, {
Expand Down