Function object should convert params to string before throw a parsing error

[Sotero Jr]

Hi everyone,

I found out an inconsistency in JSC when we declare a Function Object with a special characters as params.

Version: 235121
O.S: MacOS High Sierra Version 10.13.6

step to reproduce:
assertThrowsValue(() => Function("@", { toString() { throw 42; } }), 42);

atual result: 
SyntaxError: Invalid character: '@'

expected result:
pass without failures

V8, Chakra and SpiderMonkey works as expected 

cinfuzz

[Yusuke Suzuki]

Created attachment 348075 [details]
Patch

[Darin Adler]

Comment on attachment 348075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348075&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        ToString operation onto the `body` of the Function constructor should be performed
> +        before checking syntax correctness of the parameters.

I have no doubt that this is better, but I wonder where these semantics are defined and tested unambiguously. There must be lots of other cases where the order of evaluation side effect is visible, and the new test covers just this one.

[Saam Barati]

(In reply to Darin Adler from comment #2)
> Comment on attachment 348075 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348075&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        ToString operation onto the `body` of the Function constructor should be performed
> > +        before checking syntax correctness of the parameters.
> 
> I have no doubt that this is better, but I wonder where these semantics are
> defined and tested unambiguously. There must be lots of other cases where
> the order of evaluation side effect is visible, and the new test covers just
> this one.

It's defined here:
https://tc39.github.io/ecma262/#sec-createdynamicfunction

I agree that it would be nice to add some more tests that stress the order of evaluation, including the order of toString on the parameters.

[Yusuke Suzuki]

Comment on attachment 348075 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348075&action=review

Thanks!

>>> Source/JavaScriptCore/ChangeLog:9
>>> +        before checking syntax correctness of the parameters.
>> 
>> I have no doubt that this is better, but I wonder where these semantics are defined and tested unambiguously. There must be lots of other cases where the order of evaluation side effect is visible, and the new test covers just this one.
> 
> It's defined here:
> https://tc39.github.io/ecma262/#sec-createdynamicfunction
> 
> I agree that it would be nice to add some more tests that stress the order of evaluation, including the order of toString on the parameters.

I've added the test checking the order of toString onto the arguments of Function constructor.

[Yusuke Suzuki]

Committed r235582: <https://trac.webkit.org/changeset/235582>

[Radar WebKit Bug Importer]

<rdar://problem/43989360>