core: don't use failed network requests as potential initiators #14819
Conversation
brendankenny
force-pushed
the
failed-initiators
branch
from
5fa3b25 to
93b0744
Compare
| // unlikely to occur in practice. In particular, the initiator's timestamp | ||
| // is after the initiated's timestamp. |
There was a problem hiding this comment.
AFAICT this timestamp comment was a c/p error (it was never true for this test or the next one)
| @@ -695,74 +529,123 @@ describe('network recorder', function() { | |||
| expect(imageRequest.initiatorRequest).toBe(preloadRequest); | |||
| }); | |||
|
|
|||
| it('should not set initiator when ambiguous', () => { | |||
| const logs = [ | |||
| it('should discard failed initiators', () => { | |||
There was a problem hiding this comment.
new test from the network requests of the page I was testing
| timestamp: normalizedTiming.networkEndTime / 1000, | ||
| type: networkRecord.resourceType || undefined, |
There was a problem hiding this comment.
Apparently we didn't have any failed requests going through networkRecordsToDevtoolsLog (at least with verification on) because the missing / 1000 would have caused any to fail the roundtrip test. Fixed the timing, added support for failed resourceTypes for good measure (needed for the document redirect chain debugging I was doing), and added the line to network-records-to-devtools-log-test.js to ensure that path is exercised going forwards
| const [rootDoc, failedDoc, refreshDoc] = records; | ||
| expect(rootDoc.initiatorRequest).toBe(undefined); | ||
| expect(failedDoc.initiatorRequest).toBe(rootDoc); | ||
| expect(refreshDoc.initiatorRequest).toBe(rootDoc); |
There was a problem hiding this comment.
without this change, this line would need to be
expect(refreshDoc.initiatorRequest).toBe(undefined) because it was unable to tell if rootDoc or failedDoc should be the initiator.
Ran into a weird case where a page refreshed itself via JS, Chrome fired and quickly cancelled (in less than a millisecond) a request for the page, then completed an h3 request for it. This could match the description of how Chrome will optimistically make h3 requests in parallel to an h2 request, but AFAIK that should be invisible outside the network stack and we'd be seeing a whole lot more failed requests for any h3 sites if it were observable.
Regardless of the reason, having two requests for the page precede the successful h3 request makes the JS stack initiator URL ambiguous and so an
initiatorRequestisn't set. A request that failed can't be the initiator, so instead it's one more case that can be ruled out.Unfortunately, requiring
finishedandfailedto be set exposed the fragility of a few older tests that were manually specifying devtoolsLog messages. Each request in those tests would require an additionalNetwork.loadingFinishedmessage for them to be marked finished, so it was easier (and considerably more concise) to switch the tests over tonetworkRecordsToDevtoolsLog. I've ensured they generate the same network requests and they hit the same checks innetwork-recordercode, but it does make this PR more of a pain. Those changes are separated into the second commit to make them slightly easier to review.