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

core: remove protocol timeout for Page.navigate #6413

Merged
merged 11 commits into from
Nov 20, 2018
41 changes: 27 additions & 14 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ class Driver {
}

/**
* 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
Expand All @@ -268,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
Expand All @@ -278,30 +279,42 @@ 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) => {
const asyncTimeout = setTimeout((_ => {
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);
clearTimeout(asyncTimeout);
const result = await this._innerSendCommand(method, ...params);
resolve(result);
} catch (err) {
clearTimeout(asyncTimeout);
reject(err);
} finally {
clearTimeout(asyncTimeout);
}
});
}

/**
* Call protocol methods.
* @private
* @template {keyof LH.CrdpCommands} C
* @param {C} method
* @param {LH.CrdpCommands[C]['paramsType']} params
* @return {Promise<LH.CrdpCommands[C]['returnType']>}
*/
_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
Expand Down Expand Up @@ -882,8 +895,8 @@ class Driver {
// happen _after_ onload: https://crbug.com/768961
this.sendCommand('Page.enable');
this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS});
this.setNextProtocolTimeout(30 * 1000);
this.sendCommand('Page.navigate', {url});
// No timeout needed for Page.navigate. See #6413.
this._innerSendCommand('Page.navigate', {url});

if (waitForNavigated) {
await this._waitForFrameNavigated();
Expand Down
26 changes: 26 additions & 0 deletions lighthouse-core/test/gather/driver-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,32 @@ describe('Browser Driver', () => {
});
});

it('.sendCommand timesout when commands take too long', async () => {
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');

driver.setNextProtocolTimeout(5);
try {
await driver.sendCommand('Page.disable');
assert.fail('expected driver.sendCommand to timeout');
} catch (err) {
assert.equal(err.code, 'PROTOCOL_TIMEOUT');
}
});

it('will request default traceCategories', () => {
return driverStub.beginTrace().then(() => {
const traceCmd = sendCommandParams.find(obj => obj.command === 'Tracing.start');
Expand Down