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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Apr 17, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
74c7104
fix(audit): iframe with redirect shouldn't be marked as critical
wardpeet Dec 2, 2018
4fb0cb8
update resourcetype check
wardpeet Dec 4, 2018
13c7c17
fix critical redirects
wardpeet Dec 18, 2018
4000dc3
fix comment
wardpeet Apr 4, 2019
881e4e3
update issue string to urls
wardpeet Apr 16, 2019
7add790
update comment.
paulirish Apr 16, 2019
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
8 changes: 8 additions & 0 deletions lighthouse-core/computed/critical-request-chains.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
29 changes: 27 additions & 2 deletions lighthouse-core/test/computed/critical-request-chains-test.js
View file
Open in desktop
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 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, {
Expand Down