1388375 - ValueToId shouldn't unwrap Symbol objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

Test case:
---
var s = Symbol();
var so = Object(s);

var o = {};
o[s] = 0;
o["Symbol()"] = 1;
o["foo"] = 2;

Object.defineProperty(o, s, {enumerable: false});
Object.defineProperty(o, "Symbol()", {enumerable: false});
Object.defineProperty(o, "foo", {enumerable: true});

print(o.propertyIsEnumerable(so));

Object.defineProperty(Symbol.prototype, Symbol.toPrimitive, {
    value: function() {
        return "foo";
    }
});

print(o.propertyIsEnumerable(so));
---

Expected: Prints "false" and then "true"
Actual: Prints "false" two times

Comment 1

[André Bargull [:anba]]

Symbol object unwrapping was added in bug 1083047, but unfortunately that bug doesn't contain any info why the reporter thought unboxing is required by the spec. Maybe it was just an misinterpretation of ES6 draft rev28 which was also published on 2014-10-14, the same date bug 1083047 was reported. (ES6 draft rev28 updated multiple sections to properly handle symbol property keys.)

Comment 2

[André Bargull [:anba]]

Removing IsSymbolOrSymbolWrapper is also beneficial for performance, because IsSymbolOrSymbolWrapper is never inlined (bug 1370210).

Comment 3

[André Bargull [:anba]]

Attachment: bug1388375.patch

Unboxing symbol objects in ValueToId isn't spec-compliant, only symbol values should be directly accepted. Symbol objects should go through the normal slow path for all objects. I've also removed js::IsSymbolOrSymbolWrapper and js::ToSymbolPrimitive because these functions are no longer used.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8895348 [details] [diff] [review]
bug1388375.patch

Review of attachment 8895348 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing as this might improve perf a bit. Thanks for fixing.

Comment 5

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> Comment on attachment 8895348 [details] [diff] [review]
> bug1388375.patch
> 
> Review of attachment 8895348 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Stealing as this might improve perf a bit. Thanks for fixing.

Thanks for taking over the review! :-)

Comment 6

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c94993d0bd5230f391512f0b30ca2642646896e8

Comment 7

[André Bargull [:anba]]

Attachment: bug1388375.patch

Update reviewer to :jandem. Otherwise no changes to the patch.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2cc6a87b0b
Don't unbox symbol objects in ValueToId. r=jandem

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3c2cc6a87b0b