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)

defect
Not set
normal

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)

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
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
Assignee: general → nobody
Blocks: es6
You are volunteered, Waldo.
Assignee: nobody → jwalden+bmo
Assignee: jwalden+bmo → evilpies
I am almost not sure if some of those uses are worth the complexity, especially ToStringForStringFunction.
Attachment #8818527 - Flags: review?(jwalden+bmo)
Blocks: 1323422
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+
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 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+
Thanks arai! \o/
Attachment #8820639 - Attachment is obsolete: true
Attachment #8820645 - Flags: review?(arai.unmht)
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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: