From d7980471a1bfa0faa4740c8e2486552d0e287685 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Fri, 26 Oct 2018 17:21:14 -0700 Subject: [PATCH 01/10] core: increase Page.navigate timeout (#6412) --- lighthouse-core/gather/driver.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 5317dfcaa5f6..43cc920f5313 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -840,7 +840,7 @@ class Driver { // happen _after_ onload: https://crbug.com/768961 this.sendCommand('Page.enable'); this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS}); - this.setNextProtocolTimeout(30 * 1000); + this.setNextProtocolTimeout(60 * 1000); // see https://github.com/GoogleChrome/lighthouse/pull/6413 this.sendCommand('Page.navigate', {url}); if (waitForLoad) { From 9af14cc8dbe749ad9d7f421e5a86b43ac7b6b849 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Mon, 29 Oct 2018 15:11:52 -0700 Subject: [PATCH 02/10] disable timeout for Page.navigate --- lighthouse-core/gather/driver.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 43cc920f5313..fbe52f11c206 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -286,18 +286,19 @@ class Driver { } } return new Promise(async (resolve, reject) => { - const asyncTimeout = setTimeout((_ => { + const asyncTimeout = timeout > 0 ? setTimeout((_ => { const err = new LHError(LHError.errors.PROTOCOL_TIMEOUT); err.message += ` Method: ${method}`; reject(err); - }), timeout); + }), timeout) : null; try { - const result = await this._connection.sendCommand(method, ...params); - clearTimeout(asyncTimeout); - resolve(result); + resolve(await this._connection.sendCommand(method, ...params)); } catch (err) { - clearTimeout(asyncTimeout); reject(err); + } finally { + if (asyncTimeout) { + clearTimeout(asyncTimeout); + } } }); } @@ -840,7 +841,7 @@ class Driver { // happen _after_ onload: https://crbug.com/768961 this.sendCommand('Page.enable'); this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS}); - this.setNextProtocolTimeout(60 * 1000); // see https://github.com/GoogleChrome/lighthouse/pull/6413 + this.setNextProtocolTimeout(0); // We don't need a strict timeout for Page.navigate. See #6413. this.sendCommand('Page.navigate', {url}); if (waitForLoad) { From 273166c4d82bf1cc4fdd3d48ece94e919d6ed746 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Mon, 29 Oct 2018 17:52:08 -0700 Subject: [PATCH 03/10] add comment --- lighthouse-core/gather/driver.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index fbe52f11c206..ab157f4e7704 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -259,6 +259,8 @@ class Driver { } /** + * If > 0, timeout is used for the next call to 'sendCommand'. + * If <= 0, no timeout is used. * NOTE: This can eventually be replaced when TypeScript * resolves https://github.com/Microsoft/TypeScript/issues/5453. * @param {number} timeout From e0fa246000b7b84dd77b72fb1634716f30b25e2e Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Mon, 29 Oct 2018 17:53:30 -0700 Subject: [PATCH 04/10] pr changes --- lighthouse-core/gather/driver.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index ab157f4e7704..0a231619f382 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -294,7 +294,8 @@ class Driver { reject(err); }), timeout) : null; try { - resolve(await this._connection.sendCommand(method, ...params)); + const result = await this._connection.sendCommand(method, ...params); + resolve(result); } catch (err) { reject(err); } finally { @@ -843,7 +844,7 @@ class Driver { // happen _after_ onload: https://crbug.com/768961 this.sendCommand('Page.enable'); this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS}); - this.setNextProtocolTimeout(0); // We don't need a strict timeout for Page.navigate. See #6413. + this.setNextProtocolTimeout(0); // We don't need a timeout for Page.navigate. See #6413. this.sendCommand('Page.navigate', {url}); if (waitForLoad) { From 7dd175a54f05efcbf0b3db9f0014296a2af70cd8 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 31 Oct 2018 14:06:28 -0700 Subject: [PATCH 05/10] no ternary --- lighthouse-core/gather/driver.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 0a231619f382..78f8605b75e1 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -288,11 +288,14 @@ class Driver { } } return new Promise(async (resolve, reject) => { - const asyncTimeout = timeout > 0 ? setTimeout((_ => { - const err = new LHError(LHError.errors.PROTOCOL_TIMEOUT); - err.message += ` Method: ${method}`; - reject(err); - }), timeout) : null; + let asyncTimeout; + if (timeout > 0) { + asyncTimeout = setTimeout((() => { + const err = new LHError(LHError.errors.PROTOCOL_TIMEOUT); + err.message += ` Method: ${method}`; + reject(err); + }), timeout); + } try { const result = await this._connection.sendCommand(method, ...params); resolve(result); From 81c00a1457b075cfb9300ba6f629a229a5366eab Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Wed, 31 Oct 2018 14:07:19 -0700 Subject: [PATCH 06/10] edit comment --- lighthouse-core/gather/driver.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 78f8605b75e1..4273bc6ed9b6 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -259,8 +259,7 @@ class Driver { } /** - * If > 0, timeout is used for the next call to 'sendCommand'. - * If <= 0, no timeout is used. + * timeout is used for the next call to 'sendCommand'. If 0, no timeout is used. * NOTE: This can eventually be replaced when TypeScript * resolves https://github.com/Microsoft/TypeScript/issues/5453. * @param {number} timeout From 3c58396246c6a19d0b27f5f7ad2012ee619ca8c6 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Mon, 19 Nov 2018 15:58:04 -0800 Subject: [PATCH 07/10] innerSendCommand --- lighthouse-core/gather/driver.js | 50 ++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 4273bc6ed9b6..91b0db6dc9c5 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -269,7 +269,7 @@ class Driver { } /** - * Call protocol methods. + * Call protocol methods, with a timeout. * To configure the timeout for the next call, use 'setNextProtocolTimeout'. * @template {keyof LH.CrdpCommands} C * @param {C} method @@ -279,35 +279,41 @@ class Driver { sendCommand(method, ...params) { const timeout = this._nextProtocolTimeout; this._nextProtocolTimeout = DEFAULT_PROTOCOL_TIMEOUT; - const domainCommand = /^(\w+)\.(enable|disable)$/.exec(method); - if (domainCommand) { - const enable = domainCommand[2] === 'enable'; - if (!this._shouldToggleDomain(domainCommand[1], enable)) { - return Promise.resolve(); - } - } return new Promise(async (resolve, reject) => { - let asyncTimeout; - if (timeout > 0) { - asyncTimeout = setTimeout((() => { - const err = new LHError(LHError.errors.PROTOCOL_TIMEOUT); - err.message += ` Method: ${method}`; - reject(err); - }), timeout); - } + const asyncTimeout = setTimeout((() => { + const err = new LHError(LHError.errors.PROTOCOL_TIMEOUT); + err.message += ` Method: ${method}`; + reject(err); + }), timeout); try { - const result = await this._connection.sendCommand(method, ...params); + const result = await this.innerSendCommand(method, ...params); resolve(result); } catch (err) { reject(err); } finally { - if (asyncTimeout) { - clearTimeout(asyncTimeout); - } + clearTimeout(asyncTimeout); } }); } + /** + * Call protocol methods. + * @template {keyof LH.CrdpCommands} C + * @param {C} method + * @param {LH.CrdpCommands[C]['paramsType']} params + * @return {Promise} + */ + innerSendCommand(method, ...params) { + const domainCommand = /^(\w+)\.(enable|disable)$/.exec(method); + if (domainCommand) { + const enable = domainCommand[2] === 'enable'; + if (!this._shouldToggleDomain(domainCommand[1], enable)) { + return Promise.resolve(); + } + } + return this._connection.sendCommand(method, ...params); + } + /** * Returns whether a domain is currently enabled. * @param {string} domain @@ -846,8 +852,8 @@ class Driver { // happen _after_ onload: https://crbug.com/768961 this.sendCommand('Page.enable'); this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS}); - this.setNextProtocolTimeout(0); // We don't need a timeout for Page.navigate. See #6413. - this.sendCommand('Page.navigate', {url}); + // No timeout needed for Page.navigate. See #6413. + this.innerSendCommand('Page.navigate', {url}); if (waitForLoad) { const passConfig = /** @type {Partial} */ (passContext.passConfig || {}); From 038328516939fff416f68596f97a0891bdf4c587 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Mon, 19 Nov 2018 15:58:36 -0800 Subject: [PATCH 08/10] update comment --- lighthouse-core/gather/driver.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 91b0db6dc9c5..ab0d093fe5b0 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -259,7 +259,7 @@ class Driver { } /** - * timeout is used for the next call to 'sendCommand'. If 0, no timeout is used. + * timeout is used for the next call to 'sendCommand'. * NOTE: This can eventually be replaced when TypeScript * resolves https://github.com/Microsoft/TypeScript/issues/5453. * @param {number} timeout From 6f88aaf37d686337ee6dc0d07534751d066b1a69 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Mon, 19 Nov 2018 17:06:27 -0800 Subject: [PATCH 09/10] add tests --- lighthouse-core/gather/driver.js | 7 ++++--- lighthouse-core/test/gather/driver-test.js | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js index 1613ddbbe6b7..c49e512218f3 100644 --- a/lighthouse-core/gather/driver.js +++ b/lighthouse-core/gather/driver.js @@ -286,7 +286,7 @@ class Driver { reject(err); }), timeout); try { - const result = await this.innerSendCommand(method, ...params); + const result = await this._innerSendCommand(method, ...params); resolve(result); } catch (err) { reject(err); @@ -298,12 +298,13 @@ class Driver { /** * Call protocol methods. + * @private * @template {keyof LH.CrdpCommands} C * @param {C} method * @param {LH.CrdpCommands[C]['paramsType']} params * @return {Promise} */ - innerSendCommand(method, ...params) { + _innerSendCommand(method, ...params) { const domainCommand = /^(\w+)\.(enable|disable)$/.exec(method); if (domainCommand) { const enable = domainCommand[2] === 'enable'; @@ -895,7 +896,7 @@ class Driver { this.sendCommand('Page.enable'); this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS}); // No timeout needed for Page.navigate. See #6413. - this.innerSendCommand('Page.navigate', {url}); + this._innerSendCommand('Page.navigate', {url}); if (waitForNavigated) { await this._waitForFrameNavigated(); diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 419ddd76005f..0f588b7671f6 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -240,6 +240,24 @@ describe('Browser Driver', () => { }); }); + it('.sendCommand timesout when commands take too long', async () => { + const driver = new Driver(connection); + driver._innerSendCommand = () => { + return new Promise(resolve => setTimeout(resolve, 15)); + }; + + driver.setNextProtocolTimeout(25); + await driver.sendCommand('Page.enable'); + + driver.setNextProtocolTimeout(5); + try { + await driver.sendCommand('Page.disable'); + assert.fail('expected driver.sendCommand to timeout'); + } catch (err) { + // :) + } + }); + it('will request default traceCategories', () => { return driverStub.beginTrace().then(() => { const traceCmd = sendCommandParams.find(obj => obj.command === 'Tracing.start'); From c08cb46947c62fdb702bfec98d8da4684a2ff685 Mon Sep 17 00:00:00 2001 From: Connor Clark Date: Mon, 19 Nov 2018 18:35:43 -0800 Subject: [PATCH 10/10] update test --- lighthouse-core/test/gather/driver-test.js | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/test/gather/driver-test.js b/lighthouse-core/test/gather/driver-test.js index 0f588b7671f6..a68266a4bfcf 100644 --- a/lighthouse-core/test/gather/driver-test.js +++ b/lighthouse-core/test/gather/driver-test.js @@ -241,11 +241,19 @@ describe('Browser Driver', () => { }); it('.sendCommand timesout when commands take too long', async () => { - const driver = new Driver(connection); - driver._innerSendCommand = () => { - return new Promise(resolve => setTimeout(resolve, 15)); - }; - + class SlowConnection extends EventEmitter { + connect() { + return Promise.resolve(); + } + disconnect() { + return Promise.resolve(); + } + sendCommand() { + return new Promise(resolve => setTimeout(resolve, 15)); + } + } + const slowConnection = new SlowConnection(); + const driver = new Driver(slowConnection); driver.setNextProtocolTimeout(25); await driver.sendCommand('Page.enable'); @@ -254,7 +262,7 @@ describe('Browser Driver', () => { await driver.sendCommand('Page.disable'); assert.fail('expected driver.sendCommand to timeout'); } catch (err) { - // :) + assert.equal(err.code, 'PROTOCOL_TIMEOUT'); } });