-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
core: handle invalid network timing data #6780
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -269,8 +269,11 @@ class PageDependencyGraph { | |
const networkNodeOutput = PageDependencyGraph.getNetworkNodeOutput(networkRecords); | ||
const cpuNodes = PageDependencyGraph.getCPUNodes(traceOfTab); | ||
|
||
const rootRequest = networkRecords.reduce((min, r) => (min.startTime < r.startTime ? min : r)); | ||
// The root request is the earliest network request, using position in networkRecords array to break ties. | ||
const rootRequest = networkRecords.reduce((min, r) => (r.startTime < min.startTime ? r : min)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be overkill, but should we pull this out into another method on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd want to shift consumers to be using the graph as the source of truth, so hopefully no one else needs to compute this too? :) |
||
const rootNode = networkNodeOutput.idToNodeMap.get(rootRequest.requestId); | ||
// The main document request is the earliest network request *of type document*. | ||
// This will be different from the root request when there are server redirects. | ||
const mainDocumentRequest = NetworkAnalyzer.findMainDocument(networkRecords); | ||
const mainDocumentNode = networkNodeOutput.idToNodeMap.get(mainDocumentRequest.requestId); | ||
|
||
|
@@ -279,6 +282,12 @@ class PageDependencyGraph { | |
throw new Error(`${rootNode ? 'mainDocument' : 'root'}Node not found.`); | ||
} | ||
|
||
if (mainDocumentNode !== rootNode && | ||
(!mainDocumentNode.record.redirects || | ||
!mainDocumentNode.record.redirects.includes(rootNode.record))) { | ||
throw new Error('Root node was not in redirect chain of mainDocument'); | ||
} | ||
|
||
PageDependencyGraph.linkNetworkNodes(rootNode, networkNodeOutput); | ||
PageDependencyGraph.linkCPUNodes(rootNode, networkNodeOutput, cpuNodes); | ||
mainDocumentNode.setIsMainDocument(true); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -272,6 +272,9 @@ module.exports = class NetworkRequest { | |
* @param {LH.Crdp.Network.ResourceTiming} timing | ||
*/ | ||
_recomputeTimesWithResourceTiming(timing) { | ||
// Don't recompute times if the data is invalid. RequestTime should always be a thread timestamp. | ||
// If we don't have receiveHeadersEnd, we really don't have more accurate data. | ||
if (timing.requestTime === 0 || timing.receiveHeadersEnd === -1) return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ha, I believe this would have saved me a lot of time figuring out how to generate the right network request events in #6171 |
||
// Take startTime and responseReceivedTime from timing data for better accuracy. | ||
// Timing's requestTime is a baseline in seconds, rest of the numbers there are ticks in millis. | ||
this.startTime = timing.requestTime; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
best comment