Bug 154817 - [JSC] Private symbols should not be trapped by proxy handler
Summary: [JSC] Private symbols should not be trapped by proxy handler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-29 08:43 PST by Yusuke Suzuki
Modified: 2016-02-29 19:48 PST (History)
8 users (show)

See Also:


Attachments
Patch (12.65 KB, patch)
2016-02-29 09:28 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (13.91 KB, patch)
2016-02-29 09:30 PST, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>