core(js-usage): normalize url key #11302

connorjclark · 2020-08-22T00:13:29Z

For some reason the url for script coverage is relative to the document. This messes up some assumptions downstream:

for the unload listener LightHouse HTML report generated in Debian not rendering in browser #11261 (comment)
overlooking coverage for some js

lighthouse/lighthouse-core/audits/byte-efficiency/unused-javascript.js

Line 89 in e1a0bf0

const networkRecord = networkRecords.find(record => record.url === url);

EDIT: the reason is because of magic sourceURL comments.

fixes #11261

patrickhulce · 2020-08-22T00:16:20Z

woah nice find!

connorjclark · 2020-08-22T00:33:57Z

we really missed the mark here :o

patrickhulce · 2020-08-22T01:10:56Z

Hm where is that coming from @connorjclark ? we do pickup most of the inline script and the unused bytes doesn't change much

connorjclark · 2020-08-22T01:20:03Z

for the bytes fixture, this is what master currently uses for keys in JS Usage:

and a lil' bit of what store.google.com looks like:

connorjclark · 2020-08-22T01:21:58Z

Hm where is that coming from @connorjclark ? we do pickup most of the inline script and the unused bytes doesn't change much

bytes smoke test. Right, but we overlook some of it because the url "key" doesn't match any network record urls, because they aren't normalized. missed in L89 linked to above

connorjclark · 2020-08-22T01:27:48Z

(totally unsure about any of the changes in expectations, verifiying ...)

connorjclark · 2020-08-22T01:42:50Z

By a manual count, there is only ~15K bytes of inline JS in tester.html. However, given this change we overcount in UnusedJavaScriptSummary.request({url, scriptCoverages, bundle}, context):

It's curious that some script coverages have a url of '', but others have the document url.

connorjclark · 2020-08-22T01:46:08Z

..... wow.

We are collecting coverage data of the code we run ourselves over the protocol.

I bet all script coverages of url '' are protocol evals.

connorjclark · 2020-08-22T01:59:59Z

