core(page-functions): don't try to clone a ShadowRoot #9079

NickolasBenakis · 2019-05-29T21:13:12Z

Summary
This PR fixes the #9048 issue .

It is a bug fix.

googlebot · 2019-05-29T21:13:16Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

NickolasBenakis · 2019-05-29T21:14:47Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).

The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.

The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

I signed it!

googlebot · 2019-05-29T21:14:49Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

patrickhulce

Thanks so much for jumping on this @NickolasBenakis!

Do you think you could add a test case and remove the package-lock.json?

A good test case would be adding error-prone element from the bug report to our accessibility smoke test HTML.

https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-cli/test/fixtures/a11y/a11y_tester.html#L198

yarn smoke a11y should fail without your fix, and pass with it! 🎉

patrickhulce · 2019-05-29T21:20:56Z

lighthouse-core/lib/page-functions.js

+    const match = clone.outerHTML.match(reOpeningTag);
+    return (match && match[0]) || '';
+  } catch (error) {
+    return element.localName;


Suggested change

return element.localName;

return `<${element.localName}>`;

just to match the expected output of this function under normal conditions

Thanks a lot for the feedback @patrickhulce .
I 'll remove it , my bad.
I am checking the a11y tester case now, I 've never done this before :)
But I am trying to figure out

It seems that it pass,
but I didnt add any error-prone element inside the a11y_tester.html.

That's expected :)

Steps:

Run yarn smoke a11y, it should pass (you've already done this ✅)

Temporarily comment out your fix

Add an error-prone element inside a11y_tester.html

Run yarn smoke a11y, it should fail

Uncomment your fix

Run yarn smoke a11y, it should pass

Commit the changes! 🎉

you mean something like this?

meh either way,
I run the test with npm run test
and it crashes...
well :'(

I meant something like

it('should handle dom nodes that cannot be inspected', () => { const element = dom.createElement('div'); element.cloneNode = () => { throw new Erorr('oops!') }; assert.equal(pageFunctions.getOuterHTMLSnippet(element), '<div>'); });

@patrickhulce I commited the test case :)

patrickhulce · 2019-05-29T21:21:32Z

lighthouse-core/lib/page-functions.js

+    const reOpeningTag = /^[\s\S]*?>/;
+    const match = clone.outerHTML.match(reOpeningTag);
+    return (match && match[0]) || '';
+  } catch (error) {


Suggested change

} catch (error) {

} catch (_) {

we name our unused variables _ :)

googlebot · 2019-05-31T00:01:25Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

codecov · 2019-05-31T00:01:28Z

Codecov Report

Merging #9079 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #9079      +/-   ##
==========================================
- Coverage   91.43%   91.42%   -0.02%     
==========================================
  Files         291      291              
  Lines        9917     9917              
==========================================
- Hits         9068     9067       -1     
- Misses        849      850       +1

