core: save native HTMLElement.prototype.matches function by connorjclark · Pull Request #6283 · GoogleChrome/lighthouse

connorjclark · 2018-10-15T21:41:58Z

Summary

HTMLElement.prototype.matches was being overriden (by userland code), so we now store the native implementation at HTMLElement.prototype.__matches and use that instead.

Not sure how to make a test for this. If you use the sample HTML provided in the associated ticket, you'll see that the error no longer occurs.

Related Issues/PRs
Fixes #5934

patrickhulce

Nice! If you wanted to be extra secure with a test, you could throw a line that overrides matches to one of our smoke test files, probably the dbw_tester.html?

connorjclark · 2018-10-15T22:33:25Z

Done, thanks for pointing me towards the smoke tests.

connorjclark · 2018-10-15T22:53:20Z

Paul found this err'ing sites:

https://www.fitreisen.de/
http://historie.brann.no/

~~I confirmed they work after these changes.~~

Well, the second one does. Looking into why the first one is failing.

exterkamp

LGTM!

connorjclark · 2018-10-15T23:16:18Z

OK, the first link fails due to specific elements having a custom property descriptor (overriding matches), but the second one fails due to overriding matches somewhere on the prototype chain (I assume, did not confirm).

Switched to saving the method on window instead of the prototype, and calling it w/ the instance explicitly w/ .call.

paulirish · 2018-10-16T00:41:35Z

patch looks great. some of the smoketests are being a problem. Perhaps there's an extra console error now?

connorjclark · 2018-10-16T01:39:30Z

yeah, seems that the two elements I added produces an extra log statement. It also makes for two fewer render blocking requests ...

I guess these smoke tests are pretty fragile? I'll update the new expected values, let me know if that is incorrect.

Output before changing expectations:

lighthouse git:(issue-5934-matches-is-not-a-fn) ✗ yarn smoke dbw
yarn run v1.10.1
$ node lighthouse-cli/test/smokehouse/run-smoke.js dbw
Running ONLY smoketests for: dbw

Smoketest batch: parallel-second
dbw smoketest starting…

dbw smoketest results:
Command failed: node lighthouse-cli/test/smokehouse/smokehouse.js --config-path=lighthouse-cli/test/smokehouse/dbw-config.js --expectations-path=dobetterweb/dbw-expectations.js

Doing a run of 'http://localhost:10200/dobetterweb/dbw_tester.html'...
$ node lighthouse-cli/index.js http://localhost:10200/dobetterweb/dbw_tester.html --config-path=/Users/cjamcl/src/lighthouse/lighthouse-cli/test/smokehouse/dbw-config.js --output-path=smokehouse-32337.report.json --output=json --quiet --port=0
Asserting expected results match those found. (http://localhost:10200/dobetterweb/dbw_tester.html)
  ✓ final url: http://localhost:10200/dobetterweb/dbw_tester.html

  ✘ difference at errors-in-console.details.items.length
              expected: 6
                 found: 7

          found result:
      {
        "id": "errors-in-console",
        "title": "Browser errors were logged to the console",
        "description": "Errors logged to the console indicate unresolved problems. They can come from network request failures and other browser concerns.",
        "score": 0,
        "scoreDisplayMode": "binary",
        "rawValue": 7,
        "details": {
          "type": "table",
          "headings": [
            {
              "key": "url",
              "itemType": "url",
              "text": "URL"
            },
            {
              "key": "description",
              "itemType": "code",
              "text": "Description"
            }
          ],
          "items": [
            {
              "source": "other",
              "description": "Application Cache Error event: Manifest fetch failed (404) http://localhost:10200/dobetterweb/clock.appcache",
              "url": "http://localhost:10200/dobetterweb/dbw_tester.html"
            },
            {
              "source": "network",
              "description": "Failed to load resource: the server responded with a status of 404 (Not Found)",
              "url": "http://localhost:10200/dobetterweb/dobetterweb/dbw_tester.css?delay=100"
            },
            {
              "source": "network",
              "description": "Failed to load resource: the server responded with a status of 404 (Not Found)",
              "url": "http://localhost:10200/dobetterweb/unknown404.css?delay=200"
            },
            {
              "source": "network",
              "description": "Failed to load resource: the server responded with a status of 404 (Not Found)",
              "url": "http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000"
            },
            {
              "source": "network",
              "description": "Failed to load resource: the server responded with a status of 404 (Not Found)",
              "url": "http://localhost:10200/favicon.ico"
            },
            {
              "source": "Runtime.exception",
              "description": "TypeError: Cannot set property 'media' of null\n    at setTimeout (http://localhost:10200/dobetterweb/dbw_tester.html:24:22)",
              "url": "http://localhost:10200/dobetterweb/dbw_tester.html"
            },
            {
              "source": "Runtime.exception",
              "description": "Error: An error\n    at http://localhost:10200/dobetterweb/dbw_tester.html:63:38",
              "url": "http://localhost:10200/dobetterweb/dbw_tester.html"
            }
          ]
        }
      }


  ✘ difference at render-blocking-resources.details.items.length
              expected: 7
                 found: 5

          found result:
      {
        "id": "render-blocking-resources",
        "title": "Eliminate render-blocking resources",
        "description": "Resources are blocking the first paint of your page. Consider delivering critical JS/CSS inline and deferring all non-critical JS/styles. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/blocking-resources).",
        "score": 0.47,
        "scoreDisplayMode": "numeric",
        "rawValue": 986,
        "displayValue": "Potential savings of 990 ms",
        "details": {
          "type": "opportunity",
          "headings": [
            {
              "key": "url",
              "valueType": "url",
              "label": "URL"
            },
            {
              "key": "totalBytes",
              "valueType": "bytes",
              "label": "Size (KB)"
            },
            {
              "key": "wastedMs",
              "valueType": "timespanMs",
              "label": "Potential Savings (ms)"
            }
          ],
          "items": [
            {
              "url": "http://localhost:10200/dobetterweb/dobetterweb/dbw_tester.css?delay=100",
              "totalBytes": 171,
              "wastedMs": 377
            },
            {
              "url": "http://localhost:10200/dobetterweb/unknown404.css?delay=200",
              "totalBytes": 171,
              "wastedMs": 377
            },
            {
              "url": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200",
              "totalBytes": 853,
              "wastedMs": 377
            },
            {
              "url": "http://localhost:10200/dobetterweb/dbw_partial_a.html?delay=200",
              "totalBytes": 768,
              "wastedMs": 377
            },
            {
              "url": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped",
              "totalBytes": 853,
              "wastedMs": 227
            }
          ],
          "overallSavingsMs": 986
        }
      }

  Correctly passed 15 assertions

15 passing
2 failing
smoketest-dbw: 9636.795ms
dbw smoketest complete.

We have 1 failing smoketests: dbw
error Command failed with exit code 1.

connorjclark · 2018-10-16T01:41:02Z

My assumption is that some sort of costly processing happens with each element, and it is throwing off the expected result of all those ?delays.

patrickhulce · 2018-10-16T02:19:18Z

@hoten yeah those smoketest errors definitely aren't expected. It looks like something is wrong with the HTML that's causing the rest of the elements to be dropped.

            {
              "source": "Runtime.exception",
              "description": "TypeError: Cannot set property 'media' of null\n    at setTimeout (http://localhost:10200/dobetterweb/dbw_tester.html:24:22)",
              "url": "http://localhost:10200/dobetterweb/dbw_tester.html"
            },

is an error that shouldn't be there. A different test can't find amp-style-link which is just a few lines down from the code you added. Maybe there's something squirrely with the <object>s or script tag?

Also for the smoketest I'm thinking we want to redefine matches, not just use it to make sure our workaround is working-around something :)

paulirish · 2018-10-16T02:46:28Z

Maybe there's something squirrely with the s or script tag?

they're within the <head> tag right now. gotta move it down into the <body>

brendankenny

nj getting into the smoke tests!

brendankenny · 2018-10-16T03:07:40Z

+<object id="5934a"></object>
+<object id="5934b"></object>
+<script>
+  document.getElementById("5934a").matches = "See #5934";


can you add a comment on why these lines are here so we aren't mystified at some point in the future :)

brendankenny · 2018-10-16T03:08:01Z

        window.__nativeError = Error;
-        window.__nativeURL = URL;`);
+        window.__nativeURL = URL;
+        window.__ElementMatches = Element.prototype.matches;`);


