fix(webkit): do not swallow errors when returning by value #2723
Conversation
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.
| @@ -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}) => { | |||
There was a problem hiding this comment.
Can you also add a test for the case where it previously would return undefined and now throws?
There was a problem hiding this comment.
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).
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.