Closed
Bug 903389
Opened 12 years ago
Closed 9 years ago
valueOf getter not called for ToPrimitive conversion on wrapper types
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: anba, Assigned: evilpies)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
9.03 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
1.32 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
Both test cases should invoke the getter and print "get":
---
""+Object.defineProperty(new String("123"), "valueOf", {get: function(){print("get"); return String.prototype.valueOf}})
+Object.defineProperty(new Number(123), "valueOf", {get: function(){print("get"); return Number.prototype.valueOf}})
---
Expected: custom getter function is executed
Actual: getter function not executed
Comment 1•12 years ago
|
||
Blech. jsobjinlines.h:js::ClassMethodIsNative and jsobj.cpp:js::HasDataProperty are the pieces at fault here. We should probably audit all callers of the latter, because I doubt the current semantics of the method are what anyone expects.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Updated•11 years ago
|
Assignee: general → nobody
Assignee | ||
Updated•9 years ago
|
Assignee: jwalden+bmo → evilpies
Assignee | ||
Comment 3•9 years ago
|
||
I am almost not sure if some of those uses are worth the complexity, especially ToStringForStringFunction.
Attachment #8818527 -
Flags: review?(jwalden+bmo)
Comment 4•9 years ago
|
||
Comment on attachment 8818527 [details] [diff] [review]
Fix uses of ClassMethodIsNative
Review of attachment 8818527 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsobjinlines.h
@@ +581,2 @@
> static MOZ_ALWAYS_INLINE bool
> +HasNativeMethodPure(JSContext* cx, JSObject* obj, PropertyName* name, JSNative native)
Particularly if we're renaming this, let's adhere to the convention of putting the |cx| argument last.
::: js/src/jsstr.cpp
@@ +451,4 @@
> * Perform the initial |RequireObjectCoercible(thisv)| and |ToString(thisv)|
> * from nearly all String.prototype.* functions.
> */
> +MOZ_ALWAYS_INLINE JSString*
Why remove |static| here?
@@ +467,5 @@
> + // would be unobservable.
> + jsid id = SYMBOL_TO_JSID(cx->wellKnownSymbols().toPrimitive);
> + JSObject* pobj;
> + Shape* shape;
> + if (LookupPropertyPure(cx, obj, id, &pobj, &shape) && !shape) {
A |HasNoToPrimitiveMethod| function would be nice for this, especially as another caller needs it.
::: js/src/wasm/AsmJS.cpp
@@ +7480,2 @@
> if (v.toObject().is<JSFunction>() &&
> + HasNativeMethodPure(cx, &v.toObject(), cx->names().valueOf, obj_valueOf) &&
This is pre-existingly missing a @@toPrimitive restriction. Add a HasNoToPrimitiveMethod check? Also add this test that fails with the current implementation:
var counter = 0;
function f(stdlib, foreign)
{
"use asm";
var a = +foreign.a;
var b = +foreign.b;
function g() {}
return g;
}
var foreign =
{
a: function() {},
b: /value that doesn't coerce purely/,
};
foreign.a[Symbol.toPrimitive] =
function()
{
counter++;
return 0;
};
f(null, foreign);
assertEq(counter, 1);
Attachment #8818527 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 5•9 years ago
|
||
LookupPropertyPure can return a 0x1 Shape* for dense/typed array elements. We didn't handle this before, I guess we never looked up any int jsids. This is however what intrinsic_GetStringDataProperty does.
Attachment #8820639 -
Flags: review?(arai.unmht)
Comment 6•9 years ago
|
||
Comment on attachment 8820639 [details] [diff] [review]
Make NativeGet[Getter]PureInline handle
Review of attachment 8820639 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsobj.cpp
@@ +2358,3 @@
> {
> + if (IsImplicitDenseOrTypedArrayElement(shape)) {
> + *vp = pobj->getDenseOrTypedArrayElement(JSID_TO_INT(id));
id can be string, so we should handle JSID_IS_STRING case here.
https://dxr.mozilla.org/mozilla-central/rev/7083c0d30e75fc102c715887af9faec933e936f8/js/src/vm/TypedArrayObject.h#344-358
Attachment #8820639 -
Flags: review?(arai.unmht) → feedback+
Assignee | ||
Comment 7•9 years ago
|
||
Thanks arai! \o/
Attachment #8820639 -
Attachment is obsolete: true
Attachment #8820645 -
Flags: review?(arai.unmht)
Comment 8•9 years ago
|
||
Comment on attachment 8820645 [details] [diff] [review]
Make NativeGet[Getter]PureInline handle v2
Review of attachment 8820645 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks :)
r+ with maybe adding some note for string case that we intentionally ignore the typed array with string id case.
Attachment #8820645 -
Flags: review?(arai.unmht) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78fdff5726df
Fix uses of ClassMethodIsNative. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e9e1f7ef5a8
Make Make NativeGet[Getter]PureInline handle dense/typed array shapes. r=arai
Comment 10•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/78fdff5726df
https://hg.mozilla.org/mozilla-central/rev/4e9e1f7ef5a8
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•