👍👍 on the __ElementMatches instead of modifying the prototype

…flict (#5934)

connorjclark · 2018-10-16T03:29:08Z

objects in head was definitely the issue. Doh.

Also for the smoketest I'm thinking we want to redefine matches, not just use it to make sure our workaround is working-around something :)

@patrickhulce I'll add a line that also redefines HTMLElement.prototype.matches. I think we should keep the others, as my first attempt at a solution (HTMLElement.prototype.__matches = HTMLElement.prototype.matches) failed to fix scenarios where individual objects (not prototypes) have a matches property that hid access to HTMLElement.prototype.matches.

patrickhulce · 2018-10-16T03:48:10Z

Oh ya for sure didn't mean to suggest removing what you've already got 👍 :)

connorjclark requested review from brendankenny, patrickhulce and paulirish as code owners October 15, 2018 21:41

patrickhulce reviewed Oct 15, 2018

View reviewed changes

connorjclark force-pushed the issue-5934-matches-is-not-a-fn branch from 5b2a8d6 to b878b6a Compare October 15, 2018 22:32

exterkamp reviewed Oct 15, 2018

View reviewed changes

connorjclark force-pushed the issue-5934-matches-is-not-a-fn branch from b878b6a to 03617b3 Compare October 15, 2018 23:35

paulirish approved these changes Oct 16, 2018

View reviewed changes

connorjclark force-pushed the issue-5934-matches-is-not-a-fn branch from 03617b3 to d336580 Compare October 16, 2018 01:41

brendankenny reviewed Oct 16, 2018

View reviewed changes

core: save native HTMLElement.prototype.matches function to avoid con…

5319678

…flict (#5934)

connorjclark force-pushed the issue-5934-matches-is-not-a-fn branch from d336580 to 5319678 Compare October 16, 2018 03:28

paulirish merged commit dcccf6d into master Oct 19, 2018

paulirish deleted the issue-5934-matches-is-not-a-fn branch October 19, 2018 07:56

paulirish mentioned this pull request Oct 25, 2018

core(driver): save performance.now() to avoid conflict #6387

Merged

alexnj mentioned this pull request May 13, 2022

Review and inject all DOM API and global dependencies #14003

Open

Conversation

connorjclark commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

connorjclark commented Oct 15, 2018

Uh oh!

connorjclark commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

exterkamp left a comment

Choose a reason for hiding this comment

Uh oh!

connorjclark commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulirish commented Oct 16, 2018

Uh oh!

connorjclark commented Oct 16, 2018

Uh oh!

connorjclark commented Oct 16, 2018

Uh oh!

patrickhulce commented Oct 16, 2018

Uh oh!

paulirish commented Oct 16, 2018

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

brendankenny Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

connorjclark Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

brendankenny Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

connorjclark commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickhulce commented Oct 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

connorjclark commented Oct 15, 2018 •

edited

Loading

connorjclark commented Oct 15, 2018 •

edited

Loading

connorjclark commented Oct 15, 2018 •

edited

Loading

connorjclark commented Oct 16, 2018 •

edited

Loading