From a540147dee8f0a1fbf813600db0ba247faddd57a Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 16 May 2017 13:22:00 -0700 Subject: [PATCH 1/5] fix: always ensure tracing is off before starting --- .../gather/connections/connection.js | 8 +++++--- lighthouse-core/gather/driver.js | 19 +++++++++++++++++-- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/gather/connections/connection.js b/lighthouse-core/gather/connections/connection.js index 30c7f2567bba..637f022bb585 100644 --- a/lighthouse-core/gather/connections/connection.js +++ b/lighthouse-core/gather/connections/connection.js @@ -46,15 +46,16 @@ class Connection { * Call protocol methods * @param {!string} method * @param {!Object} params + * @param {boolean=} silent * @return {!Promise} */ - sendCommand(method, params = {}) { + sendCommand(method, params = {}, silent = false) { log.formatProtocol('method => browser', {method, params}, 'verbose'); const id = ++this._lastCommandId; const message = JSON.stringify({id, method, params}); this.sendRawMessage(message); return new Promise((resolve, reject) => { - this._callbacks.set(id, {resolve, reject, method}); + this._callbacks.set(id, {resolve, reject, method, silent}); }); } @@ -98,7 +99,8 @@ class Connection { return callback.resolve(Promise.resolve().then(_ => { if (object.error) { - log.formatProtocol('method <= browser ERR', {method: callback.method}, 'error'); + const logLevel = callback.silent ? 'verbose' : 'error'; + log.formatProtocol('method <= browser ERR', {method: callback.method}, logLevel); throw new Error(`Protocol error (${callback.method}): ${object.error.message}`); } diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 8a588bf45f50..dd69f865a502 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -187,9 +187,10 @@ class Driver { * Call protocol methods * @param {!string} method * @param {!Object} params + * @param {boolean=} silent * @return {!Promise} */ - sendCommand(method, params) { + sendCommand(method, params, silent) { const domainCommand = /^(\w+)\.(enable|disable)$/.exec(method); if (domainCommand) { const enable = domainCommand[2] === 'enable'; @@ -198,7 +199,7 @@ class Driver { } } - return this._connection.sendCommand(method, params); + return this._connection.sendCommand(method, params, silent); } /** @@ -710,9 +711,23 @@ class Driver { // Enable Page domain to wait for Page.loadEventFired return this.sendCommand('Page.enable') + // ensure tracing is stopped before we can start + // see https://github.com/GoogleChrome/lighthouse/issues/1091 + .then(_ => this.endTraceIfStarted()) .then(_ => this.sendCommand('Tracing.start', tracingOpts)); } + endTraceIfStarted() { + return new Promise((resolve) => { + const traceCallback = () => resolve(); + this.once('Tracing.tracingComplete', traceCallback); + return this.sendCommand('Tracing.end', undefined, true).catch(() => { + this.off('Tracing.tracingComplete', traceCallback); + traceCallback(); + }); + }); + } + endTrace() { return new Promise((resolve, reject) => { // When the tracing has ended this will fire with a stream handle. From 14eef5dd1811e36e579c16b9c8ed81fe4992f072 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 16 May 2017 13:38:14 -0700 Subject: [PATCH 2/5] add test --- lighthouse-core/test/gather/driver-test.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index eed22af4b850..da0b156eb2e8 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -86,6 +86,8 @@ connection.sendCommand = function(command, params) { case 'ServiceWorker.enable': case 'ServiceWorker.disable': return Promise.resolve(); + case 'Tracing.end': + return Promise.reject(new Error('tracing not started')); default: throw Error(`Stub not implemented: ${command}`); } @@ -186,6 +188,21 @@ describe('Browser Driver', () => { }); }); + it('waits for tracingComplete when tracing already started', () => { + const fakeConnection = new Connection(); + const fakeDriver = new Driver(fakeConnection); + const commands = []; + fakeConnection.sendCommand = evt => { + commands.push(evt); + return Promise.resolve(); + }; + + fakeDriver.once = createOnceStub({'Tracing.tracingComplete': {}}); + return fakeDriver.beginTrace().then(() => { + assert.deepEqual(commands, ['Page.enable', 'Tracing.end', 'Tracing.start']); + }); + }); + it('will request default traceCategories', () => { return driverStub.beginTrace().then(() => { const traceCmd = sendCommandParams.find(obj => obj.command === 'Tracing.start'); From 9acbc1a18e12839506f43ea77d0262013ef18de9 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Wed, 17 May 2017 17:13:24 -0700 Subject: [PATCH 3/5] change bool to object of options --- lighthouse-core/gather/connections/connection.js | 8 ++++---- lighthouse-core/gather/driver.js | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lighthouse-core/gather/connections/connection.js b/lighthouse-core/gather/connections/connection.js index 637f022bb585..a04b386b7bbc 100644 --- a/lighthouse-core/gather/connections/connection.js +++ b/lighthouse-core/gather/connections/connection.js @@ -46,16 +46,16 @@ class Connection { * Call protocol methods * @param {!string} method * @param {!Object} params - * @param {boolean=} silent + * @param {Object=} options * @return {!Promise} */ - sendCommand(method, params = {}, silent = false) { + sendCommand(method, params = {}, options) { log.formatProtocol('method => browser', {method, params}, 'verbose'); const id = ++this._lastCommandId; const message = JSON.stringify({id, method, params}); this.sendRawMessage(message); return new Promise((resolve, reject) => { - this._callbacks.set(id, {resolve, reject, method, silent}); + this._callbacks.set(id, {resolve, reject, method, options}); }); } @@ -99,7 +99,7 @@ class Connection { return callback.resolve(Promise.resolve().then(_ => { if (object.error) { - const logLevel = callback.silent ? 'verbose' : 'error'; + const logLevel = callback.options && callback.options.silent ? 'verbose' : 'error'; log.formatProtocol('method <= browser ERR', {method: callback.method}, logLevel); throw new Error(`Protocol error (${callback.method}): ${object.error.message}`); } diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index dd69f865a502..02df850560d8 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -187,10 +187,10 @@ class Driver { * Call protocol methods * @param {!string} method * @param {!Object} params - * @param {boolean=} silent + * @param {Object=} options * @return {!Promise} */ - sendCommand(method, params, silent) { + sendCommand(method, params, options) { const domainCommand = /^(\w+)\.(enable|disable)$/.exec(method); if (domainCommand) { const enable = domainCommand[2] === 'enable'; @@ -199,7 +199,7 @@ class Driver { } } - return this._connection.sendCommand(method, params, silent); + return this._connection.sendCommand(method, params, options); } /** @@ -721,7 +721,7 @@ class Driver { return new Promise((resolve) => { const traceCallback = () => resolve(); this.once('Tracing.tracingComplete', traceCallback); - return this.sendCommand('Tracing.end', undefined, true).catch(() => { + return this.sendCommand('Tracing.end', undefined, {silent: true}).catch(() => { this.off('Tracing.tracingComplete', traceCallback); traceCallback(); }); From 7cdb197efe4c4a5fa3e492aaa0c19936b2ccbef4 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 22 May 2017 11:07:49 -0700 Subject: [PATCH 4/5] add default options --- lighthouse-core/gather/connections/connection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/gather/connections/connection.js b/lighthouse-core/gather/connections/connection.js index a04b386b7bbc..0313f8174505 100644 --- a/lighthouse-core/gather/connections/connection.js +++ b/lighthouse-core/gather/connections/connection.js @@ -49,7 +49,7 @@ class Connection { * @param {Object=} options * @return {!Promise} */ - sendCommand(method, params = {}, options) { + sendCommand(method, params = {}, options = {}) { log.formatProtocol('method => browser', {method, params}, 'verbose'); const id = ++this._lastCommandId; const message = JSON.stringify({id, method, params}); From 18c3e4869e10750fe91d4003547f572127e408fa Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 22 May 2017 11:37:08 -0700 Subject: [PATCH 5/5] feedback --- lighthouse-core/gather/connections/connection.js | 6 +++--- lighthouse-core/gather/driver.js | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/gather/connections/connection.js b/lighthouse-core/gather/connections/connection.js index 0313f8174505..ec3370892a02 100644 --- a/lighthouse-core/gather/connections/connection.js +++ b/lighthouse-core/gather/connections/connection.js @@ -46,16 +46,16 @@ class Connection { * Call protocol methods * @param {!string} method * @param {!Object} params - * @param {Object=} options + * @param {{silent: boolean}=} cmdOpts * @return {!Promise} */ - sendCommand(method, params = {}, options = {}) { + sendCommand(method, params = {}, cmdOpts = {}) { log.formatProtocol('method => browser', {method, params}, 'verbose'); const id = ++this._lastCommandId; const message = JSON.stringify({id, method, params}); this.sendRawMessage(message); return new Promise((resolve, reject) => { - this._callbacks.set(id, {resolve, reject, method, options}); + this._callbacks.set(id, {resolve, reject, method, options: cmdOpts}); }); } diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 02df850560d8..d91f8b3d32fc 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -187,10 +187,10 @@ class Driver { * Call protocol methods * @param {!string} method * @param {!Object} params - * @param {Object=} options + * @param {{silent: boolean}=} cmdOpts * @return {!Promise} */ - sendCommand(method, params, options) { + sendCommand(method, params, cmdOpts) { const domainCommand = /^(\w+)\.(enable|disable)$/.exec(method); if (domainCommand) { const enable = domainCommand[2] === 'enable'; @@ -199,7 +199,7 @@ class Driver { } } - return this._connection.sendCommand(method, params, options); + return this._connection.sendCommand(method, params, cmdOpts); } /**