Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(webkit): do not swallow errors when returning by value #2723

Merged
merged 1 commit into from
Jun 26, 2020
Merged

fix(webkit): do not swallow errors when returning by value #2723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/common/utilityScriptSerializers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
3 changes: 3 additions & 0 deletions src/injected/utilityScript.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }));
}

Expand Down
15 changes: 8 additions & 7 deletions src/webkit/wkExecutionContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,16 +80,20 @@ 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);
}
}

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(),
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will likely break some of the code where no exception is currently expected. My understanding was that the current contract of returning undefined instead of throwing was intentional since ling ago. I personally agree that it's not good to silently swallow errors but Lusha/Joel might remember if there was a specific request for that behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This current behavior is webkit-specific. It has to do with returnByValue being a separate step from the evaluate itself, due to special promise handling.

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

Expand Down
4 changes: 4 additions & 0 deletions test/evaluation.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a test for the case where it previously would return undefined and now throws?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a test for that called 'should not throw an error when evaluation does a synchronous navigation and returns undefined'. It's racy, so run with repeat(1000).

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