From ea99908bf44931de0cb17afbaea4f0f350f75e9d Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Thu, 19 Mar 2020 13:07:33 -0700 Subject: [PATCH] fix(eval): adopt nested handles (#1430) We were only adopting top-level handles in FrameExecutionContext. Now we do that universally. --- src/chromium/crExecutionContext.ts | 36 ++++++++++++++------------ src/dom.ts | 37 +++++++-------------------- src/firefox/ffExecutionContext.ts | 32 +++++++++++++---------- src/javascript.ts | 41 +++++++++++++++++++++++------- src/webkit/wkExecutionContext.ts | 26 +++++++++++-------- 5 files changed, 94 insertions(+), 78 deletions(-) diff --git a/src/chromium/crExecutionContext.ts b/src/chromium/crExecutionContext.ts index d73cbde660..845eac0063 100644 --- a/src/chromium/crExecutionContext.ts +++ b/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(pageFunction, context, args, (value: any) => { + const { functionText, values, handles, dispose } = await js.prepareFunctionCall(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', { - functionDeclaration: functionText + '\n' + suffix + '\n', - executionContextId: this._contextId, - arguments: [ - ...values.map(value => ({ value })), - ...handles, - ], - returnByValue, - awaitPromise: true, - userGesture: true - }).catch(rewriteError); - if (exceptionDetails) - throw new Error('Evaluation failed: ' + getExceptionMessage(exceptionDetails)); - return returnByValue ? valueFromRemoteObject(remoteObject) : context._createHandle(remoteObject); + try { + const { exceptionDetails, result: remoteObject } = await this._client.send('Runtime.callFunctionOn', { + functionDeclaration: functionText + '\n' + suffix + '\n', + executionContextId: this._contextId, + arguments: [ + ...values.map(value => ({ value })), + ...handles, + ], + returnByValue, + awaitPromise: true, + userGesture: true + }).catch(rewriteError); + if (exceptionDetails) + 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')) diff --git a/src/dom.ts b/src/dom.ts index 83164bbef5..a3d9b379b7 100644 --- a/src/dom.ts +++ b/src/dom.ts @@ -45,35 +45,16 @@ export class FrameExecutionContext extends js.ExecutionContext { this.frame = frame; } + _adoptIfNeeded(handle: js.JSHandle): Promise | 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 { - const needsAdoption = (value: any): boolean => { - return typeof value === 'object' && value instanceof ElementHandle && value._context !== this; - }; - - 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[] = []; - 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; + return await this.frame._page._frameManager.waitForNavigationsCreatedBy(async () => { + return this._delegate.evaluate(this, returnByValue, pageFunction, ...args); + }, waitForNavigations ? undefined : { waitUntil: 'nowait' }); } _createHandle(remoteObject: any): js.JSHandle { diff --git a/src/firefox/ffExecutionContext.ts b/src/firefox/ffExecutionContext.ts index 47c650979d..20147ea579 100644 --- a/src/firefox/ffExecutionContext.ts +++ b/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(pageFunction, context, args, (value: any) => { + const { functionText, values, handles, dispose } = await js.prepareFunctionCall(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', { - functionDeclaration: functionText, - args: [ - ...values.map(value => ({ value })), - ...handles, - ], - returnByValue, - executionContextId: this._executionContextId - }).catch(rewriteError); - checkException(payload.exceptionDetails); - if (returnByValue) - return deserializeValue(payload.result!); - return context._createHandle(payload.result); + try { + const payload = await this._session.send('Runtime.callFunction', { + functionDeclaration: functionText, + args: [ + ...values.map(value => ({ value })), + ...handles, + ], + returnByValue, + executionContextId: this._executionContextId + }).catch(rewriteError); + checkException(payload.exceptionDetails); + if (returnByValue) + 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')) diff --git a/src/javascript.ts b/src/javascript.ts index aa9bf4fe8b..92a17bb950 100644 --- a/src/javascript.ts +++ b/src/javascript.ts @@ -33,6 +33,10 @@ export class ExecutionContext { this._delegate = delegate; } + _adoptIfNeeded(handle: JSHandle): Promise | null { + return null; + } + _evaluate(returnByValue: boolean, waitForNavigations: boolean, pageFunction: string | Function, ...args: any[]): Promise { return this._delegate.evaluate(this, returnByValue, pageFunction, ...args); } @@ -104,11 +108,11 @@ export class JSHandle { } } -export function prepareFunctionCall( +export async function prepareFunctionCall( 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( } const guids: string[] = []; - const handles: T[] = []; - const pushHandle = (handle: T): string => { + const handles: (Promise)[] = []; + const toDispose: Promise[] = []; + const pushHandle = (handle: Promise): string => { const guid = platform.guid(); guids.push(guid); handles.push(handle); @@ -165,14 +170,17 @@ export function prepareFunctionCall( 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( 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( })()); }`; - 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 }; } diff --git a/src/webkit/wkExecutionContext.ts b/src/webkit/wkExecutionContext.ts index 107c69febf..45525b974a 100644 --- a/src/webkit/wkExecutionContext.ts +++ b/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(pageFunction, context, args, (value: any) => { + const { functionText, values, handles, dispose } = await js.prepareFunctionCall(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); - const thisObjectId = await this._contextGlobalObjectId(); - return await this._session.send('Runtime.callFunctionOn', { - functionDeclaration: callParams.functionText + '\n' + suffix + '\n', - objectId: thisObjectId, - arguments: callParams.callArguments, - returnByValue: false, - emulateUserGesture: true - }); + try { + const callParams = this._serializeFunctionAndArguments(functionText, values, handles); + const thisObjectId = await this._contextGlobalObjectId(); + return await this._session.send('Runtime.callFunctionOn', { + functionDeclaration: callParams.functionText + '\n' + suffix + '\n', + objectId: thisObjectId, + arguments: callParams.callArguments, + returnByValue: false, + emulateUserGesture: true + }); + } finally { + dispose(); + } } private _serializeFunctionAndArguments(functionText: string, values: any[], handles: MaybeCallArgument[]): { functionText: string, callArguments: Protocol.Runtime.CallArgument[] } {