Skip to content
Permalink
Browse files

fix(page): do evaluations with one roundtrip (#4539)

This patch teaches page.evaluate to do 1 hop instead of 2 hops.
As a result, things such as `page.select` will not throw an
unfortunate exception when they schedule a navigation.

Fix #4537
  • Loading branch information...
aslushnikov committed Jun 7, 2019
1 parent 7e19846 commit 90df69cf77e88cc4a057cb2ec04b6aa973f568c7
Showing with 34 additions and 9 deletions.
  1. +17 −9 lib/ExecutionContext.js
  2. +8 −0 test/evaluation.spec.js
  3. +9 −0 test/page.spec.js
@@ -45,16 +45,15 @@ class ExecutionContext {
* @return {!Promise<(!Object|undefined)>}
*/
async evaluate(pageFunction, ...args) {
const handle = await this.evaluateHandle(pageFunction, ...args);
const result = await handle.jsonValue().catch(error => {
try {
return await this._evaluateInternal(true /* returnByValue */, pageFunction, ...args);
} catch (error) {
if (error.message.includes('Object reference chain is too long'))
return;
if (error.message.includes('Object couldn\'t be returned by value'))
return;
throw error;
});
await handle.dispose();
return result;
}
}

/**
@@ -63,6 +62,15 @@ class ExecutionContext {
* @return {!Promise<!JSHandle>}
*/
async evaluateHandle(pageFunction, ...args) {
return this._evaluateInternal(false /* returnByValue */, pageFunction, ...args);
}

/**
* @param {Function|string} pageFunction
* @param {...*} args
* @return {!Promise<!JSHandle>}
*/
async _evaluateInternal(returnByValue, pageFunction, ...args) {
const suffix = `//# sourceURL=${EVALUATION_SCRIPT_URL}`;

if (helper.isString(pageFunction)) {
@@ -72,13 +80,13 @@ class ExecutionContext {
const {exceptionDetails, result: remoteObject} = await this._client.send('Runtime.evaluate', {
expression: expressionWithSourceUrl,
contextId,
returnByValue: false,
returnByValue,
awaitPromise: true,
userGesture: true
}).catch(rewriteError);
if (exceptionDetails)
throw new Error('Evaluation failed: ' + helper.getExceptionMessage(exceptionDetails));
return createJSHandle(this, remoteObject);
return returnByValue ? helper.valueFromRemoteObject(remoteObject) : createJSHandle(this, remoteObject);
}

if (typeof pageFunction !== 'function')
@@ -107,7 +115,7 @@ class ExecutionContext {
functionDeclaration: functionText + '\n' + suffix + '\n',
executionContextId: this._contextId,
arguments: args.map(convertArgument.bind(this)),
returnByValue: false,
returnByValue,
awaitPromise: true,
userGesture: true
});
@@ -119,7 +127,7 @@ class ExecutionContext {
const { exceptionDetails, result: remoteObject } = await callFunctionOnPromise.catch(rewriteError);
if (exceptionDetails)
throw new Error('Evaluation failed: ' + helper.getExceptionMessage(exceptionDetails));
return createJSHandle(this, remoteObject);
return returnByValue ? helper.valueFromRemoteObject(remoteObject) : createJSHandle(this, remoteObject);

/**
* @param {*} arg
@@ -231,6 +231,14 @@ module.exports.addTests = function({testRunner, expect}) {
const error = await executionContext.evaluate(() => null).catch(e => e);
expect(error.message).toContain('navigation');
});
it_fails_ffox('should not throw an error when evaluation does a navigation', async({page, server}) => {
await page.goto(server.PREFIX + '/one-style.html');
const result = await page.evaluate(() => {
window.location = '/empty.html';
return [42];
});
expect(result).toEqual([42]);
});
});

describe('Page.evaluateOnNewDocument', function() {
@@ -1067,6 +1067,15 @@ module.exports.addTests = function({testRunner, expect, headless, puppeteer, CHR
expect(await page.evaluate(() => result.onInput)).toEqual(['blue']);
expect(await page.evaluate(() => result.onChange)).toEqual(['blue']);
});
it_fails_ffox('should not throw when select causes navigation', async({page, server}) => {
await page.goto(server.PREFIX + '/input/select.html');
await page.$eval('select', select => select.addEventListener('input', () => window.location = '/empty.html'));
await Promise.all([
page.select('select', 'blue'),
page.waitForNavigation(),
]);
expect(page.url()).toContain('empty.html');
});
it('should select multiple options', async({page, server}) => {
await page.goto(server.PREFIX + '/input/select.html');
await page.evaluate(() => makeMultiple());

0 comments on commit 90df69c

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