Skip to content
Permalink
Browse files

feat: nicer protocol error messages (#2742)

This patch:
- stops appending `undefined` to our protocol messages unnecessarily.
- rewrites `Cannot find execution context id` to `Execution context was destroyed, most likely because of a navigation.` when it occurs from a Puppeteer ExecutionContext. The error message is left alone if it occurs via a CDPSession.
  • Loading branch information...
JoelEinbinder authored and aslushnikov committed Jun 14, 2018
1 parent 9a650c8 commit 73f9c48081017c228ec1f1318c3c7946321e0538
Showing with 41 additions and 7 deletions.
  1. +16 −2 lib/Connection.js
  2. +13 −3 lib/ExecutionContext.js
  3. +2 −2 test/cookies.spec.js
  4. +10 −0 test/page.spec.js
@@ -108,7 +108,7 @@ class Connection extends EventEmitter {
if (callback) {
this._callbacks.delete(object.id);
if (object.error)
callback.reject(rewriteError(callback.error, `Protocol error (${callback.method}): ${object.error.message} ${object.error.data}`));
callback.reject(createProtocolError(callback.error, callback.method, object));
else
callback.resolve(object.result);
}
@@ -213,7 +213,7 @@ class CDPSession extends EventEmitter {
const callback = this._callbacks.get(object.id);
this._callbacks.delete(object.id);
if (object.error)
callback.reject(rewriteError(callback.error, `Protocol error (${callback.method}): ${object.error.message} ${object.error.data}`));
callback.reject(createProtocolError(callback.error, callback.method, object));
else
callback.resolve(object.result);
} else {
@@ -256,6 +256,20 @@ class CDPSession extends EventEmitter {
}
helper.tracePublicAPI(CDPSession);

/**
* @param {!Error} error
* @param {string} method
* @param {{error: {message: string, data: any}}} object
* @return {!Error}
*/
function createProtocolError(error, method, object) {
let message = `Protocol error (${method}): ${object.error.message}`;
if ('data' in object.error)
message += ` ${object.error.data}`;
if (object.error.message)
return rewriteError(error, message);
}

/**
* @param {!Error} error
* @param {string} message
@@ -64,13 +64,13 @@ class ExecutionContext {
if (helper.isString(pageFunction)) {
const contextId = this._contextId;
const expression = /** @type {string} */ (pageFunction);
const { exceptionDetails, result: remoteObject } = await this._client.send('Runtime.evaluate', {
const {exceptionDetails, result: remoteObject} = await this._client.send('Runtime.evaluate', {
expression,
contextId,
returnByValue: false,
awaitPromise: true,
userGesture: true
});
}).catch(rewriteError);
if (exceptionDetails)
throw new Error('Evaluation failed: ' + helper.getExceptionMessage(exceptionDetails));
return this._objectHandleFactory(remoteObject);
@@ -83,7 +83,7 @@ class ExecutionContext {
returnByValue: false,
awaitPromise: true,
userGesture: true
});
}).catch(rewriteError);
if (exceptionDetails)
throw new Error('Evaluation failed: ' + helper.getExceptionMessage(exceptionDetails));
return this._objectHandleFactory(remoteObject);
@@ -116,6 +116,16 @@ class ExecutionContext {
}
return { value: arg };
}

/**
* @param {!Error} error
* @return {!Protocol.Runtime.evaluateReturnValue}
*/
function rewriteError(error) {
if (error.message.endsWith('Cannot find context with specified id'))
throw new Error('Execution context was destroyed, most likely because of a navigation.');
throw error;
}
}

/**
@@ -124,7 +124,7 @@ module.exports.addTests = function({testRunner, expect}) {
error = e;
}
expect(error).toBeTruthy();
expect(error.message).toEqual('Protocol error (Network.deleteCookies): At least one of the url and domain needs to be specified undefined');
expect(error.message).toEqual('Protocol error (Network.deleteCookies): At least one of the url and domain needs to be specified');
});

it('should not set a cookie with blank page URL', async function({page, server}) {
@@ -154,7 +154,7 @@ module.exports.addTests = function({testRunner, expect}) {
}
expect(error).toBeTruthy();
expect(error.message).toEqual(
'Protocol error (Network.deleteCookies): At least one of the url and domain needs to be specified undefined'
'Protocol error (Network.deleteCookies): At least one of the url and domain needs to be specified'
);
});

@@ -226,6 +226,16 @@ module.exports.addTests = function({testRunner, expect, puppeteer, DeviceDescrip
return audio.play();
}
});
it('should throw a nice error after a navigation', async({page, server}) => {
const executionContext = await page.mainFrame().executionContext();

await Promise.all([
page.waitForNavigation(),
executionContext.evaluate(() => window.location.reload())
]);
const error = await executionContext.evaluate(() => null).catch(e => e);
expect(error.message).toContain('navigation');
});
});

describe('Page.setOfflineMode', function() {

0 comments on commit 73f9c48

Please sign in to comment.
You can’t perform that action at this time.