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: append sourceURL comment to eval code #13754

Merged
merged 9 commits into from
Mar 30, 2022
6 changes: 4 additions & 2 deletions lighthouse-core/gather/driver/execution-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ class ExecutionContext {
.catch(${pageFunctions.wrapRuntimeEvalErrorInBrowserString})
.then(resolve);
});
}())`,
}())
//# sourceURL=lighthouse-eval.js`,
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a few underscores to this guy? I feel like someone is using lighthouse-eval.js as their script name somewhere out in the web.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If that's the case, then the url property will be the full absolute URL ending with this name. But when we set it via sourceURL it's exactly this string.

Copy link
Member

Choose a reason for hiding this comment

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

I still think //# sourceURL=lighthouse-eval.js is within the realm of possibilities and it can't hurt to add a few underscores or something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok will do

connorjclark marked this conversation as resolved.
Show resolved Hide resolved
includeCommandLineAPI: true,
awaitPromise: true,
returnByValue: true,
Expand Down Expand Up @@ -195,7 +196,8 @@ class ExecutionContext {
${ExecutionContext._cachedNativesPreamble};
${depsSerialized};
(${mainFn})(${argsSerialized});
})()`;
})()
//# sourceURL=lighthouse-eval.js`;

await this._session.sendCommand('Page.addScriptToEvaluateOnNewDocument', {source: expression});
}
Expand Down
8 changes: 3 additions & 5 deletions lighthouse-core/gather/gatherers/js-usage.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,9 @@ class JsUsage extends FRGatherer {

for (const scriptUsage of this._scriptUsages) {
// If `url` is blank, that means the script was anonymous (eval, new Function, onload, ...).
// Or, it's because it was code Lighthouse over the protocol via `Runtime.evaluate`.
// We currently don't consider coverage of anonymous scripts, and we definitely don't want
// coverage of code Lighthouse ran to inspect the page, so we ignore this ScriptCoverage if
// url is blank.
if (scriptUsage.url === '') {
if (scriptUsage.url === '' || scriptUsage.url === 'lighthouse-eval.js') {
// We currently don't consider coverage of anonymous scripts, and we definitely don't want
// coverage of code Lighthouse ran to inspect the page, so we ignore this ScriptCoverage.
continue;
}

Expand Down
21 changes: 19 additions & 2 deletions lighthouse-core/gather/gatherers/scripts.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,22 @@ async function runInSeriesOrParallel(values, promiseMapper, runInSeries) {
}
}

/**
* Returns true if the script was created via our own calls
* to Runtime.evaluate.
* @param {LH.Crdp.Debugger.ScriptParsedEvent} script
*/
function isLighthouseRuntimeEvaluateScript(script) {
// Scripts created by Runtime.evaluate that run on the main session/frame
// result in an empty string for the embedderName.
if (!script.embedderName) return true;
connorjclark marked this conversation as resolved.
Show resolved Hide resolved

// Otherwise, when running our own code inside other frames, the embedderName
// is set to the frame's url. In that case, we rely on the special sourceURL that
// we set.
return script.hasSourceURL && script.url === 'lighthouse-eval.js';
}

/**
* @fileoverview Gets JavaScript file contents.
*/
Expand Down Expand Up @@ -68,8 +84,9 @@ class Scripts extends FRGatherer {
// it also blocks scripts from the same origin but that happen to run in a different process,
// like a worker.
if (event.method === 'Debugger.scriptParsed' && !sessionId) {
// Events without an embedderName (read: a url) are for JS that we ran over the protocol.
if (event.params.embedderName) this._scriptParsedEvents.push(event.params);
if (!isLighthouseRuntimeEvaluateScript(event.params)) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: now can we add to the condition above ;)

this._scriptParsedEvents.push(event.params);
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/test/gather/driver/execution-context-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,8 @@ const fetch = globalThis.__nativeFetch || globalThis.fetch;
})
.then(resolve);
});
}())`.trim();
}())
//# sourceURL=lighthouse-eval.js`.trim();
expect(trimTrailingWhitespace(expression)).toBe(trimTrailingWhitespace(expected));
expect(await eval(expression)).toBe(1);
});
Expand Down