Stop the trace when multiple tabs are found. #639

wardpeet · 2016-09-08T19:26:34Z

Fixes #601
I check if multiple service workers got found and it throws an error. I check in the cli and the extension on this error and show the a good message.

brendankenny · 2016-09-08T22:29:29Z

lighthouse-core/gather/drivers/driver.js

+      let activeServiceWorkers = [];
+      registrations
+        .filter(reg => !reg.isDeleted)
+        .filter(reg => parseURL(reg.scopeURL).hostname === parseURL(pageUrl).hostname)


need to check the origin is the same, not just the hostname

oh, I think @paulirish had something about using just hostname to do some limited redirect handling. One issue is port number...e.g. our smoketest on localhost:whatever might alert on this even though one of them has a SW and one doesn't

paulirish · 2016-09-12T07:25:05Z

lighthouse-cli/index.js

@@ -145,7 +145,7 @@ lighthouse(url, flags, config)
    if (err.code === 'ECONNREFUSED') {
      console.error('Unable to connect to Chrome. Please run Chrome w/ debugging port 9222 open:');
      console.error('    npm explore -g lighthouse -- npm run chrome');
-    } else {
+    } else if (err) {
      console.error('Runtime error encountered:', err);


not a bad idea. thanks

brendankenny

some quick nits

brendankenny · 2016-09-15T22:50:38Z

lighthouse-core/gather/drivers/extension.js

+    return new Promise((resolve, reject) => {
+      this.queryCurrentTab_().then(currentTab => {
+        chrome.debugger.getTargets(targets => {
+          const tab = targets.find(tab => tab.tabId === currentTab.id);


maybe rename tab to target to keep things clearer?

yes ^^. for all three on this line.

const target = targets.find(target => target.tabId === currentTab.id);

brendankenny · 2016-09-15T22:52:31Z

lighthouse-core/gather/drivers/driver.js

+    return Promise.all([getRegistrations, getVersions, getActiveTabId]).then(res => {
+      const registrations = res[0].registrations;
+      const versions = res[1].versions;
+      const activePageId = res[2];


still should be activeTabId here?

brendankenny · 2016-09-15T22:53:40Z

lighthouse-core/gather/drivers/driver.js

+
+            const multipleTabsAttached = ver.controlledClients.length > 1;
+            const oneInactiveTabIsAttached = ver.controlledClients.length === 1 &&
+              !ver.controlledClients.find(sw => sw === activePageId);


rename sw to client

brendankenny · 2016-09-15T22:56:17Z

lighthouse-extension/app/src/popup.js

        message = 'You probably have DevTools open.' +
          ' Close DevTools to use lighthouse';
      }
+      if (message.toLowerCase().startsWith('unable to kill')) {
+        message = 'You probably have multiple tabs open.' +


You probably have multiple tabs open to the same origin or something like that?

wardpeet · 2016-09-16T06:30:10Z

Updated

wardpeet · 2016-09-17T09:10:24Z

updated, sorry for the many reviews!

paulirish · 2016-09-21T01:47:35Z

In action screenshot:

paulirish · 2016-09-21T01:30:54Z

lighthouse-core/gather/drivers/driver.js

+
+      let swHasMoreThanOneClient = false;
+      registrations
+        .filter(reg => !reg.isDeleted)


nuke this isDeleted filter. Even if it's deleted (aka unregistered) it can come back from the dead.

Wow didn't know that

paulirish · 2016-09-21T01:49:22Z

lighthouse-core/gather/drivers/driver.js

+        });
+
+      if (swHasMoreThanOneClient) {
+        throw new Error(


we'll need to call driver.disconnect around now, so we dont keep the debugger attached.

Also, Is there any other cleanup we need to do?

will check if anything else needs to be done.

only needed to disconnect the driver. Couldn't find anything else. I will have a look at what happens when we close the trace in between.. (shouldn't be part of this PR)

brendankenny

Looking good, a few more remaining things I've added. Sorry we were slow this week, lots of stuff going on.

I'll leave checkForMultipleTabsAttached for @paulirish to check, he's the one who's had his head in there :)

brendankenny · 2016-09-24T22:09:21Z

lighthouse-cli/index.js

+    .catch(err => {
+      if (err.code === 'ECONNREFUSED') {
+        showConnectionError();
+      } else if (err.message.toLowerCase().includes('multiple tabs')) {


I think you want this down in handleError()

brendankenny · 2016-09-24T22:10:44Z

lighthouse-core/gather/drivers/driver.js

+  }
+
+  /**
+   * Checks all serviceworkes and see if any duplications are running


service workers

brendankenny · 2016-09-24T22:16:45Z

lighthouse-core/index.js

@@ -44,7 +44,7 @@ const Config = require('./config/config');
 */

 module.exports = function(url, flags, configJSON) {
-  return new Promise((resolve, reject) => {
+  return (new Promise((resolve, reject) => {


any reason to add these parens here?

brendankenny · 2016-09-24T22:16:49Z

lighthouse-core/index.js

@@ -62,7 +62,7 @@ module.exports = function(url, flags, configJSON) {

    // kick off a lighthouse run
    resolve(Runner.run(driver, {url, flags, config}));
-  });
+  }));


brendankenny · 2016-09-24T22:18:26Z

lighthouse-core/gather/gather-runner.js

@@ -251,8 +260,7 @@ class GatherRunner {
        // We dont need to hold up the reporting for the reload/disconnect,
        // so we will not return a promise in here.
        driver.reloadForCleanStateIfNeeded(options).then(_ => {
-          log.log('status', 'Disconnecting from browser...');
-          driver.disconnect();
+          GatherRunner.disposeDriver(driver);


maybe move the log message back here?

Are you sure? I feel it's ok in dispose because of another source runs dispose we still want to show the log disconnecting because it's tightly coupled to disconnect?

how you have it is probably fine

brendankenny · 2016-09-24T22:19:03Z

lighthouse-core/gather/gather-runner.js

+      })
+      // cleanup on error
+      .catch(err => {
+        GatherRunner.disposeDriver(driver);


that way you can have an error specific log here. Even as simple as `log.log('status', 'Disconnecting from browser after error...'); or something

nice job adding this here. We should have been doing this for other errors already

brendankenny · 2016-09-24T22:21:06Z

lighthouse-core/gather/gather-runner.js

@@ -91,9 +92,17 @@ class GatherRunner {
      }).then(_ => {
        // Clears storage for origin.
        return driver.clearDataForOrigin(options.url);
+      }).then(_ => {
+        // Check Service Workers running
+        return driver.checkForMultipleTabsAttached(options.url);


I feel like setupDriver has a weird order. Feel free to punt to a followup PR, but I feel like the multiple tabs check should come before clearingDataForOrigin, for instance (why bother if the SW clear isn't going to work anyways?). Not sure of optimal order, though.

brendankenny · 2016-09-24T22:29:29Z

lighthouse-core/gather/drivers/extension.js

+        });
+      });
+    });
+  }


I think you want to switch the outer Promise and this.queryCurrentTab_ here, to put this.queryCurrentTab_() into the promise chain so any rejections in it will be handled. Something like:

return this.queryCurrentTab_().then(currentTab => { return new Promise((resolve, reject) => { chrome.debugger.getTargets(targets => { const target = targets.find(target => target.tabId === currentTab.id); if (!target) { reject(new Error('We can\'t find a target id.')); } resolve(target.id); }); }); });

brendankenny · 2016-09-24T22:34:01Z

oh, he already approved checkForMultipleTabsAttached, I missed that

brendankenny

a few more comments on the tests :)

brendankenny · 2016-09-24T22:39:54Z

lighthouse-core/test/lib/drivers/driver-test.js

@@ -80,4 +83,123 @@ describe('Browser Driver', () => {
    assert.notEqual(opts.url, req1.url, 'opts.url changed after the redirects');
    assert.equal(opts.url, req3.url, 'opts.url matches the last redirect');
  });
+
+  it('will fail when multiple tabs are found with the same active serviceworker', () => {


I think it would be helpful to surround these tests with a nested
describe('Multiple tab check', () => { to make it clear what the it in these are, since these are just tests for checkForMultipleTabsAttached, not end to end tests.

brendankenny · 2016-09-24T22:42:55Z

lighthouse-core/test/lib/drivers/driver-test.js

+      .then(_ => assert.ok(true), _ => assert.ok(false));
+  });
+
+  it('will succeed when old sw are deleted', () => {


is this test still what you want given what @paulirish said about deleted service workers?

brendankenny · 2016-09-24T22:43:08Z

lighthouse-core/test/lib/drivers/driver-test.js

+      .then(_ => assert.ok(true), _ => assert.ok(false));
+  });
+
+  it('will succeed when only one sw loaded', () => {


s/sw/service worker

brendankenny · 2016-09-24T22:44:00Z

lighthouse-core/test/lib/drivers/driver-test.js

 });
+
+function createOnceStub(events) {


move these to the top of the file with the other helper functions

…Attached

wardpeet · 2016-10-13T20:59:07Z

@brendankenny could you look over this :)

brendankenny

Hey Ward. Somehow I missed your update commits and thought this PR was just chilling. Thanks for pinging me and staying on this :) Looking good, just some last comments.

brendankenny · 2016-10-18T01:11:41Z

lighthouse-cli/index.js

@@ -196,6 +196,13 @@ function lighthouseRun(addresses) {
      }

      return lighthouseRun(addresses);
+    })
+    .catch(err => {


unfortunately I can't remember at all now, but why is this catch necessary and errors won't be handled by handleError?

Prolly just a mistake

brendankenny · 2016-10-18T01:14:15Z

lighthouse-core/gather/drivers/driver.js

+    return new Promise(resolve => {
+      this.once('ServiceWorker.workerRegistrationUpdated', data => {
+        this.sendCommand('ServiceWorker.disable');
+        resolve(data);


need to add what's been added to getServiceWorkerVersions to handle promise rejections.

this.sendCommand('ServiceWorker.disable') .then(_ => resolve(data), reject);

and this.sendCommand('ServiceWorker.enable').catch(reject);

brendankenny · 2016-10-18T01:25:15Z

lighthouse-core/gather/gather-runner.js

@@ -90,11 +91,19 @@ class GatherRunner {
      .then(_ => {
        return driver.cleanAndDisableBrowserCaches();
      }).then(_ => {
+        // Check if multiple tabs of origin are open


yeah, I now think this check definitely be moved up first. No point in getting started if it's not going to work out. Maybe just:

return driver.checkForMultipleTabsAttached(options.url) .then(_ => { // Enable emulation if required. return options.flags.mobile && driver.beginEmulation(); }).then(_ => { return driver.cleanAndDisableBrowserCaches(); }).then(_ => { // Clears storage for origin. return driver.clearDataForOrigin(options.url); });

?

brendankenny · 2016-10-18T01:26:00Z

lighthouse-core/gather/gather-runner.js

@@ -251,8 +260,7 @@ class GatherRunner {
        // We dont need to hold up the reporting for the reload/disconnect,
        // so we will not return a promise in here.
        driver.reloadForCleanStateIfNeeded(options).then(_ => {
-          log.log('status', 'Disconnecting from browser...');
-          driver.disconnect();
+          GatherRunner.disposeDriver(driver);


how you have it is probably fine

brendankenny · 2016-10-18T01:32:58Z

lighthouse-core/gather/drivers/driver.js

+        })
+        .forEach(reg => {
+          // Check if any of the service workers are the same (registration id)
+          // Check if the controlledClients are bigger than 1 and it's not the active tab


move these comments down to each step

brendankenny · 2016-10-18T01:34:47Z

lighthouse-core/gather/drivers/driver.js

+
+            const multipleTabsAttached = ver.controlledClients.length > 1;
+            const oneInactiveTabIsAttached = ver.controlledClients.length === 1 &&
+              !ver.controlledClients.find(client => client === activeTabId);


nit: since you know it's length 1, clearer to just write this line as ver.controlledClients[0] !== activeTabId;?

brendankenny · 2016-10-18T01:37:44Z

lighthouse-core/gather/drivers/driver.js

+  }
+
+  /**
+   * Checks all service workers and see if any duplications are running


something like Rejects if any open tabs would share a service worker with the target URL.?

brendankenny · 2016-10-25T23:58:19Z

hmm, smoke test is broken, saying it's hitting Runtime error encountered: Error: Not implemented in Driver.getCurrentTabId

brendankenny · 2016-10-26T00:03:09Z

lighthouse-core/gather/drivers/driver.js

+   * @return {!Promise<string>}
+   */
+  getCurrentTabId() {
+    return Promise.reject(new Error('Not implemented'));


looks like the issue is here. Driver no longer extends ExtensionConnection or CriConnection, so I believe driver's getCurrentTabId needs to call this._connection.getCurrentTabId()

brendankenny

:)

brendankenny · 2016-10-26T00:18:30Z

lighthouse-core/gather/drivers/driver.js

+        );
+      }
+
+      return res;


no need to return anything here

brendankenny · 2016-10-26T00:23:34Z

lighthouse-core/gather/gather-runner.js

-        log.log('status', 'Disconnecting from browser...');
-        return driver.disconnect();
-      }).then(_ => {
+        // We dont need to hold up the reporting for the reload/disconnect,


this section doesn't exist anymore after #816. I think you just want

.then(_ => GatherRunner.disposeDriver(driver)) .then(_ => { // Collate all the gatherer results. ...

brendankenny · 2016-10-26T00:24:29Z

lighthouse-core/gather/gather-runner.js

      .then(_ => driver.enableRuntimeEvents())
      .then(_ => driver.cleanAndDisableBrowserCaches())
      .then(_ => driver.clearDataForOrigin(options.url));
  }

+  static disposeDriver(driver) {
+    log.log('status', 'Disconnecting from browser...');
+    driver.disconnect();


should copy the

// We dont need to hold up the reporting for the reload/disconnect, // so we will not return a promise in here.

comment up here so it's clear why driver.disconnect(); isn't being returned

wardpeet · 2016-10-28T05:01:03Z

@brendankenny fixed latest nits :)

brendankenny

Looks great! 🎊 🎉 🎈 🍰

…oogleChrome#639)

brendankenny reviewed Sep 8, 2016
View reviewed changes

wardpeet force-pushed the sw/warn-tabs branch from 191a6c2 to d05bcfb Compare September 10, 2016 17:51

paulirish reviewed Sep 12, 2016
View reviewed changes

brendankenny mentioned this pull request Sep 13, 2016

Launch chrome as a part of Lighthouse CLI #642

Merged

wardpeet force-pushed the sw/warn-tabs branch from a811960 to 9ddb408 Compare September 15, 2016 21:08

brendankenny requested changes Sep 15, 2016

View reviewed changes

wardpeet force-pushed the sw/warn-tabs branch 3 times, most recently from 6dc888a to a58fef2 Compare September 17, 2016 09:09

paulirish requested changes Sep 21, 2016

View reviewed changes

paulirish approved these changes Sep 22, 2016

View reviewed changes

brendankenny requested changes Sep 24, 2016

View reviewed changes

brendankenny reviewed Sep 24, 2016

View reviewed changes

wardpeet added 2 commits October 4, 2016 17:55

Updated gatherer to check if multiple service workers are registered

2a9d09b

Show error messages to the user

eb0d9a9

wardpeet added 6 commits October 4, 2016 17:55

Change error message and name of the function to checkForMultipleTabs…

e21fd75

…Attached

Updated multiple tabs open logic

065d327

Make sw check more readable

5d571f4

Fix message variable readability

cc4b7b4

Review changes

40919bb

Latest review changes

5f7f0a6

wardpeet force-pushed the sw/warn-tabs branch from 41e390d to f8152ff Compare October 4, 2016 18:44

Review changes

acc3c4f

wardpeet force-pushed the sw/warn-tabs branch from f8152ff to acc3c4f Compare October 5, 2016 04:18

brendankenny requested changes Oct 18, 2016

View reviewed changes

wardpeet added 2 commits October 25, 2016 07:03

Merge branch 'master' into sw/warn-tabs

fcf2b0b

Fixed review @brendankenny

3b4379e

brendankenny reviewed Oct 26, 2016

View reviewed changes

brendankenny requested changes Oct 26, 2016

View reviewed changes

wardpeet added 3 commits October 26, 2016 07:02

Fix fetching tabid for cri connection

809c565

Fixes latest nits

3d2d822

Merge branch 'master' into sw/warn-tabs

a6238f2

brendankenny approved these changes Oct 28, 2016

View reviewed changes

brendankenny merged commit 48a524d into GoogleChrome:master Oct 28, 2016

This was referenced Oct 28, 2016

navigate to about:blank before driver setup #850

Merged

remove tab targetId dependency from multiple tab check #852

Merged

andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017

Stop lighthouse run when multiple tabs to the same origin are found (G…

7bd8cd4

…oogleChrome#639)

wardpeet deleted the sw/warn-tabs branch April 29, 2017 10:18

brendankenny mentioned this pull request Jun 20, 2019

core(gather-runner): add PageLoadError base artifact #9236

Merged

Stop the trace when multiple tabs are found. #639

Stop the trace when multiple tabs are found. #639

Conversation

wardpeet commented Sep 8, 2016

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

brendankenny Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

paulirish Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Sep 15, 2016 • edited Loading

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

wardpeet commented Sep 16, 2016

wardpeet commented Sep 17, 2016

paulirish commented Sep 21, 2016

paulirish Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish Sep 21, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Sep 24, 2016

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

wardpeet commented Oct 13, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Oct 25, 2016

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

wardpeet commented Oct 28, 2016

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Sep 15, 2016 •

edited

Loading

paulirish Sep 15, 2016 •

edited

Loading

brendankenny Sep 15, 2016 •

edited

Loading

paulirish Sep 21, 2016 •

edited

Loading

paulirish Sep 21, 2016 •

edited

Loading

brendankenny Sep 24, 2016 •

edited

Loading