core(main-resource): fix protocol error when page is reloaded #14520

connorjclark · 2022-11-10T20:24:14Z

tldr location.reload during a navigation results in MainDocumentContent artifact trying to fetch the contents of a resource that Chrome backend has already evicted. We only use this artifact in one place charset, FYI.

mainDocumentUrl is defined to be the last document during a navigation, so we should respect that in main-resource by returning the last record that matches this url. Note, the smoke test provided in this PR did not error (w/o these changes) if the page JS-navigates to a different url (ex: ?redirect), because in that case only one record applies and the current code correctly selects it.

Co-authored-by: Wojciech Reszelewski <wreszelewski@gmail.com>

connorjclark · 2022-11-10T20:26:43Z

cli/test/smokehouse/core-tests.js

 import redirectsHistoryPushState from './test-definitions/redirects-history-push-state.js';
 import redirectsMultipleServer from './test-definitions/redirects-multiple-server.js';
-import redirectScripts from './test-definitions/redirects-scripts.js';
+import redirectsSelf from './test-definitions/redirects-self.js';
 import redirectsSingleClient from './test-definitions/redirects-single-client.js';


drive-by sort, btw

connorjclark · 2022-11-10T20:28:47Z

core/computed/main-resource.js

@@ -22,7 +22,16 @@ class MainResource {
    const {mainDocumentUrl} = data.URL;
    if (!mainDocumentUrl) throw new Error('mainDocumentUrl must exist to get the main resource');
    const requests = await NetworkRecords.request(data.devtoolsLog, context);
-    const mainResource = NetworkAnalyzer.findResourceForUrl(requests, mainDocumentUrl);
+
+    const mainResourceRequests = requests.filter(request =>


1- Should we add a new function to NetworkAnalyzer for findLastResourceForUrl?

2- I got rid of the resourceUrl.startsWith(request.url) optimization check because my test case tripped on it, suggesting that it may be broken. I think it might need to become (resourceUrl.startsWith(request.url) || request.url.startsWith(resourceUrl)) to be correct?

Should we add a new function to NetworkAnalyzer for findLastResourceForUrl?

I think this is a good idea.

I got rid of the resourceUrl.startsWith(request.url) optimization check because my test case tripped on it, suggesting that it may be broken.

What did it trip on?

Two urls can be equal ignoring the fragment, without one being a substring of the other. Just think of a url with a fragment and one without, and consider that this filter predicate won't return the same value if you flip the order.

request.url won't ever have fragments, so it should always be the start of a URL that it's equal to when excluding fragments.

adamraine · 2022-11-10T20:51:50Z

core/computed/main-resource.js

@@ -22,7 +22,16 @@ class MainResource {
    const {mainDocumentUrl} = data.URL;
    if (!mainDocumentUrl) throw new Error('mainDocumentUrl must exist to get the main resource');
    const requests = await NetworkRecords.request(data.devtoolsLog, context);
-    const mainResource = NetworkAnalyzer.findResourceForUrl(requests, mainDocumentUrl);
+
+    const mainResourceRequests = requests.filter(request =>


Should we add a new function to NetworkAnalyzer for findLastResourceForUrl?

I think this is a good idea.

I got rid of the resourceUrl.startsWith(request.url) optimization check because my test case tripped on it, suggesting that it may be broken.

What did it trip on?

adamraine · 2022-11-10T20:52:58Z

core/computed/main-resource.js

+    // document request, we should return the last candidate here. Besides, the browser
+    // would have evicted the first request by the time `MainDocumentRequest` (a consumer
+    // of this computed artifact) attempts to fetch the contents, resulting in a protocol error.
+    const mainResource = mainResourceRequests[mainResourceRequests.length - 1];


There are a couple other places where we get the main document request this way:

lighthouse/core/lib/navigation-error.js

Line 119 in 1b843de

let mainRecord = NetworkAnalyzer.findResourceForUrl(networkRecords, url);

lighthouse/core/computed/page-dependency-graph.js

Lines 409 to 414 in 1b843de

const rootRequest = NetworkAnalyzer.findResourceForUrl(networkRecords, requestedUrl);

if (!rootRequest) throw new Error('rootRequest not found');

const rootNode = networkNodeOutput.idToNodeMap.get(rootRequest.requestId);

if (!rootNode) throw new Error('rootNode not found');

const mainDocumentRequest = NetworkAnalyzer.findResourceForUrl(networkRecords, mainDocumentUrl);

Should we be getting the last request in these places as well?

brendankenny · 2022-11-10T21:01:42Z

The update:sample-json errors are correct: you can't assume the last request for the main document URL is the actual main document request (thanks dbw for being such a gross site that you'd XHR your own html :)

Is this case generally recoverable? There's an unexpected navigation in the middle of the navigation.

I guess you can think of it as a redirect, but there's going to be a lot of strange stuff going on vs a typical redirect, resources already in the cache, etc

connorjclark · 2022-11-10T21:09:13Z

(authored before reading previous comments..)

Hehe, this is why sample json's critical request chain broke:

For dbw_tester.html, that xhr request is now the main resource! Just need to check the type is Document :)

connorjclark · 2022-11-10T21:12:18Z

I guess you can think of it as a redirect, but there's going to be a lot of strange stuff going on vs a typical redirect, resources already in the cache, etc

This seems better than the alternatives:

catch this error and make the charset audit n/a
re-define what MainDocumentContent/mainDocumentUrl means, if we don't really mean "the last document request"

We could hedge things a bit by emitting a warning when we detect a reload, treating it like the redirect warning.

brendankenny · 2022-11-10T21:20:36Z

iframes of itself will also be caught by this but have type Document.

The real alternative is issuing a navigationError in this case. It doesn't seem like any metrics should be trusted for this kind of load.

adamraine · 2022-11-10T21:26:14Z

iframes of itself will also be caught by this but have type Document.

Would the initiator type help us in this case? Looks like the main document has type other when iframes have type parser

wreszelewski · 2022-11-25T07:51:41Z

Can I help somehow to push this PR further?

core/computed/main-resource.js

connorjclark requested a review from a team as a code owner November 10, 2022 20:24

connorjclark requested review from adamraine and removed request for a team November 10, 2022 20:24

connorjclark mentioned this pull request Nov 10, 2022

core: fix for page reload #14518

Closed

connorjclark commented Nov 10, 2022

View reviewed changes

devtools-bot assigned adamraine Nov 10, 2022

devtools-bot added the waiting4reviewer label Nov 10, 2022

connorjclark commented Nov 10, 2022

View reviewed changes

connorjclark force-pushed the main-res-network-res branch from 7c3381b to 9859e0a Compare November 10, 2022 20:30

vercel bot deployed to Preview November 10, 2022 20:30 View deployment

adamraine reviewed Nov 10, 2022

View reviewed changes

vercel bot deployed to Preview November 10, 2022 21:17 View deployment

connorjclark mentioned this pull request Aug 2, 2023

core(main-resource): find last matching document request #15323

Closed

connorjclark force-pushed the main-res-network-res branch from 8de8e9f to 86b9602 Compare August 9, 2023 21:13

vercel bot deployed to Preview August 9, 2023 21:13 View deployment

vercel bot deployed to Preview August 9, 2023 21:38 View deployment

connorjclark mentioned this pull request Aug 9, 2023

tests: do not allow hashes in mock dt log #15363

Merged

adamraine approved these changes Aug 10, 2023

View reviewed changes

core/computed/main-resource.js Show resolved Hide resolved

connorjclark added 5 commits September 6, 2023 11:27

core(main-resource): fix protocol error when page is reloaded

c3389f5

filter on document types

4e7661a

sort

3c8147a

findLastDocumentForUrl

e557dba

fix test

6fe0987

connorjclark force-pushed the main-res-network-res branch from 4210ece to 6fe0987 Compare September 6, 2023 18:29

use in page dep graph too

c74d087

vercel bot deployed to Preview September 6, 2023 18:30 View deployment

connorjclark merged commit 09aba86 into main Sep 6, 2023
29 checks passed

connorjclark deleted the main-res-network-res branch September 6, 2023 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(main-resource): fix protocol error when page is reloaded #14520

core(main-resource): fix protocol error when page is reloaded #14520

connorjclark commented Nov 10, 2022 •

edited

connorjclark Nov 10, 2022

connorjclark Nov 10, 2022 •

edited

adamraine Nov 10, 2022

connorjclark Nov 10, 2022 •

edited

brendankenny Nov 10, 2022

adamraine Nov 10, 2022

adamraine Nov 10, 2022

brendankenny commented Nov 10, 2022 •

edited

connorjclark commented Nov 10, 2022 •

edited

connorjclark commented Nov 10, 2022

brendankenny commented Nov 10, 2022

adamraine commented Nov 10, 2022

wreszelewski commented Nov 25, 2022

	const rootRequest = NetworkAnalyzer.findResourceForUrl(networkRecords, requestedUrl);
	if (!rootRequest) throw new Error('rootRequest not found');
	const rootNode = networkNodeOutput.idToNodeMap.get(rootRequest.requestId);
	if (!rootNode) throw new Error('rootNode not found');

	const mainDocumentRequest = NetworkAnalyzer.findResourceForUrl(networkRecords, mainDocumentUrl);

core(main-resource): fix protocol error when page is reloaded #14520

core(main-resource): fix protocol error when page is reloaded #14520

Conversation

connorjclark commented Nov 10, 2022 • edited

connorjclark Nov 10, 2022

Choose a reason for hiding this comment

connorjclark Nov 10, 2022 • edited

Choose a reason for hiding this comment

adamraine Nov 10, 2022

Choose a reason for hiding this comment

connorjclark Nov 10, 2022 • edited

Choose a reason for hiding this comment

brendankenny Nov 10, 2022

Choose a reason for hiding this comment

adamraine Nov 10, 2022

Choose a reason for hiding this comment

adamraine Nov 10, 2022

Choose a reason for hiding this comment

brendankenny commented Nov 10, 2022 • edited

connorjclark commented Nov 10, 2022 • edited

connorjclark commented Nov 10, 2022

brendankenny commented Nov 10, 2022

adamraine commented Nov 10, 2022

wreszelewski commented Nov 25, 2022

connorjclark commented Nov 10, 2022 •

edited

connorjclark Nov 10, 2022 •

edited

connorjclark Nov 10, 2022 •

edited

brendankenny commented Nov 10, 2022 •

edited

connorjclark commented Nov 10, 2022 •

edited