Skip to content
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: improve resilience of nodeId-dependent gatherers #10877

Merged
merged 5 commits into from
Jun 11, 2020

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented May 29, 2020

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 ? anymore

Related Issues/PRs
inspired by but does not address #10873
fixes #10893

Comment on lines 16 to 25
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));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a minute to understand but this is really good!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Contributor

@Beytoven Beytoven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hahaha, you're quite right. I'll rework :)

* @param {number} backendNodeId
* @return {Promise<string|undefined>}
*/
async resolveNodeId(driver, backendNodeId) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this move to the Driver class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it certainly can although nothing else will use it in this way, is your concern mostly forward looking in making the same mistake?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

either way, I think this was worth the cleanliness, so done :)

lighthouse-core/gather/driver.js Show resolved Hide resolved
*/
async resolveNodeIdToObjectId(backendNodeId) {
try {
const resolveNodeResponse = await this.sendCommand('DOM.resolveNode', {backendNodeId});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there any "call DOM.getDocument before running this" restrictions for these two new methods?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the push to frontend one yes, added a note

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM3!

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v happy about extracting these methods :)

*/
async resolveNodeIdToObjectId(backendNodeId) {
try {
const resolveNodeResponse = await this.sendCommand('DOM.resolveNode', {backendNodeId});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LCP element and ALLS diagnostics give an Error though working on web.dev/measure
7 participants