fix(eval): adopt nested handles (#1430)

We were only adopting top-level handles in FrameExecutionContext. Now we do that universally.

src/chromium/crExecutionContext.ts

@@ -55,7 +55,7 @@ export class CRExecutionContext implements js.ExecutionContextDelegate {
     if (typeof pageFunction !== 'function')
       throw new Error(`Expected to get |string| or |function| as the first argument, but got "${pageFunction}" instead.`);
 
-    const { functionText, values, handles } = js.prepareFunctionCall<Protocol.Runtime.CallArgument>(pageFunction, context, args, (value: any) => {
+    const { functionText, values, handles, dispose } = await js.prepareFunctionCall<Protocol.Runtime.CallArgument>(pageFunction, context, args, (value: any) => {
       if (typeof value === 'bigint') // eslint-disable-line valid-typeof
         return { handle: { unserializableValue: `${value.toString()}n` } };
       if (Object.is(value, -0))
@@ -71,26 +71,30 @@ export class CRExecutionContext implements js.ExecutionContextDelegate {
         if (remoteObject.unserializableValue)
           return { handle: { unserializableValue: remoteObject.unserializableValue } };
         if (!remoteObject.objectId)
-          return { value: remoteObject.value };
+          return { handle: { value: remoteObject.value } };
         return { handle: { objectId: remoteObject.objectId } };
       }
       return { value };
     });
 
-    const { exceptionDetails, result: remoteObject } = await this._client.send('Runtime.callFunctionOn', {
+    try {
-      functionDeclaration: functionText + '\n' + suffix + '\n',
+      const { exceptionDetails, result: remoteObject } = await this._client.send('Runtime.callFunctionOn', {
-      executionContextId: this._contextId,
+        functionDeclaration: functionText + '\n' + suffix + '\n',
-      arguments: [
+        executionContextId: this._contextId,
-        ...values.map(value => ({ value })),
+        arguments: [
-        ...handles,
+          ...values.map(value => ({ value })),
-      ],
+          ...handles,
-      returnByValue,
+        ],
-      awaitPromise: true,
+        returnByValue,
-      userGesture: true
+        awaitPromise: true,
-    }).catch(rewriteError);
+        userGesture: true
-    if (exceptionDetails)
+      }).catch(rewriteError);
-      throw new Error('Evaluation failed: ' + getExceptionMessage(exceptionDetails));
+      if (exceptionDetails)
-    return returnByValue ? valueFromRemoteObject(remoteObject) : context._createHandle(remoteObject);
+        throw new Error('Evaluation failed: ' + getExceptionMessage(exceptionDetails));
+      return returnByValue ? valueFromRemoteObject(remoteObject) : context._createHandle(remoteObject);
+    } finally {
+      dispose();
+    }
 
     function rewriteError(error: Error): Protocol.Runtime.evaluateReturnValue {
       if (error.message.includes('Object reference chain is too long'))

src/dom.ts

@@ -45,35 +45,16 @@ export class FrameExecutionContext extends js.ExecutionContext {
     this.frame = frame;
   }
 
+  _adoptIfNeeded(handle: js.JSHandle): Promise<js.JSHandle> | null {
+    if (handle instanceof ElementHandle && handle._context !== this)
+      return this.frame._page._delegate.adoptElementHandle(handle, this);
+    return null;
+  }
+
   async _evaluate(returnByValue: boolean, waitForNavigations: boolean, pageFunction: string | Function, ...args: any[]): Promise<any> {
-    const needsAdoption = (value: any): boolean => {
+    return await this.frame._page._frameManager.waitForNavigationsCreatedBy(async () => {
-      return typeof value === 'object' && value instanceof ElementHandle && value._context !== this;
+      return this._delegate.evaluate(this, returnByValue, pageFunction, ...args);
-    };
+    }, waitForNavigations ? undefined : { waitUntil: 'nowait' });
-
-    if (!args.some(needsAdoption)) {
-      // Only go through asynchronous calls if required.
-      return await this.frame._page._frameManager.waitForNavigationsCreatedBy(async () => {
-        return this._delegate.evaluate(this, returnByValue, pageFunction, ...args);
-      }, waitForNavigations ? undefined : { waitUntil: 'nowait' });
-    }
-
-    const toDispose: Promise<ElementHandle>[] = [];
-    const adopted = await Promise.all(args.map(async arg => {
-      if (!needsAdoption(arg))
-        return arg;
-      const adopted = this.frame._page._delegate.adoptElementHandle(arg, this);
-      toDispose.push(adopted);
-      return adopted;
-    }));
-    let result;
-    try {
-      result = await this.frame._page._frameManager.waitForNavigationsCreatedBy(async () => {
-        return this._delegate.evaluate(this, returnByValue, pageFunction, ...adopted);
-      }, waitForNavigations ? undefined : { waitUntil: 'nowait' });
-    } finally {
-      toDispose.map(handlePromise => handlePromise.then(handle => handle.dispose()));
-    }
-    return result;
   }
 
   _createHandle(remoteObject: any): js.JSHandle {

src/firefox/ffExecutionContext.ts

@@ -44,7 +44,7 @@ export class FFExecutionContext implements js.ExecutionContextDelegate {
     if (typeof pageFunction !== 'function')
       throw new Error(`Expected to get |string| or |function| as the first argument, but got "${pageFunction}" instead.`);
 
-    const { functionText, values, handles } = js.prepareFunctionCall<Protocol.Runtime.CallFunctionArgument>(pageFunction, context, args, (value: any) => {
+    const { functionText, values, handles, dispose } = await js.prepareFunctionCall<Protocol.Runtime.CallFunctionArgument>(pageFunction, context, args, (value: any) => {
       if (Object.is(value, -0))
         return { handle: { unserializableValue: '-0' } };
       if (Object.is(value, Infinity))
@@ -58,19 +58,23 @@ export class FFExecutionContext implements js.ExecutionContextDelegate {
       return { value };
     });
 
-    const payload = await this._session.send('Runtime.callFunction', {
+    try {
-      functionDeclaration: functionText,
+      const payload = await this._session.send('Runtime.callFunction', {
-      args: [
+        functionDeclaration: functionText,
-        ...values.map(value => ({ value })),
+        args: [
-        ...handles,
+          ...values.map(value => ({ value })),
-      ],
+          ...handles,
-      returnByValue,
+        ],
-      executionContextId: this._executionContextId
+        returnByValue,
-    }).catch(rewriteError);
+        executionContextId: this._executionContextId
-    checkException(payload.exceptionDetails);
+      }).catch(rewriteError);
-    if (returnByValue)
+      checkException(payload.exceptionDetails);
-      return deserializeValue(payload.result!);
+      if (returnByValue)
-    return context._createHandle(payload.result);
+        return deserializeValue(payload.result!);
+      return context._createHandle(payload.result);
+    } finally {
+      dispose();
+    }
 
     function rewriteError(error: Error): (Protocol.Runtime.evaluateReturnValue | Protocol.Runtime.callFunctionReturnValue) {
       if (error.message.includes('cyclic object value') || error.message.includes('Object is not serializable'))

src/javascript.ts

@@ -33,6 +33,10 @@ export class ExecutionContext {
     this._delegate = delegate;
   }
 
+  _adoptIfNeeded(handle: JSHandle): Promise<JSHandle> | null {
+    return null;
+  }
+
   _evaluate(returnByValue: boolean, waitForNavigations: boolean, pageFunction: string | Function, ...args: any[]): Promise<any> {
     return this._delegate.evaluate(this, returnByValue, pageFunction, ...args);
   }
@@ -104,11 +108,11 @@ export class JSHandle<T = any> {
   }
 }
 
-export function prepareFunctionCall<T>(
+export async function prepareFunctionCall<T>(
   pageFunction: Function,
   context: ExecutionContext,
   args: any[],
-  toCallArgumentIfNeeded: (value: any) => { handle?: T, value?: any }): { functionText: string, values: any[], handles: T[] } {
+  toCallArgumentIfNeeded: (value: any) => { handle?: T, value?: any }): Promise<{ functionText: string, values: any[], handles: T[], dispose: () => void }> {
 
   let functionText = pageFunction.toString();
   try {
@@ -129,8 +133,9 @@ export function prepareFunctionCall<T>(
   }
 
   const guids: string[] = [];
-  const handles: T[] = [];
+  const handles: (Promise<JSHandle | T>)[] = [];
-  const pushHandle = (handle: T): string => {
+  const toDispose: Promise<JSHandle>[] = [];
+  const pushHandle = (handle: Promise<JSHandle | T>): string => {
     const guid = platform.guid();
     guids.push(guid);
     handles.push(handle);
@@ -165,14 +170,17 @@ export function prepareFunctionCall<T>(
       return result;
     }
     if (arg && (arg instanceof JSHandle)) {
-      if (arg._context !== context)
-        throw new Error('JSHandles can be evaluated only in the context they were created!');
       if (arg._disposed)
         throw new Error('JSHandle is disposed!');
+      const adopted = context._adoptIfNeeded(arg);
+      if (adopted === null)
+        return pushHandle(Promise.resolve(arg));
+      toDispose.push(adopted);
+      return pushHandle(adopted);
     }
     const { handle, value } = toCallArgumentIfNeeded(arg);
     if (handle)
-      return pushHandle(handle);
+      return pushHandle(Promise.resolve(handle));
     return value;
   };
 
@@ -181,7 +189,7 @@ export function prepareFunctionCall<T>(
     throw new Error(error);
 
   if (!guids.length)
-    return { functionText, values: args, handles: [] };
+    return { functionText, values: args, handles: [], dispose: () => {} };
 
   functionText = `(...__playwright__args__) => {
     return (${functionText})(...(() => {
@@ -208,5 +216,20 @@ export function prepareFunctionCall<T>(
     })());
   }`;
 
-  return { functionText, values: [ args.length, ...args, guids.length, ...guids ], handles };
+  const resolved = await Promise.all(handles);
+  const resultHandles: T[] = [];
+  for (let i = 0; i < resolved.length; i++) {
+    const handle = resolved[i];
+    if (handle instanceof JSHandle) {
+      if (handle._context !== context)
+        throw new Error('JSHandles can be evaluated only in the context they were created!');
+      resultHandles.push(toCallArgumentIfNeeded(handle).handle!);
+    } else {
+      resultHandles.push(handle);
+    }
+  }
+  const dispose = () => {
+    toDispose.map(handlePromise => handlePromise.then(handle => handle.dispose()));
+  };
+  return { functionText, values: [ args.length, ...args, guids.length, ...guids ], handles: resultHandles, dispose };
 }

src/webkit/wkExecutionContext.ts

@@ -87,7 +87,7 @@ export class WKExecutionContext implements js.ExecutionContextDelegate {
     if (typeof pageFunction !== 'function')
       throw new Error(`Expected to get |string| or |function| as the first argument, but got "${pageFunction}" instead.`);
 
-    const { functionText, values, handles } = js.prepareFunctionCall<MaybeCallArgument>(pageFunction, context, args, (value: any) => {
+    const { functionText, values, handles, dispose } = await js.prepareFunctionCall<MaybeCallArgument>(pageFunction, context, args, (value: any) => {
       if (typeof value === 'bigint' || Object.is(value, -0) || Object.is(value, Infinity) || Object.is(value, -Infinity) || Object.is(value, NaN))
         return { handle: { unserializable: value } };
       if (value && (value instanceof js.JSHandle)) {
@@ -95,21 +95,25 @@ export class WKExecutionContext implements js.ExecutionContextDelegate {
         if (!remoteObject.objectId && !Object.is(valueFromRemoteObject(remoteObject), remoteObject.value))
           return { handle: { unserializable: value } };
         if (!remoteObject.objectId)
-          return { value: valueFromRemoteObject(remoteObject) };
+          return { handle: { value: valueFromRemoteObject(remoteObject) } };
         return { handle: { objectId: remoteObject.objectId } };
       }
       return { value };
     });
 
-    const callParams = this._serializeFunctionAndArguments(functionText, values, handles);
+    try {
-    const thisObjectId = await this._contextGlobalObjectId();
+      const callParams = this._serializeFunctionAndArguments(functionText, values, handles);
-    return await this._session.send('Runtime.callFunctionOn', {
+      const thisObjectId = await this._contextGlobalObjectId();
-      functionDeclaration: callParams.functionText + '\n' + suffix + '\n',
+      return await this._session.send('Runtime.callFunctionOn', {
-      objectId: thisObjectId,
+        functionDeclaration: callParams.functionText + '\n' + suffix + '\n',
-      arguments: callParams.callArguments,
+        objectId: thisObjectId,
-      returnByValue: false,
+        arguments: callParams.callArguments,
-      emulateUserGesture: true
+        returnByValue: false,
-    });
+        emulateUserGesture: true
+      });
+    } finally {
+      dispose();
+    }
   }
 
   private _serializeFunctionAndArguments(functionText: string, values: any[], handles: MaybeCallArgument[]): { functionText: string, callArguments: Protocol.Runtime.CallArgument[] } {