Closed Bug 903389 Opened 7 years ago Closed 4 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: evilpie)

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
https://hg.mozilla.org/mozilla-central/rev/78fdff5726df
https://hg.mozilla.org/mozilla-central/rev/4e9e1f7ef5a8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.