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

@@ -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')

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 }));
   }
 

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);
     }
   }
 

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);
   });