Bug 154877 – [ES6] Add support for Symbol.toPrimitive

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

[ES6] Add support for Symbol.toPrimitive

[Keith Miller]

Created attachment 272593 [details]
Patch

[Keith Miller]

Created attachment 272599 [details]
Patch

[Keith Miller]

Created attachment 272600 [details]
Patch

[Keith Miller]

Created attachment 272602 [details]
Benchmark results

Results of note:

   3d-raytrace                                      588.7549+-2.7082     !    599.1740+-2.7997        ! definitely 1.0177x slower
   string-base64                                    316.4520+-1.8219     !    320.9670+-2.1476        ! definitely 1.0143x slower
   ArrayBuffer-DataView-alloc-long-lived             12.1300+-0.3915     !     13.3037+-0.5720        ! definitely 1.0968x slower
   ftl-polymorphic-bitand                           109.1163+-5.1286     !    132.0995+-0.7221        ! definitely 1.2106x slower
   ftl-polymorphic-bitor                            107.0320+-2.9510     !    132.0258+-1.4583        ! definitely 1.2335x slower
   ftl-polymorphic-bitxor                           106.2510+-0.2048     !    131.9367+-0.9145        ! definitely 1.2417x slower
   ftl-polymorphic-div                              413.1315+-0.4384     !    442.6412+-4.5124        ! definitely 1.0714x slower
   ftl-polymorphic-lshift                           134.8433+-3.7186     !    158.9005+-1.3790        ! definitely 1.1784x slower
   ftl-polymorphic-mul                              203.1172+-0.3581     !    230.5443+-4.6064        ! definitely 1.1350x slower
   ftl-polymorphic-rshift                           134.6206+-1.3621     !    159.2634+-1.3534        ! definitely 1.1831x slower
   ftl-polymorphic-StringFromCharCode                39.4506+-0.5848     !     40.7247+-0.3355        ! definitely 1.0323x slower
   ftl-polymorphic-sub                              153.3777+-0.1528     !    179.1507+-0.6174        ! definitely 1.1680x slower
   ftl-polymorphic-urshift                          145.3800+-0.4638     !    170.9575+-1.9772        ! definitely 1.1759x slower

ArrayBuffer-DataView-alloc-long-lived is just noise. The other benchmarks are relevant but we are going to defer for now since we don't have the current tooling to make an easy fix for them yet. I have created a bugzilla, however, https://bugs.webkit.org/show_bug.cgi?id=154887

[Build Bot]

Comment on attachment 272600 [details]
Patch

Attachment 272600 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/908255

New failing tests:
http/tests/security/cross-frame-access-location-get.html
http/tests/security/cross-frame-access-object-setPrototypeOf.html
http/tests/security/cross-frame-access-custom.html

[Build Bot]

Created attachment 272606 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 272600 [details]
Patch

Attachment 272600 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/908257

New failing tests:
http/tests/security/cross-frame-access-object-setPrototypeOf.html
http/tests/security/cross-frame-access-custom.html

[Build Bot]

Created attachment 272608 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Keith Miller]

I think the EWS failures are just a rebaseling of the tests now that we also attempt to lookup Symbol.toPrimitive. I'll double check before landing though.

[Build Bot]

Comment on attachment 272600 [details]
Patch

Attachment 272600 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/908289

New failing tests:
http/tests/security/cross-frame-access-location-get.html
http/tests/security/cross-frame-access-object-setPrototypeOf.html
http/tests/security/cross-frame-access-custom.html

[Build Bot]

Created attachment 272610 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Saam Barati]

Comment on attachment 272600 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:356
> +    vm, globalObject, headStructure, nullptr,

style: this should all be indented.

> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:359
> +        generateCondition(vm, nullptr, object, uid, PropertyCondition::Absence);

this can go on one line

> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:365
> +}

If you feel inclined, this and the function below only have one thing that are different.
It would be easy to create a common function for both that is templatized or takes an argument

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:631
> +inline PreferredPrimitiveType toPreferredPrimitiveType(ExecState* exec, JSValue value)

Style: indentation is off

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:638
> +        StringImpl* hintString = jsCast<JSString*>(value)->value(exec).impl();

This can throw.

> Source/JavaScriptCore/runtime/JSObject.cpp:1402
> +enum ThrowMode { TakesHint, DoesNotTakeHint };

Why is this called ThrowMode?
That name doesn't make sense to me.
I'd also make this an enum class

> Source/JavaScriptCore/runtime/JSObject.cpp:1443
>  // ECMA 8.6.2.6

It's worth updating this to section 7.1.1 of ES6.

> Source/JavaScriptCore/tests/stress/ropes-symbol-toprimitive.js:13
> +String.prototype[Symbol.toPrimitive] = function() { return "changed"; }

It's worth adding a test that compiles through the DFG and starts with String.prototype having this property
and ensuring we prevent the optimizations that we check using canOptimizeStringObjectAccess

> Source/JavaScriptCore/tests/stress/symbol-toprimitive.js:14
> +let foo = { }
> +foo[Symbol.toPrimitive] = function() { return {} };
> +
> +failed = true;
> +try {
> +    foo >= 1;
> +} catch (e) {
> +    if (e instanceof TypeError)
> +        failed = false;
> +}
> +
> +if (failed)
> +    throw "should have thrown on return of object";

it's probably worth adding a test involving snippets and the JITs

[Keith Miller]

Comment on attachment 272600 [details]
Patch

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

>> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:356
>> +    vm, globalObject, headStructure, nullptr,
> 
> style: this should all be indented.

Fixed.

>> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:359
>> +        generateCondition(vm, nullptr, object, uid, PropertyCondition::Absence);
> 
> this can go on one line

Fixed.

>> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:365
>> +}
> 
> If you feel inclined, this and the function below only have one thing that are different.
> It would be easy to create a common function for both that is templatized or takes an argument

I think it's probably fine, I think it would just end up making the code harder to read.

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:631
>> +inline PreferredPrimitiveType toPreferredPrimitiveType(ExecState* exec, JSValue value)
> 
> Style: indentation is off

Fixed.

>> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:638
>> +        StringImpl* hintString = jsCast<JSString*>(value)->value(exec).impl();
> 
> This can throw.

Good catch, thanks.

>> Source/JavaScriptCore/runtime/JSObject.cpp:1402
>> +enum ThrowMode { TakesHint, DoesNotTakeHint };
> 
> Why is this called ThrowMode?
> That name doesn't make sense to me.
> I'd also make this an enum class

Yeah, that should be TypeHintMode. switched to class.

>> Source/JavaScriptCore/runtime/JSObject.cpp:1443
>>  // ECMA 8.6.2.6
> 
> It's worth updating this to section 7.1.1 of ES6.

Fixed.

>> Source/JavaScriptCore/tests/stress/ropes-symbol-toprimitive.js:13
>> +String.prototype[Symbol.toPrimitive] = function() { return "changed"; }
> 
> It's worth adding a test that compiles through the DFG and starts with String.prototype having this property
> and ensuring we prevent the optimizations that we check using canOptimizeStringObjectAccess

Added.

>> Source/JavaScriptCore/tests/stress/symbol-toprimitive.js:14
>> +    throw "should have thrown on return of object";
> 
> it's probably worth adding a test involving snippets and the JITs

Snippets always call the slow path when passed an object and a primitive so that won't test anything yet. However, I put this function in a loop for good measure.

[Keith Miller]

Created attachment 272783 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 272783 [details]
Patch for landing

Clearing flags on attachment: 272783

Committed r197531: <http://trac.webkit.org/changeset/197531>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

*** Bug 150462 has been marked as a duplicate of this bug. ***