Closed
Bug 1388375
Opened 7 years ago
Closed 7 years ago
ValueToId shouldn't unwrap Symbol objects
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
Details
Attachments
(1 file, 1 obsolete file)
4.30 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•7 years ago
|
||
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.)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
Removing IsSymbolOrSymbolWrapper is also beneficial for performance, because IsSymbolOrSymbolWrapper is never inlined (bug 1370210).
Assignee | ||
Comment 3•7 years ago
|
||
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.
Attachment #8895348 -
Flags: review?(jorendorff)
Comment 4•7 years ago
|
||
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.
Attachment #8895348 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 5•7 years ago
|
||
(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! :-)
Assignee | ||
Comment 6•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c94993d0bd5230f391512f0b30ca2642646896e8
Keywords: checkin-needed
Assignee | ||
Comment 7•7 years ago
|
||
Update reviewer to :jandem. Otherwise no changes to the patch.
Attachment #8895348 -
Attachment is obsolete: true
Attachment #8898790 -
Flags: review+
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2cc6a87b0b Don't unbox symbol objects in ValueToId. r=jandem
Keywords: checkin-needed
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3c2cc6a87b0b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•