core(driver): use execution context isolation when necessary #3500

patrickhulce · 2017-10-06T23:19:41Z

paulirish · 2017-10-10T20:32:38Z

lighthouse-core/gather/driver.js

-      }).then(result => {
+      };
+
+      if (useIsolation && typeof this._isolatedExecutionContextId === 'number') {


if isolation is requested but there is no contextId... should we throw?

I had this originally to check for leaks and it seemed ok, but it's definitely possible and seemed bad to tank the run because it since it won't matter most of the time

perhaps something to add to sentry though when it lands?

Seems like we should be able to figure out definitively if it's possible to reach here without having created a new context? Then future violations of this can complain loudly by throwing (alternatively, see other comment about lazily calling _createIsolatedWorld())

going the lazy route 👍

paulirish · 2017-10-10T20:40:57Z

lighthouse-cli/test/smokehouse/run-all-tests.sh

+DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+
+for d in "$DIR"/*/ ; do
+  bash "${d}run-tests.sh"


there are a few other ways to do this, but the benefit of this approach is the failures won't be silent. :)

sgtm

can we just manually list the files in here? If I'm messing with some smoke tests it's a lot easier to comment out lines in one place by name than look up where the individual run-tests file is

also it'd probably be better to just get rid of all the run-tests.sh files since they're all exactly the same except for the expectations and config filename strings :)

will nuke 'em all in a follow up PR and improve this file here 👍

brendankenny

for future calls to evaluateAsync, how does one know if it should be isolated or not? Try the default out and if it fails (returns nothing when expected something?) try turning off isolation? :)

Is there anything we can do for these non-isolated ones (js-libraries, function callsites, and check for quiet) to make them isolated? I guess it's just unclear what's isolated and what's not if the axe checks work but js-library-detector does not

brendankenny · 2017-10-10T21:00:08Z

lighthouse-cli/test/smokehouse/run-all-tests.sh

+DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+
+for d in "$DIR"/*/ ; do
+  bash "${d}run-tests.sh"


can we just manually list the files in here? If I'm messing with some smoke tests it's a lot easier to comment out lines in one place by name than look up where the individual run-tests file is

also it'd probably be better to just get rid of all the run-tests.sh files since they're all exactly the same except for the expectations and config filename strings :)

brendankenny · 2017-10-10T21:37:18Z

lighthouse-core/gather/driver.js

   * @return {!Promise<*>}
   */
-  evaluateAsync(expression) {
+  evaluateAsync(expression, options) {
+    const {useIsolation = true} = options || {};


the double defaults here is kind of confusing. Maybe move the {} default for options to the params?

brendankenny · 2017-10-10T21:44:43Z

lighthouse-core/gather/driver.js

-      }).then(result => {
+      };
+
+      if (useIsolation && typeof this._isolatedExecutionContextId === 'number') {


Seems like we should be able to figure out definitively if it's possible to reach here without having created a new context? Then future violations of this can complain loudly by throwing (alternatively, see other comment about lazily calling _createIsolatedWorld())

brendankenny · 2017-10-10T21:52:13Z

lighthouse-core/gather/driver.js

@@ -633,6 +661,7 @@ class Driver {
      })
      .then(_ => waitForLoad && this._waitForFullyLoaded(pauseAfterLoadMs,
          networkQuietThresholdMs, cpuQuietThresholdMs, maxWaitMs))
+      .then(_ => this._createIsolatedWorld())


Would there be downsides to clearing on all navigations (as above) but calling _createIsolatedWorld lazily on next evaluateAsync() when _isolatedExecutionContextId isn't defined?

patrickhulce · 2017-10-11T00:34:58Z

Is there anything we can do for these non-isolated ones (js-libraries, function callsites, and check for quiet) to make them isolated? I guess it's just unclear what's isolated and what's not if the axe checks work but js-library-detector does not

discussed offline, decided to switch the default to useIsolation=false and just opt-in a11y for now

patrickhulce · 2017-10-13T16:57:50Z

PTAL :)

paulirish

this lgtm in general. just nits

paulirish · 2017-10-13T17:06:20Z

lighthouse-cli/test/fixtures/a11y/a11y_tester.html

@@ -197,5 +197,13 @@
    <section>
      <video id="video-description"></video>
    </section>
+    <script>
+      // axe fails if `define` is present but misbehaves, create a bogus implementation to exercise our


I don't totally get this. I'll ask you in person if i'm interpreting it correctly, but it could probably use a slightly more extended comment. :)

// axe fails if a different UMD loader like almond.js is used on the page, see https://github.com/GoogleChrome/lighthouse/issues/2505 // We can simulate this behavior by defining a similarly misbehaving `define` function.

sounds better?

YESSS so good

paulirish · 2017-10-13T17:07:04Z

lighthouse-cli/test/smokehouse/a11y/a11y-config.js

+  settings: {
+    onlyCategories: [
+      'accessibility',
+    ],


since we were talking about it yesterday... is it possible to reuse an existing smoketest?

I know we were literally just talking about not adding more yesterday but of all the testers that should probably get their own one, I think a11y is one of them 😆

maybe we can start by reducing DBW's 6 passes instead? :)

paulirish · 2017-10-13T17:10:27Z

lighthouse-core/gather/driver.js

@@ -596,6 +619,26 @@ class Driver {
  }

  /**
+   * @return {!Promise<number>}
+   */
+  _getOrCreateIsolatedContextId() {


_acquireIsolatedContextId() ?

discussed offline, Paul's a huge fan of getOrCreate now 🚗 🎏 🎉

paulirish · 2017-10-13T17:11:26Z

lighthouse-core/gather/driver.js

+    return this.sendCommand('Page.getResourceTree')
+      .then(data => {
+        const frameId = data.frameTree.frame.id;
+        return this.sendCommand('Page.createIsolatedWorld', {frameId});


let's give it a worldName while we're at it.

paulirish · 2017-10-13T17:12:15Z

lighthouse-core/gather/driver.js

+
+    return this.sendCommand('Page.getResourceTree')
+      .then(data => {
+        const frameId = data.frameTree.frame.id;


frameId => mainFrameId

brendankenny

LGTM. ⛰ 💻 🔐

brendankenny · 2017-10-13T20:36:12Z

lighthouse-core/gather/driver.js

-   * Evaluate an expression in the context of the current page.
+   * Evaluate an expression in the context of the current page. If useIsolation is true, the expression
+   * will be evaluated in a content script that has access to the page's DOM but whose JavaScript state
+   * is completely separate.


brendankenny · 2017-10-13T20:39:24Z

lighthouse-core/gather/driver.js

+   * Evaluate an expression in the given execution context; an undefined contextId implies the main
+   * page without isolation.
+   * @param {string} expression
+   * @param {number|undefined} contextId


can be {number=} FWIW

yeah just seemed more explicit since something is always passed in but doesn't matter to me

brendankenny · 2017-10-13T20:45:00Z

lighthouse-core/gather/driver.js

@@ -596,6 +619,26 @@ class Driver {
  }

  /**
+   * @return {!Promise<number>}
+   */
+  _getOrCreateIsolatedContextId() {


maybe a jsdoc comment here that the contextId won't persist past a page load (so should be cleared and recreated)?

good call, done

patrickhulce force-pushed the isolated_worlds branch from 1a3eaf8 to 03c5e29 Compare October 6, 2017 23:20

paulirish changed the title ~~fix(driver): use execution context isolation when necessary~~ fix(driver): use execution context isolation when it's necessary Oct 7, 2017

paulirish changed the title ~~fix(driver): use execution context isolation when it's necessary~~ fix(driver): use execution context isolation when necessary Oct 8, 2017

paulirish changed the title ~~fix(driver): use execution context isolation when necessary~~ fix(driver): use execution context isolation when necessary. Oct 8, 2017

paulirish reviewed Oct 10, 2017

View reviewed changes

brendankenny requested changes Oct 10, 2017

View reviewed changes

patrickhulce force-pushed the isolated_worlds branch from 03c5e29 to 2779925 Compare October 11, 2017 00:40

patrickhulce changed the title ~~fix(driver): use execution context isolation when necessary.~~ core(driver): use execution context isolation when necessary Oct 11, 2017

patrickhulce force-pushed the isolated_worlds branch from 2779925 to 9c446ec Compare October 11, 2017 00:45

patrickhulce added 2 commits October 11, 2017 11:00

fix(driver): use execution context isolation when necessary

38286b8

feedback

a860423

patrickhulce force-pushed the isolated_worlds branch from 9c446ec to a860423 Compare October 11, 2017 18:01

add unit test

0a423dd

paulirish approved these changes Oct 13, 2017

View reviewed changes

brendankenny approved these changes Oct 13, 2017

View reviewed changes

patrickhulce added 2 commits October 13, 2017 13:47

feedback

807fc31

add jsdoc comment

767dcf2

patrickhulce merged commit 04b685f into master Oct 13, 2017

patrickhulce deleted the isolated_worlds branch October 13, 2017 22:57

dependencies bot mentioned this pull request Dec 17, 2017

Update lighthouse in / from 2.5.0 to 2.7.0 chauncey-garrett/dotfiles#57

Open

core(driver): use execution context isolation when necessary #3500

core(driver): use execution context isolation when necessary #3500

Conversation

patrickhulce commented Oct 6, 2017

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

brendankenny left a 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

patrickhulce commented Oct 11, 2017

patrickhulce commented Oct 13, 2017

paulirish left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a 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