core(lantern): ignore circular initiators #11148
Conversation
There was a problem hiding this comment.
somehow has Chrome listing the initiator of a request to be itself
I'm going to assume from your lack of worry that this isn't some Chrome regression that we need to worry about? :) (like, we used to get valid information, but now instead sometimes it gives an initiator of the request itself)
| const rootRequest = createRequest(1, 'a.com', 0); | ||
| const jsRequest1 = createRequest(2, 'a.com/js1', 1); | ||
| const jsRequest2 = createRequest(3, 'a.com/js2', 1); | ||
| jsRequest1.initiator = { |
There was a problem hiding this comment.
is there a reason that createRequest() has an initiator parameter but it appears to always be set like this? :)
There was a problem hiding this comment.
good question, this time around I just didn't look at createRequest and forgot it was there 😆
when it was written I can no longer say why, either way I regret the positional-only test creators
| }; | ||
| jsRequest2.initiator = { | ||
| type: 'script', | ||
| stack: {callFrames: [{url: 'a.com/js1'}]}, |
There was a problem hiding this comment.
I had to study this test to find the circular dependency here, so maybe worth calling out in a comment? (also slowed me down that this file always uses stack to set the initiator instead of the top-level url (I had forgotten that network-recorder falls back to that), but a comment would clear that up too)
There was a problem hiding this comment.
👍 done, I also just switched to using the url way since it's clearer and not relevant for the bug at hand
| // Only add the edge if the parent is unambiguous with valid timing and isn't circular. | ||
| if (parentCandidates.length === 1 && | ||
| parentCandidates[0].startTime <= node.startTime && | ||
| !parentCandidates[0].isDependentOn(node)) { |
There was a problem hiding this comment.
should we be doing the same/similar fixes in networkRecorder._chooseInitiator? I can see an argument for "everyone should be using PageDependencyGraph", but if we're going to have networkRequest.initiator exposed, we should want it to be sensical?
There was a problem hiding this comment.
networkRecorder._chooseInitiator
actually, the relationship of _chooseInitiator with PageDependencyGraph.getNetworkInitiators is a little confusing. If seems like networkRecorder._chooseInitiator tries to get an initiator however it can, including from the top of stack, but that seems like it would short circuit PageDependencyGraph.getNetworkInitiators with an early [record.initiator.url] return and it would never look into stack, even if initiator.type is script?
Is that desired or am I missing how that works?
There was a problem hiding this comment.
should we be doing the same/similar fixes in networkRecorder._chooseInitiator?
I don't think so. We really don't have a way of piecing together the full graph picture in networkRecorder without duplicating page-depedency-graph and we have to have some layer that represents what Chrome said. I'm OK keeping the request the (mostly) raw info from the protocol and filtering out information that's conflicting in aggregate at this graph layer.
There was a problem hiding this comment.
If seems like networkRecorder._chooseInitiator tries to get an initiator however it can, including from the top of stack, but that seems like it would short circuit PageDependencyGraph.getNetworkInitiators with an early [record.initiator.url] return and it would never look into stack, even if initiator.type is script?
That won't happen because in page-dependency-graph we look at .initiators first and fallback to .initiatorRequest only if we can't find the requests from initiators. That behavior wasn't tested though so added a test for it 👍
This also brings up a good point that in network-recorder we don't have the same caution with ambiguity though, so added a test for that as well 👍
There was a problem hiding this comment.
ugh, initiator vs initiatorRequest, right...
Yeah I'm not worried, in this specific case after digging in more, this page has a unique set of circumstances that makes this almost valid. It looks like a client-side pushState style redirect to another HTML page which then has inline script execution of rerequesting the HTML for the new URL. In that case, the "parser" of the redirected URL HTML is the initiator of a new request for the same URL. |
|
there were nontrivial cascading changes to network recorder based on your comments if you want to take another look @brendankenny (which now also slightly impacts lantern accuracy) :) |
good job tests (and the ones catching issues (I assume with the unambiguous change) in 03e2763)! Changes look great. |
Summary
The page in #11147 somehow has Chrome listing the initiator of a request to be itself. Conceptually this can happen for any cyclic relationship so this PR adds generic base-node detection of dependent relationships and guards initiator calls with them.
Related Issues/PRs
fixes #11147