Review and inject all DOM API and global dependencies

[alexnj]

Similar to #14001, #6387 and #6283, there are more dependencies on DOM APIs that might make sense to inject, like this and this and more. This issue to track if we want to review and convert these into injected references in some generic way vs. whack-a-mole each time a conflict occurs?

[adamraine]

Should we merge #13646 into this? I think saving Array.map + using isolated execution context in a few places might fix the site mentioned there.

[brendankenny]

Should we merge #13646 into this?

Yeah, definitely seems like this should be step 1. We've been talking about doing that for years, just haven't gotten around to it. Gatherers should be safe from alterations by default unless they know they need access to the non-isolated space and can then opt out.

This issue to track if we want to review and convert these into injected references in some generic way vs. whack-a-mole each time a conflict occurs?

I don't think there's any general fix, unfortunately. There's always some site that will change something. We cache window.fetch, for instance, but a site could still override Response.prototype.json and break everything. The lengths we go to for performance.now, as another example of the lengths required to try to make things iron clad:

lighthouse/lighthouse-core/gather/driver/execution-context.js

Lines 218 to 224 in 4e5d5d1

	// Ensure the native `performance.now` is not overwritable.
	const performance = window.performance;
	const performanceNow = window.performance.now;
	Object.defineProperty(performance, 'now', {
	value: () => performanceNow.call(performance),
	writable: false,
	});

(if you ever want to confuse the DevTools console repl, just override Promise.prototype.then in it :)

On top of all this, caching doesn't happen in timespans because it only works on load of a new document.

ShadowRealms could help, but that will be a pain and would only help with a subset of DOM APIs (and is behind --harmony-shadow-realm for the foreseeable future).

So, default isolation seems like the best first step, and after that I think we just have to draw a line somewhere, dividing how much we're willing to take on for sites doing destructive things to their execution environment vs just replying to issues, sorry, looks like your sites overrides X, and Lighthouse can't complete that audit if it's overridden.

[brendankenny]

From the eng sync today:

• try flipping the useIsolation default here:

  lighthouse/lighthouse-core/gather/driver/execution-context.js

  Lines 142 to 143 in 4e5d5d1

  	async evaluateAsync(expression, options = {}) {
  	const contextId = options.useIsolation ? await this._getOrCreateIsolatedContextId() : undefined;

• see what fails with yarn smoke (beyond just element IDs like in core(image-elements): use execution context isolation #14005) and probably some manual testing (that should maybe be used to augment the smoke tests :)
• switch the gatherers that need to not be isolated to explicitly use useIsolation: false (example: stacks since its whole purpose is to identify the code the page has executed)

Follow up:

• consider renaming the useIsolation option to noIsolation or runInPageContext or whatever for ergonomics of no option specified -> uses isolation

[adamraine]

Merging #13646 into this one