Skip to content

Commit

Permalink
fix(eval): adopt nested handles (#1430)
Browse files Browse the repository at this point in the history 
We were only adopting top-level handles in FrameExecutionContext. Now we do that universally.
  • Loading branch information
@dgozman
dgozman committed Mar 19, 2020
1 parent f5ecbff commit ea99908
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 78 deletions.
36 changes: 20 additions & 16 deletions src/chromium/crExecutionContext.ts
View file
Expand Up @@ -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))
Expand All @@ -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'))
Expand Down
37 changes: 9 additions & 28 deletions src/dom.ts
View file
Expand Up @@ -45,35 +45,16 @@ export class FrameExecutionContext extends js.ExecutionContext {
this.frame = frame;
}

async _evaluate(returnByValue: boolean, waitForNavigations: boolean, pageFunction: string | Function, ...args: any[]): Promise<any> {
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' });
}
_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;
}

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;
async _evaluate(returnByValue: boolean, waitForNavigations: boolean, pageFunction: string | Function, ...args: any[]): Promise<any> {
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 {
Expand Down
32 changes: 18 additions & 14 deletions src/firefox/ffExecutionContext.ts
View file
Expand Up @@ -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))
Expand All @@ -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'))
Expand Down
41 changes: 32 additions & 9 deletions src/javascript.ts
View file
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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 {
Expand All @@ -129,8 +133,9 @@ export function prepareFunctionCall<T>(
}

const guids: string[] = [];
const handles: T[] = [];
const pushHandle = (handle: T): string => {
const handles: (Promise<JSHandle | T>)[] = [];
const toDispose: Promise<JSHandle>[] = [];
const pushHandle = (handle: Promise<JSHandle | T>): string => {
const guid = platform.guid();
guids.push(guid);
handles.push(handle);
Expand Down Expand Up @@ -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;
};

Expand All @@ -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})(...(() => {
Expand All @@ -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 };
}
26 changes: 15 additions & 11 deletions src/webkit/wkExecutionContext.ts
View file
Expand Up @@ -87,29 +87,33 @@ 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)) {
const remoteObject = toRemoteObject(value);
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[] } {
Expand Down

0 comments on commit ea99908

Please sign in to comment.