Flag	Coverage Δ
#smoke	`84.2% <ø> (-0.09%)`	⬇️
#unit	`89.53% <ø> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
lighthouse-core/lib/page-functions.js	`100% <ø> (ø)`	⬆️
lighthouse-core/audits/user-timings.js	`96% <0%> (-4%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff4f486...51449a0. Read the comment docs.

patrickhulce

LGTM, thanks @NickolasBenakis!

@hoten want to take a look at this for another pair of eyes? I'm somewhat biased on some of these parts :)

googlebot · 2019-06-03T20:34:38Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

exterkamp · 2019-06-03T20:35:14Z

Just updating the CLA

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

googlebot · 2019-06-07T01:37:00Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

brendankenny

LGTM! Good to go when the build is green.

Thanks for sticking with this, @NickolasBenakis!

NickolasBenakis · 2019-06-07T04:40:35Z

LGTM! Good to go when the build is green.

Thanks for sticking with this, @NickolasBenakis!

My pleasure...
This is one of my first steps in open source and I had fun doing this.
I will keep my eyes open for any future issue that I could help.
=)

GoogleChrome#9048 Fix Avoid an excess DOM size error

a5f9343

NickolasBenakis requested review from brendankenny and paulirish as code owners May 29, 2019 21:13

googlebot added the cla: no label May 29, 2019

vercel bot deployed to staging May 29, 2019 21:13 View deployment

googlebot added cla: yes and removed cla: no labels May 29, 2019

NickolasBenakis changed the title ~~#9048 Fix Avoid an excess DOM size error~~ Bugfix Avoid an excess DOM size error May 29, 2019

NickolasBenakis changed the title ~~Bugfix Avoid an excess DOM size error~~ Bug-fix: Avoid an excess DOM size error May 29, 2019

NickolasBenakis changed the title ~~Bug-fix: Avoid an excess DOM size error~~ bug-fix: Avoid an excess DOM size error May 29, 2019

patrickhulce suggested changes May 29, 2019

View reviewed changes

bug-fix: Avoid an excess DOM size error GoogleChrome#9079

5de9be9

NickolasBenakis force-pushed the master branch from a5f9343 to 5de9be9 Compare May 29, 2019 22:17

vercel bot deployed to staging May 29, 2019 22:17 View deployment

bug-fix: Avoid an excess DOM size error GoogleChrome#9079

bf80f17

vercel bot deployed to staging May 29, 2019 22:18 View deployment

updates

f34fd58

googlebot added cla: no and removed cla: yes labels May 31, 2019

vercel bot deployed to staging May 31, 2019 00:01 View deployment

fix tests I broke

51449a0

vercel bot deployed to staging May 31, 2019 00:06 View deployment

Add test for nodes that cannot be inspected

a1528b6

vercel bot deployed to staging June 1, 2019 05:57 View deployment

patrickhulce approved these changes Jun 3, 2019

View reviewed changes

exterkamp added cla: yes and removed cla: no labels Jun 3, 2019

Update lighthouse-core/lib/page-functions.js

13d4753

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

googlebot added cla: no and removed cla: yes labels Jun 4, 2019

vercel bot deployed to staging June 4, 2019 20:02 View deployment

Update lighthouse-core/test/lib/page-functions-test.js

29b209d

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

vercel bot deployed to staging June 4, 2019 20:05 View deployment

GoogleChrome deleted a comment from googlebot Jun 5, 2019

exterkamp added cla: yes and removed cla: no labels Jun 5, 2019

brendankenny changed the title ~~bug-fix: Avoid an excess DOM size error~~ core(page-functions): don't try to clone a ShadowRoot Jun 7, 2019

ShadowRoot

cbd0e48

googlebot added cla: no and removed cla: yes labels Jun 7, 2019

vercel bot deployed to staging June 7, 2019 01:36 View deployment

brendankenny added cla: yes and removed cla: no labels Jun 7, 2019

GoogleChrome deleted a comment from googlebot Jun 7, 2019

brendankenny approved these changes Jun 7, 2019

View reviewed changes

brendankenny added the land-when-ci-is-green label Jun 7, 2019

patrickhulce merged commit 212b6ac into GoogleChrome:master Jun 7, 2019

brendankenny mentioned this pull request Jun 7, 2019

Avoid an excess DOM size goes "Error!" #9048

Closed

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

adamraine mentioned this pull request Oct 23, 2020

core: add devtools path to DOMStats #11578

Merged

brendankenny mentioned this pull request Jan 14, 2021

core: add type checking to page functions #11958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(page-functions): don't try to clone a ShadowRoot #9079

core(page-functions): don't try to clone a ShadowRoot #9079

What to do if you already signed the CLA

Individual signers

Corporate signers

core(page-functions): don't try to clone a ShadowRoot #9079

core(page-functions): don't try to clone a ShadowRoot #9079

Conversation

What to do if you already signed the CLA

Individual signers

Corporate signers

What to do if you already signed the CLA

Individual signers

Corporate signers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment