903389 - valueOf getter not called for ToPrimitive conversion on wrapper types

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

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

[Jeff Walden [:Waldo]]

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.

Comment 2

[Shu-yu Guo [:shu]]

You are volunteered, Waldo.

Comment 3

[Tom Schuster [:evilpie]]

Attachment: Fix uses of ClassMethodIsNative

I am almost not sure if some of those uses are worth the complexity, especially ToStringForStringFunction.

Comment 4

[Jeff Walden [:Waldo]]

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);

Comment 5

[Tom Schuster [:evilpie]]

Attachment: Make NativeGet[Getter]PureInline handle

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.

Comment 6

[Tooru Fujisawa [:arai]]

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

Comment 7

[Tom Schuster [:evilpie]]

Attachment: Make NativeGet[Getter]PureInline handle v2

Thanks arai! \o/

Comment 8

[Tooru Fujisawa [:arai]]

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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/78fdff5726df
https://hg.mozilla.org/mozilla-central/rev/4e9e1f7ef5a8