From 064a0a115460d3e54d34bb8b87ad321bdaf426ff Mon Sep 17 00:00:00 2001
From: Dmitry Gozman <dgozman@gmail.com>
Date: Fri, 26 Jun 2020 09:50:57 -0700
Subject: [PATCH] fix(webkit): do not swallow errors when returning by value
 (#2723)

We currently return undefined whenever we had an error trying
return the evaluation result by error. The most common error
is "execution context destroyed".

This produces very unexpected undefined from methods that do not
ever expect undefined. Instead, we should throw because we were
not able to return the result.
---
 src/common/utilityScriptSerializers.ts |  3 +--
 src/injected/utilityScript.ts          |  3 +++
 src/webkit/wkExecutionContext.ts       | 15 ++++++++-------
 test/evaluation.spec.js                |  4 ++++
 4 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/src/common/utilityScriptSerializers.ts b/src/common/utilityScriptSerializers.ts
index 8eec2fc7d6..e35a1a11d0 100644
--- a/src/common/utilityScriptSerializers.ts
+++ b/src/common/utilityScriptSerializers.ts
@@ -15,8 +15,7 @@
  */
 
 export function parseEvaluationResultValue(value: any, handles: any[] = []): any {
-  // { type: 'undefined' } does not even have value.
-  if (value === 'undefined')
+  if (value === undefined)
     return undefined;
   if (typeof value === 'object') {
     if (value.v === 'undefined')
diff --git a/src/injected/utilityScript.ts b/src/injected/utilityScript.ts
index d48c6f1610..c1cc401130 100644
--- a/src/injected/utilityScript.ts
+++ b/src/injected/utilityScript.ts
@@ -32,6 +32,9 @@ export default class UtilityScript {
   }
 
   jsonValue(returnByValue: true, value: any) {
+    // Special handling of undefined to work-around multi-step returnByValue handling in WebKit.
+    if (Object.is(value, undefined))
+      return undefined;
     return serializeAsCallArgument(value, (value: any) => ({ fallThrough: value }));
   }
 
diff --git a/src/webkit/wkExecutionContext.ts b/src/webkit/wkExecutionContext.ts
index 10c6b018c4..0ba8733e02 100644
--- a/src/webkit/wkExecutionContext.ts
+++ b/src/webkit/wkExecutionContext.ts
@@ -80,8 +80,13 @@ export class WKExecutionContext implements js.ExecutionContextDelegate {
         throw new Error('Evaluation failed: ' + response.result.description);
       if (!returnByValue)
         return utilityScript._context.createHandle(response.result);
-      if (response.result.objectId)
+      if (response.result.objectId) {
+        // Avoid protocol round trip for evaluates that do not return anything.
+        // Otherwise, we can fail with 'execution context destroyed' without any reason.
+        if (response.result.type === 'undefined')
+          return undefined;
         return await this._returnObjectByValue(utilityScript, response.result.objectId);
+      }
       return parseEvaluationResultValue(response.result.value);
     } catch (error) {
       throw rewriteError(error);
@@ -89,7 +94,6 @@ export class WKExecutionContext implements js.ExecutionContextDelegate {
   }
 
   private async _returnObjectByValue(utilityScript: js.JSHandle<any>, objectId: Protocol.Runtime.RemoteObjectId): Promise<any> {
-    // This is different from handleJSONValue in that it does not throw.
     try {
       const serializeResponse = await this._session.send('Runtime.callFunctionOn', {
         functionDeclaration: 'object => object' + sourceMap.generateSourceUrl(),
@@ -98,13 +102,10 @@ export class WKExecutionContext implements js.ExecutionContextDelegate {
         returnByValue: true
       });
       if (serializeResponse.wasThrown)
-        return undefined;
+        throw new Error('Evaluation failed: ' + serializeResponse.result.description);
       return parseEvaluationResultValue(serializeResponse.result.value);
     } catch (error) {
-      return undefined;
-      // TODO: we should actually throw an error, but that breaks the common case of undefined
-      // that is for some reason reported as an object and cannot be accessed after navigation.
-      // throw rewriteError(error);
+      throw rewriteError(error);
     }
   }
 
diff --git a/test/evaluation.spec.js b/test/evaluation.spec.js
index bcc2f94961..8924585d87 100644
--- a/test/evaluation.spec.js
+++ b/test/evaluation.spec.js
@@ -228,6 +228,10 @@ describe('Page.evaluate', function() {
   it('should properly serialize undefined fields', async({page}) => {
     expect(await page.evaluate(() => ({a: undefined}))).toEqual({});
   });
+  it('should return undefined properties', async({page}) => {
+    const value = await page.evaluate(() => ({a: undefined}));
+    expect('a' in value).toBe(true);
+  });
   it('should properly serialize null arguments', async({page}) => {
     expect(await page.evaluate(x => x, null)).toEqual(null);
   });