Bug 154817

Summary: [JSC] Private symbols should not be trapped by proxy handler
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, keith_miller, mark.lam, msaboff, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch mark.lam: review+

Description Yusuke Suzuki 2016-02-29 08:43:02 PST
Since the runtime has some assumptions on the properties bound with private symbols, ES6 Proxy should not trap these property operations.
For example, in ArrayIteratorPrototype.js

var itemKind = this.@arrayIterationKind;
if (itemKind === @undefined)
    throw new @TypeError("%ArrayIteratorPrototype%.next requires that |this| be an Array Iterator instance");

Here, we assume that only the array iterator has @arrayIterationKind property that value is non-undefined.
But If we implement Proxy with the get handler, that returns non-undefined value for every operations, we accidentally assumes that the given value is an array iterator.

To avoid these situation, we perform the default operations onto property ops with private symbols.
Comment 1 Yusuke Suzuki 2016-02-29 09:28:58 PST
Created attachment 272494 [details]
Patch
Comment 2 Yusuke Suzuki 2016-02-29 09:30:53 PST
Created attachment 272495 [details]
Patch
Comment 3 Mark Lam 2016-02-29 09:50:45 PST
Comment on attachment 272495 [details]
Patch

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

r=me

> Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js:25
> +            // this will throw because we conver private symbols to strings.

typo: conver ==> convert

> Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js:51
> +            // this will throw because we conver private symbols to strings.

Ditto.  Typo: conver ==> convert.

> Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js:65
> +        set: function(theTarget, propName, value, reciever) {

typo: reciever => receiver.

> Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js:78
> +            // this will throw because we conver private symbols to strings.

Ditto.
Comment 4 Saam Barati 2016-02-29 09:57:27 PST
Comment on attachment 272495 [details]
Patch

We should probably remove the identifierToPublicSafeJSValur
Comment 5 Yusuke Suzuki 2016-02-29 10:04:48 PST
(In reply to comment #4)
> Comment on attachment 272495 [details]
> Patch
> 
> We should probably remove the identifierToPublicSafeJSValur

OK, I'll remove this :)
Comment 6 Yusuke Suzuki 2016-02-29 10:05:33 PST
Comment on attachment 272495 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js:25
>> +            // this will throw because we conver private symbols to strings.
> 
> typo: conver ==> convert

Fixed.

>> Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js:51
>> +            // this will throw because we conver private symbols to strings.
> 
> Ditto.  Typo: conver ==> convert.

Fixed.

>> Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js:65
>> +        set: function(theTarget, propName, value, reciever) {
> 
> typo: reciever => receiver.

Fixed, thanks!

>> Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js:78
>> +            // this will throw because we conver private symbols to strings.
> 
> Ditto.

Fixed.
Comment 7 Saam Barati 2016-02-29 10:09:37 PST
Comment on attachment 272495 [details]
Patch

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

>> Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js:25
>> +            // this will throw because we conver private symbols to strings.
> 
> typo: conver ==> convert

It's probably worth asserting on the error message here
Comment 8 Yusuke Suzuki 2016-02-29 19:44:50 PST
Comment on attachment 272495 [details]
Patch

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

Thanks!

>>>> Source/JavaScriptCore/tests/stress/proxy-with-private-symbols.js:25
>>>> +            // this will throw because we conver private symbols to strings.
>>> 
>>> typo: conver ==> convert
>> 
>> Fixed.
> 
> It's probably worth asserting on the error message here

Nice! Added.
Comment 9 Yusuke Suzuki 2016-02-29 19:48:33 PST
Committed r197383: <http://trac.webkit.org/changeset/197383>