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

Stop the trace when multiple tabs are found. #639

Merged
merged 14 commits into from
Oct 28, 2016
2 changes: 2 additions & 0 deletions lighthouse-cli/bin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,8 @@ function showRuntimeError(err) {
function handleError(err) {
if (err.code === 'ECONNREFUSED') {
showConnectionError();
} else if (err.message.toLowerCase().includes('multiple tabs')) {
console.error(err.message);
} else {
showRuntimeError(err);
}
Expand Down
4 changes: 4 additions & 0 deletions lighthouse-core/gather/drivers/cri.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ class CriConnection extends Connection {
sendRawMessage(message) {
this._ws.send(message);
}

getCurrentTabId() {
return Promise.resolve(this._tab.id);
}
}

module.exports = CriConnection;
76 changes: 76 additions & 0 deletions lighthouse-core/gather/drivers/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,14 @@ class Driver {
return this._connection.sendCommand(method, params);
}

/**
* Get current tab id
* @return {!Promise<string>}
*/
getCurrentTabId() {
return Promise.reject(new Error('Not implemented'));
Copy link
Member

Choose a reason for hiding this comment

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

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()

}

evaluateScriptOnLoad(scriptSource) {
return this.sendCommand('Page.addScriptToEvaluateOnLoad', {
scriptSource
Expand Down Expand Up @@ -177,6 +185,74 @@ class Driver {
});
}

getServiceWorkerRegistrations() {
return new Promise((resolve, reject) => {
this.once('ServiceWorker.workerRegistrationUpdated', data => {
this.sendCommand('ServiceWorker.disable')
.then(_ => resolve(data), reject);
});

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

/**
* Rejects if any open tabs would share a service worker with the target URL.
* @param {!string} pageUrl
* @return {!Promise}
*/
Copy link
Member

@brendankenny brendankenny Sep 8, 2016

Choose a reason for hiding this comment

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

add a @return {!Promise} to be clear

checkForMultipleTabsAttached(pageUrl) {
// Get necessary information of serviceWorkers
const getRegistrations = this.getServiceWorkerRegistrations();
const getVersions = this.getServiceWorkerVersions();
const getActiveTabId = this.getCurrentTabId();

return Promise.all([getRegistrations, getVersions, getActiveTabId]).then(res => {
const registrations = res[0].registrations;
const versions = res[1].versions;
const activeTabId = res[2];
const parsedURL = parseURL(pageUrl);
const origin = `${parsedURL.protocol}//${parsedURL.hostname}` +
(parsedURL.port ? `:${parsedURL.port}` : '');

let swHasMoreThanOneClient = false;
registrations
.filter(reg => {
const parsedURL = parseURL(reg.scopeURL);
const swOrigin = `${parsedURL.protocol}//${parsedURL.hostname}` +
(parsedURL.port ? `:${parsedURL.port}` : '');

return origin === swOrigin;
})
.forEach(reg => {
swHasMoreThanOneClient = !!versions.find(ver => {
// Check if any of the service workers are the same (registration id)
if (ver.registrationId !== reg.registrationId) {
return false;
}

// Check if the controlledClients are bigger than 1 and it's not the active tab
if (!ver.controlledClients || ver.controlledClients.length === 0) {
return false;
}

const multipleTabsAttached = ver.controlledClients.length > 1;
const oneInactiveTabIsAttached = ver.controlledClients[0] !== activeTabId;

return multipleTabsAttached || oneInactiveTabIsAttached;
});
});

if (swHasMoreThanOneClient) {
throw new Error(
Copy link
Member

@paulirish paulirish Sep 21, 2016

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will check if anything else needs to be done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)

'You probably have multiple tabs open to the same origin.'
);
}

return res;
Copy link
Member

Choose a reason for hiding this comment

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

probably no need to return this

Copy link
Member

Choose a reason for hiding this comment

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

no need to return anything here

});
}

/**
* If our main document URL redirects, we will update options.url accordingly
* As such, options.url will always represent the post-redirected URL.
Expand Down
16 changes: 16 additions & 0 deletions lighthouse-core/gather/drivers/extension.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,22 @@ class ExtensionConnection extends Connection {
getCurrentTabURL() {
return this._queryCurrentTab().then(tab => tab.url);
}

getCurrentTabId() {
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);
});
});
});
}
Copy link
Member

Choose a reason for hiding this comment

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

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);
    });
  });
});

}

module.exports = ExtensionConnection;
31 changes: 24 additions & 7 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@ const path = require('path');
* 1. Setup
* A. driver.connect()
* B. GatherRunner.setupDriver()
* i. beginEmulation
* ii. cleanAndDisableBrowserCaches
* iii. clearDataForOrigin
* i. checkForMultipleTabsAttached
* ii. beginEmulation
* iii. cleanAndDisableBrowserCaches
* iiii. clearDataForOrigin
*
* 2. For each pass in the config:
* A. GatherRunner.beforePass()
Expand Down Expand Up @@ -85,12 +86,18 @@ class GatherRunner {
static setupDriver(driver, options) {
log.log('status', 'Initializing…');
// Enable emulation if required.
return Promise.resolve(options.flags.mobile && driver.beginEmulation())
return driver.checkForMultipleTabsAttached(options.url)
.then(_ => options.flags.mobile && driver.beginEmulation())
.then(_ => driver.enableRuntimeEvents())
.then(_ => driver.cleanAndDisableBrowserCaches())
.then(_ => driver.clearDataForOrigin(options.url));
}

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

Choose a reason for hiding this comment

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

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

}

/**
* Navigates to about:blank and calls beforePass() on gatherers before tracing
* has started and before navigation to the target page.
Expand Down Expand Up @@ -242,9 +249,13 @@ class GatherRunner {
});
})
.then(_ => {
log.log('status', 'Disconnecting from browser...');
return driver.disconnect();
}).then(_ => {
// We dont need to hold up the reporting for the reload/disconnect,
Copy link
Member

Choose a reason for hiding this comment

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

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

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

// so we will not return a promise in here.
driver.reloadForCleanStateIfNeeded(options).then(_ => {
GatherRunner.disposeDriver(driver);
Copy link
Member

Choose a reason for hiding this comment

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

maybe move the log message back here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

how you have it is probably fine

});
})
.then(_ => {
// Collate all the gatherer results.
const computedArtifacts = this.instantiateComputedArtifacts();
const artifacts = Object.assign({}, computedArtifacts, tracingData);
Expand All @@ -259,6 +270,12 @@ class GatherRunner {
});
});
return artifacts;
})
// cleanup on error
.catch(err => {
GatherRunner.disposeDriver(driver);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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


throw err;
});
}

Expand Down
6 changes: 6 additions & 0 deletions lighthouse-core/test/gather/fake-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ module.exports = {
beginEmulation() {
return Promise.resolve();
},
checkForMultipleTabsAttached() {
return Promise.resolve();
},
reloadForCleanStateIfNeeded() {
return Promise.resolve();
},
enableRuntimeEvents() {
return Promise.resolve();
},
Expand Down
6 changes: 6 additions & 0 deletions lighthouse-core/test/gather/gather-runner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ describe('GatherRunner', function() {
enableRuntimeEvents() {
return Promise.resolve();
},
checkForMultipleTabsAttached() {
return Promise.resolve();
},
cleanAndDisableBrowserCaches() {
return Promise.resolve();
},
Expand Down Expand Up @@ -114,6 +117,9 @@ describe('GatherRunner', function() {
enableRuntimeEvents() {
return Promise.resolve();
},
checkForMultipleTabsAttached() {
return Promise.resolve();
},
cleanAndDisableBrowserCaches() {
return Promise.resolve();
},
Expand Down
99 changes: 99 additions & 0 deletions lighthouse-core/test/lib/drivers/driver-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,32 @@ const assert = require('assert');
const connection = new Connection();
const driverStub = new Driver(connection);

function createOnceStub(events) {
return (eventName, cb) => {
if (events[eventName]) {
return cb(events[eventName]);
}

throw Error(`Stub not implemented: ${eventName}`);
};
}

function createSWRegistration(id, url, isDeleted) {
return {
isDeleted: !!isDeleted,
registrationId: id,
scopeURL: url,
};
}

function createActiveWorker(id, url, controlledClients) {
return {
registrationId: id,
scriptURL: url,
controlledClients,
};
}

connection.sendCommand = function(command, params) {
switch (command) {
case 'DOM.getDocument':
Expand All @@ -34,6 +60,9 @@ connection.sendCommand = function(command, params) {
return Promise.resolve({
nodeId: params.selector === 'invalid' ? 0 : 231
});
case 'ServiceWorker.enable':
case 'ServiceWorker.disable':
return Promise.resolve();
default:
throw Error(`Stub not implemented: ${command}`);
}
Expand Down Expand Up @@ -83,3 +112,73 @@ describe('Browser Driver', () => {
assert.equal(opts.url, req3.url, 'opts.url matches the last redirect');
});
});

describe('Multiple tab check', () => {
it('will fail when multiple tabs are found with the same active serviceworker', () => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

const pageUrl = 'https://example.com/';
const swUrl = `${pageUrl}sw.js`;
const registrations = [
createSWRegistration(1, pageUrl),
];
const versions = [
createActiveWorker(1, swUrl, ['unique'])
];

driverStub.getCurrentTabId = () => Promise.resolve('unique2');
driverStub.once = createOnceStub({
'ServiceWorker.workerRegistrationUpdated': {
registrations
},
'ServiceWorker.workerVersionUpdated': {
versions
}
});

return driverStub.checkForMultipleTabsAttached(pageUrl)
.then(_ => assert.ok(false), _ => assert.ok(true));
});

it('will succeed when service worker is already registered on current tab', () => {
const pageUrl = 'https://example.com/';
const swUrl = `${pageUrl}sw.js`;
const registrations = [
createSWRegistration(1, pageUrl),
];
const versions = [
createActiveWorker(1, swUrl, ['unique'])
];

driverStub.getCurrentTabId = () => Promise.resolve('unique');
driverStub.once = createOnceStub({
'ServiceWorker.workerRegistrationUpdated': {
registrations
},
'ServiceWorker.workerVersionUpdated': {
versions
}
});

return driverStub.checkForMultipleTabsAttached(pageUrl)
.then(_ => assert.ok(true), _ => assert.ok(false));
});

it('will succeed when only one service worker loaded', () => {
const pageUrl = 'https://example.com/';
const swUrl = `${pageUrl}sw.js`;
const registrations = [createSWRegistration(1, pageUrl)];
const versions = [createActiveWorker(1, swUrl, [])];

driverStub.getCurrentTabId = () => Promise.resolve('unique');
driverStub.once = createOnceStub({
'ServiceWorker.workerRegistrationUpdated': {
registrations
},
'ServiceWorker.workerVersionUpdated': {
versions
}
});

return driverStub.checkForMultipleTabsAttached(pageUrl)
.then(_ => assert.ok(true), _ => assert.ok(false));
});
});
9 changes: 7 additions & 2 deletions lighthouse-extension/app/src/popup.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,16 @@ document.addEventListener('DOMContentLoaded', _ => {
}, selectedAudits);
})
.catch(err => {
let {message} = err;
if (err.message.toLowerCase().startsWith('another debugger')) {
let message = err.message;
if (message.toLowerCase().startsWith('another debugger')) {
message = 'You probably have DevTools open.' +
' Close DevTools to use lighthouse';
}
if (message.toLowerCase().includes('multiple tabs')) {
message = 'You probably have multiple tabs open to the same origin.' +
' Close the other tabs to use lighthouse.';
}

feedbackEl.textContent = message;
stopSpinner();
background.console.error(err);
Expand Down