Closed
Bug 903389
Opened 11 years ago
Closed 7 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: evilpie)
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•11 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•10 years ago
|
Assignee: general → nobody
Assignee | ||
Updated•8 years ago
|
Assignee: jwalden+bmo → evilpies
Assignee | ||
Comment 3•8 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•7 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•7 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•7 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•7 years ago
|
||
Thanks arai! \o/
Attachment #8820639 -
Attachment is obsolete: true
Attachment #8820645 -
Flags: review?(arai.unmht)
Comment 8•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/78fdff5726df https://hg.mozilla.org/mozilla-central/rev/4e9e1f7ef5a8
Status: NEW → RESOLVED
Closed: 7 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
•