From 90df69cf77e88cc4a057cb2ec04b6aa973f568c7 Mon Sep 17 00:00:00 2001
From: Andrey Lushnikov <lushnikov@chromium.org>
Date: Fri, 7 Jun 2019 13:46:43 -0700
Subject: [PATCH] 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
---
 lib/ExecutionContext.js | 26 +++++++++++++++++---------
 test/evaluation.spec.js |  8 ++++++++
 test/page.spec.js       |  9 +++++++++
 3 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/lib/ExecutionContext.js b/lib/ExecutionContext.js
index 9d3c14b1..b1b1fcba 100644
--- a/lib/ExecutionContext.js
+++ b/lib/ExecutionContext.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
diff --git a/test/evaluation.spec.js b/test/evaluation.spec.js
index 58566d22..b4a9daf3 100644
--- a/test/evaluation.spec.js
+++ b/test/evaluation.spec.js
@@ -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() {
diff --git a/test/page.spec.js b/test/page.spec.js
index bc2a065d..f7428cdf 100644
--- a/test/page.spec.js
+++ b/test/page.spec.js
@@ -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());