core: improve resilience of nodeId-dependent gatherers#10877
Conversation
| function rerender(iterations) { | ||
| const rerender_ = (i, resolve) => { | ||
| const filler = `<div>Filler element</div>`.repeat(4000); | ||
| document.body.innerHTML = `<div id="div-${i}">${i} left</div>${filler}`; | ||
| if (i === 0) resolve(); | ||
| if (i > 0) requestAnimationFrame(() => rerender_(i - 1, resolve)); | ||
| }; | ||
|
|
||
| return new Promise(resolve => rerender_(iterations, resolve)); | ||
| } |
There was a problem hiding this comment.
Took me a minute to understand but this is really good!
There was a problem hiding this comment.
thanks @Beytoven! @connorjclark is right though, it was mean of me to expect others to parse a recursive version of something trivially iterative in nature, sorry you had to do that :) hopefully the new version is clearer 👍
Beytoven
left a comment
There was a problem hiding this comment.
LGTM!
If you have time, you could also improve the anchor elements tests in the dbw smoke tests. Right now it just checks the length but not the content. May or may not be worth the trouble though so approving anyways.
| } | ||
|
|
||
| /** Render large number of elements to fill up the backend node cache. */ | ||
| function rerender(iterations) { |
There was a problem hiding this comment.
might this be easier to read as an async function with a for loop, instead of a recursive promise function? the complexity then would just contained to requestAnimationFrame being wrapped in a Promise.
There was a problem hiding this comment.
hahaha, you're quite right. I'll rework :)
| * @param {number} backendNodeId | ||
| * @return {Promise<string|undefined>} | ||
| */ | ||
| async resolveNodeId(driver, backendNodeId) { |
There was a problem hiding this comment.
can this move to the Driver class?
There was a problem hiding this comment.
it certainly can although nothing else will use it in this way, is your concern mostly forward looking in making the same mistake?
There was a problem hiding this comment.
either way, I think this was worth the cleanliness, so done :)
| */ | ||
| async resolveNodeIdToObjectId(backendNodeId) { | ||
| try { | ||
| const resolveNodeResponse = await this.sendCommand('DOM.resolveNode', {backendNodeId}); |
There was a problem hiding this comment.
are there any "call DOM.getDocument before running this" restrictions for these two new methods?
There was a problem hiding this comment.
on the push to frontend one yes, added a note
There was a problem hiding this comment.
hmmm is this true?
a comment @Beytoven had previously written: // DOM.getDocument is necessary for pushNodesByBackendIdsToFrontend to properly retrieve nodeIds.
I am pretty sure we found that while it was necessary for getting valid nodeIds, it's not necessary if you're getting these objectIds
also our codebase only has 1 remaining call to getDocument and it's unrelated.
all that said, i wouldn't mind keeping a comment around to remind us of this fact if we ever try to get nodeIds again
There was a problem hiding this comment.
I can't tell if we're saying the same thing or not :)
a comment @Beytoven had previously written: // DOM.getDocument is necessary for pushNodesByBackendIdsToFrontend to properly retrieve nodeIds.
I'm not sure where that one is, but I see @umaar 's comment in anchor elements that's probably a typo since it uses pushNodeByPathToFrontend, which is where I added the note in driver already.
Are you saying that DOM.resolveNode also requires DOM.getDocument? That would be surprising to me because trace-elements doesn't use it but seems to work. Happy to update the jsdoc on this function if that's wrong though.
paulirish
left a comment
There was a problem hiding this comment.
v happy about extracting these methods :)
| */ | ||
| async resolveNodeIdToObjectId(backendNodeId) { | ||
| try { | ||
| const resolveNodeResponse = await this.sendCommand('DOM.resolveNode', {backendNodeId}); |
There was a problem hiding this comment.
hmmm is this true?
a comment @Beytoven had previously written: // DOM.getDocument is necessary for pushNodesByBackendIdsToFrontend to properly retrieve nodeIds.
I am pretty sure we found that while it was necessary for getting valid nodeIds, it's not necessary if you're getting these objectIds
also our codebase only has 1 remaining call to getDocument and it's unrelated.
all that said, i wouldn't mind keeping a comment around to remind us of this fact if we ever try to get nodeIds again
Summary
There are a few gatherers that fail if the node ID fell out of memory. Our best guess is that the node is evicted from the backendNode cache, thus resulting in a protocol error like "No node found". Just removing from the DOM isn't enough to trigger error... it's only when you add a bunch more nodes to the DOM after it was removed.
This PR adds a smoketest for the trace elements one since the CLS case is more common and reproducible than the LCP or anchor case.
Best repro: run on https://bonusi.soft-press.com/ to see that the category scores aren't
?anymoreRelated Issues/PRs
inspired by but does not address #10873
fixes #10893