(note to self and separate issue: file crbug like https://bugs.chromium.org/p/chromium/issues/detail?id=927464&q=coverage%20reporter%3Ame&can=1 but for unused code behind if ...)

https://bugs.chromium.org/p/chromium/issues/detail?id=1120879

connorjclark · 2020-08-22T02:08:13Z

lighthouse-cli/test/smokehouse/test-definitions/byte-efficiency/expectations.js

@@ -147,7 +147,7 @@ const expectations = [
            // the specific ms value here is not meaningful for this smoketest
            // *some* savings should be reported
            overallSavingsMs: '>0',
-            overallSavingsBytes: '>=25000',
+            overallSavingsBytes: '35000 +/- 1000',


+ / - is better, and good to be closer to the actual value.

WDYT about adding some assertions against the JsUsage artifact in smokes?

done, like that?

patrickhulce · 2020-08-22T03:54:54Z

sooooo, full circle this really had no impact on unused-javascript but it's nice to clean up the artifacts for other use cases and clearer understanding anyway? :)

connorjclark · 2020-08-22T04:20:00Z

sooooo, full circle this really had no impact on unused-javascript but it's nice to clean up the artifacts for other use cases and clearer understanding anyway? :)

not exactly. the smoke tests don't change, but we were ignoring coverage (total and wasted bytes) for scripts that for whatever reason had relative urls.

connorjclark · 2020-08-22T04:28:00Z

so it's a relative url when it comes from an iframe's inline scripts:

/_/gstore/_/js/k=gstore.gs.en_US.wT0--8C61Io.O/d=1/ct=zgms/rs=AF2QpWz4sGCjLARARhHywMDWhQy1Mxuxgg/m=base,main_app

or via an xhr (i don't get this. shows as an XHR in devtools, and I guess it gets eval'd, but how would CDT know to assign that script execution to this xhr URL?):

//www.gstatic.com/_/gstore/_/js/k=gstore.gs.en_US.wT0--8C61Io.O/d=0/ct=zgms/rs=AF2QpWz4sGCjLARARhHywMDWhQy1Mxuxgg/m=i50cKd,IytKEc,KtUhlb,sy2j,b4NnPb,sy3s,OiSVpc,Gph6af,NpD4ec,ws9Tlc,sy2o,mDYeOb,sy34,sy3g,IN5sze,vBnY1b,sy2r,sy4a,OZLtzd,AmCo0,tEPM8d,X63hgd,rFHKrb,ftfIab,ht3Cde,siEdTe,PqBip,eqAa9b,sy4y,sy5o,ns7pce,BH9pn,sy40,sy42,sy41,sy43,sy45,sy44,sy46,spsgvd,Xs5gRb,sy77,ZxjeB,q2fDU,gH5dsc,HV7egb,sy3u,sy58,sy3w,syy,sy3i,sy3t,sy3v,sy5x,sy59,sy6p,sy6q,sy76,sy7f,sy6r,sy7g,BlKz5e,zj4b1e,sy3y,A3htC,sy3z,ocC1vd,sy9b,sy9e,sy9f,xQtZb,sy2n,sy2q,L1AAkb,Q7UYhe,sy33,waWync,VNGsPd,rHjpXd,aW3pY,NN0gd,sy6w,WP5mZd,sy3x,AX3Okd,sy9c,uiNkee,sy9d,SM1lmd,sy6,sy2c,syg,sy2b,sy2t,syj,sy2g,sy3a,sy8,sy9,syb,syc,syd,sye,syk,sy2s,sy2y,sy2d,sy2e,sy2f,sy2l,sy3b,sy3c,sy6k,syac,fgj8Rb,BNLg8,sy57,qFbipd,RlUmc,I7Tk7e,sy72,sy73,M3lKNb,sy5,syq,sy4l,sy3l,sy4n,sy3m,sy4i,CLTtpb,sy4s,sy6z,OvhPc,sy3n,GniU2c,sy74,sy75,pAGKFb,Hchzpe,y86Xpd,pIZMdc,sy5a,vm9TOc,uaNaCf,zJdDwc,zJRogb,EVfple,rswQ7b,sy51,f1cdZb,syab,syf,K99qY,Jdbz6e,sy1x,sy9p,sy7,syl,syu,Mq9n0c,sya,syx,cQ3Jxd,sy4z,J3qlCf,bVZOGe,sy4f,WZ8Gbd,sy3h,zT0wnf,knK7bb,YiQRpc,sy54,lFKx5,KpROUe

connorjclark · 2020-08-22T04:29:07Z

anyway, let's hold this PR until I add to the bytes fixture to test coverage of JS loaded in an iframe + loaded via xhr+eval.

connorjclark · 2020-08-26T17:07:13Z

Update: nothing to do with iframes or xhr. It's all because of //# sourceURL=. This overrides the actual network request url in ScriptCoverage.url. ex: normally the coverage object for an inline script uses html document for the url. but not if there's a sourceURL comment.

We have the scriptId, so we can use that to get a more proper URL.

IMO there really should be a distinction in the protocol between network urls and the magic comment urls. This has already bit us before.

connorjclark · 2020-08-26T17:33:31Z

in a gatherer, how might I turn a scriptId into a URL (if applicable)?

patrickhulce · 2020-08-26T18:44:58Z

in a gatherer, how might I turn a scriptId into a URL (if applicable)?

AFAIK Debugger.scriptParsed is the way to get this, so if the URL it provides is not the real URL then we might be stuck. DevTools folks/@paulirish would know better though.

I have this problem in DevTools where it's impossible to get back the non-sourcemap comment URL of what I'm looking at, so that leads me to believe it might be a limitation with the current protocol setup. I hope I'm wrong 🤞

connorjclark · 2020-08-26T19:21:48Z

AFAIK Debugger.scriptParsed is the way to get this, so if the URL it provides is not the real URL then we might be stuck. DevTools folks/@paulirish would know better though.

yup, the url there is also overriden by magic url.

however, embedderName seems to be the "original" url ...

(bytes fixture)

(store.google.com, highlighted is a known inline script w/ a comment url)

(store.google.com, sometimes url is present but embedderName is blank ... I think this is eval from an xhr?)

connorjclark · 2020-08-27T01:03:27Z

this seems inconsistent with our treatment of sourcemaps. :/

I agree, but resolving will require larger changes within unused-javascript (for one, making network record not necessary). And given it's just to give a better name for inline scripts using sourceURL, I'm not inclined to prioritize the work.

Thinking out loud, an easy resolution might be to replace the url in unused-javascript items with JsUsage[realurl][0].url, which should either be the real url or the sourceURL (LinkValue is probably better so we can link to the real url, and not end up with the same discoverability issues that exist in DevTools)

paulirish · 2020-08-27T01:15:09Z

our treatment of sourcemaps

recapping.....

sourceURL is a way to tell tools that the current script has a url, mostly it's used for improved debuggability on eval() or inline scripts. used cleverly, it can mimic sourcemaps... one of webpack's many devtool settings uses sourceURL+eval. (Oh and you can change a script's real URL to a fake one with sourceURL.)
source maps tell tools that the current script is comprised of 1+ source files, and shows how. the URLs it includes in its sources property can be arbitrary too, so you could wreak similar kinds of havoc if you wanted to.

(fun fact: you can actually use both sourceURL and sourcemappingurl on the same anonymous script to give it identity and describe it's provenance)

for sourcemaps we use subitems to show how a generated/compiled/network source is comprised of its original mapped sources:

and in #10930 we are showing mapped source locations instead of the generated/compiled/network location.

this seems inconsistent with our treatment of sourcemaps. :/

I agree, but resolving will require larger changes within unused-javascript (for one, making network record not necessary). And given it's just to give a better name for inline scripts using sourceURL, I'm not inclined to prioritize the work.

Thinking out loud, an easy resolution might be to replace the url in unused-javascript items with JsUsage[realurl][0].url, which should either be the real url or the sourceURL (LinkValue is probably better so we can link to the real url, and not end up with the same discoverability issues that exist in DevTools)

K yeah. it sounds like we're mostly on the same page here then.

at first blush that resolution seems pretty good. i agree the complete solution is quite complicated and changes a lot of ways that we've been modeling scripts vs requests. i need some more time to noodle on the problem. and perhaps we can bring this discussion to the eng sync.

connorjclark · 2020-08-27T01:39:12Z

Made an issue: #11334

I think this particular PR could land, though. Keying these coverage data by the network url is the most natural thing to do here. I think any usage of urls from sourceURL comments should be done explicitly within the audits.

patrickhulce

+1 to discussing further in eng sync but still landing this great progress in the meantime :)

lighthouse-core/gather/gatherers/js-usage.js

lighthouse-core/test/gather/gatherers/js-usage-test.js

patrickhulce · 2020-08-27T02:18:21Z

lighthouse-cli/test/smokehouse/test-definitions/byte-efficiency/expectations.js

@@ -147,7 +147,7 @@ const expectations = [
            // the specific ms value here is not meaningful for this smoketest
            // *some* savings should be reported
            overallSavingsMs: '>0',
-            overallSavingsBytes: '>=25000',
+            overallSavingsBytes: '35000 +/- 1000',


WDYT about adding some assertions against the JsUsage artifact in smokes?

lighthouse-core/test/gather/gatherers/js-usage-test.js

paulirish · 2020-08-27T21:28:53Z

lighthouse-core/gather/gatherers/js-usage.js

@@ -34,9 +43,23 @@ class JsUsage extends Gatherer {
    /** @type {Record<string, Array<LH.Crdp.Profiler.ScriptCoverage>>} */
    const usageByUrl = {};
    for (const scriptUsage of scriptUsages) {
-      const scripts = usageByUrl[scriptUsage.url] || [];
+      // ScriptCoverages without a URL are from code we run over the protocol.


I think we should go a bit further and also continue if scriptParsedEvent.embedderName === ''

yup sg

lighthouse-core/gather/gatherers/js-usage.js

misc

de4f60f

connorjclark requested a review from a team as a code owner August 22, 2020 00:13

connorjclark requested review from paulirish and removed request for a team August 22, 2020 00:13

googlebot added the cla: yes label Aug 22, 2020

devtools-bot assigned paulirish Aug 22, 2020

devtools-bot added the waiting4reviewer label Aug 22, 2020

connorjclark changed the title ~~misc~~ core(js-usage): normalize url key Aug 22, 2020

maybe fix smoke

886c4c0

vercel bot deployed to Preview August 22, 2020 01:52 View deployment

connorjclark commented Aug 22, 2020

View reviewed changes

wip

f4e3fe2

vercel bot deployed to Preview August 26, 2020 17:34 View deployment

lint

e198fe7

vercel bot deployed to Preview August 27, 2020 01:06 View deployment

connorjclark mentioned this pull request Aug 27, 2020

Incorporate sourceURL comments throughout report #11334

Open

fix byte expectations

7642949

vercel bot deployed to Preview August 27, 2020 01:38 View deployment

patrickhulce reviewed Aug 27, 2020

View reviewed changes

pr

6980168

vercel bot deployed to Preview August 27, 2020 19:53 View deployment

paulirish reviewed Aug 27, 2020

View reviewed changes

pr stuff, unflake test, fix types

f8cfde7

vercel bot deployed to Preview August 27, 2020 22:20 View deployment

paulirish approved these changes Aug 28, 2020

View reviewed changes

lighthouse-core/gather/gatherers/js-usage.js Show resolved Hide resolved

fix failing smoke in stable

26a42d2

vercel bot deployed to Preview August 28, 2020 00:26 View deployment

Merge remote-tracking branch 'origin/master' into js-usage-ur

b23275e

vercel bot deployed to Preview September 1, 2020 19:12 View deployment

connorjclark removed the chillin ❄ label Sep 1, 2020

Merge remote-tracking branch 'origin/master' into js-usage-ur

6ea8efd

vercel bot deployed to Preview September 2, 2020 20:45 View deployment

fix smoke tests

d554694

vercel bot deployed to Preview September 2, 2020 21:10 View deployment

connorjclark added the land-when-ci-is-green label Sep 2, 2020

lint

899625f

vercel bot deployed to Preview September 2, 2020 21:16 View deployment

devtools-bot merged commit 8dd649a into master Sep 2, 2020

devtools-bot deleted the js-usage-ur branch September 2, 2020 21:27

devtools-bot removed the land-when-ci-is-green label Sep 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(js-usage): normalize url key #11302

core(js-usage): normalize url key #11302

connorjclark commented Aug 22, 2020 •

edited by paulirish

Loading

patrickhulce commented Aug 22, 2020

connorjclark commented Aug 22, 2020

patrickhulce commented Aug 22, 2020

connorjclark commented Aug 22, 2020 •

edited

Loading

connorjclark commented Aug 22, 2020 •

edited

Loading

connorjclark commented Aug 22, 2020

connorjclark commented Aug 22, 2020 •

edited

Loading

connorjclark commented Aug 22, 2020

connorjclark commented Aug 22, 2020 •

edited

Loading

connorjclark Aug 22, 2020 •

edited

Loading

patrickhulce Aug 27, 2020

connorjclark Aug 27, 2020

patrickhulce commented Aug 22, 2020

connorjclark commented Aug 22, 2020

connorjclark commented Aug 22, 2020

connorjclark commented Aug 22, 2020

connorjclark commented Aug 26, 2020 •

edited

Loading

connorjclark commented Aug 26, 2020

patrickhulce commented Aug 26, 2020

connorjclark commented Aug 26, 2020 •

edited

Loading

connorjclark commented Aug 27, 2020 •

edited

Loading

paulirish commented Aug 27, 2020

connorjclark commented Aug 27, 2020

patrickhulce left a comment

patrickhulce Aug 27, 2020

paulirish Aug 27, 2020

core(js-usage): normalize url key #11302

core(js-usage): normalize url key #11302

Conversation

connorjclark commented Aug 22, 2020 • edited by paulirish Loading

patrickhulce commented Aug 22, 2020

connorjclark commented Aug 22, 2020

patrickhulce commented Aug 22, 2020

connorjclark commented Aug 22, 2020 • edited Loading

connorjclark commented Aug 22, 2020 • edited Loading

connorjclark commented Aug 22, 2020

connorjclark commented Aug 22, 2020 • edited Loading

connorjclark commented Aug 22, 2020

connorjclark commented Aug 22, 2020 • edited Loading

connorjclark Aug 22, 2020 • edited Loading

Choose a reason for hiding this comment

patrickhulce Aug 27, 2020

Choose a reason for hiding this comment

connorjclark Aug 27, 2020

Choose a reason for hiding this comment

patrickhulce commented Aug 22, 2020

connorjclark commented Aug 22, 2020

connorjclark commented Aug 22, 2020

connorjclark commented Aug 22, 2020

connorjclark commented Aug 26, 2020 • edited Loading

connorjclark commented Aug 26, 2020

patrickhulce commented Aug 26, 2020

connorjclark commented Aug 26, 2020 • edited Loading

connorjclark commented Aug 27, 2020 • edited Loading

paulirish commented Aug 27, 2020

connorjclark commented Aug 27, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

patrickhulce Aug 27, 2020

Choose a reason for hiding this comment

paulirish Aug 27, 2020

Choose a reason for hiding this comment

connorjclark commented Aug 22, 2020 •

edited by paulirish

Loading

connorjclark commented Aug 22, 2020 •

edited

Loading

connorjclark commented Aug 22, 2020 •

edited

Loading

connorjclark commented Aug 22, 2020 •

edited

Loading

connorjclark commented Aug 22, 2020 •

edited

Loading

connorjclark Aug 22, 2020 •

edited

Loading

connorjclark commented Aug 26, 2020 •

edited

Loading

connorjclark commented Aug 26, 2020 •

edited

Loading

connorjclark commented Aug 27, 2020 •

edited

Loading