646129 - [[DefaultValue]] on Date objects is wrong when called with no hint

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

See URL.  Note the crazy Date-specific behavior at the end of 8.12.8 in ES5.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch, enable test that already landed (or is ahead of this in my mq) to test it

So, this is the right way to do it (atop a bunch of other patches, so probably not directly applicable without some fussing).  But it seems to regress SunSpider slightly, maybe 22% or so.  I have ideas.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch without perf loss

Turns out gcc treats (x == EXTERN_SYMBOL ? EXTERN_SYMBOL : x)(...) as though it were x(...), and at least if x is from memory dereferenced to a function pointer, that's a little slow.  Why that was *22%* I'm not certain, but (x == EXTERN_SYMBOL ? INTERNAL_SYMBOL_CALLED_BY_EXTERN_SYMBOL : x)(...) fixes the problem.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: A few more adjustments, be more careful about cross-compartment [[DefaultValue]] (and test it)

Comment 4

[Brian Hackett [Laid off!]]

This testcase (from bug 642894 comment 4) is crashing TM tip with a stack overflow.  Should check this is fixed once this bug is resolved.

a = <x><y/></x>;
a.function::__proto__ = null;
-a

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 524595 [details] [diff] [review]
A few more adjustments, be more careful about cross-compartment [[DefaultValue]] (and test it)

Review of attachment 524595 [details] [diff] [review]:
-----------------------------------------------------------------

Good stuff, r+ with nits.

::: js/src/jsapi.cpp
@@ -2894,4 @@
>  JS_ConvertStub(JSContext *cx, JSObject *obj, JSType type, jsval *vp)
>  {
>      JS_ASSERT(type != JSTYPE_OBJECT && type != JSTYPE_FUNCTION);
> -    return js_TryValueOf(cx, obj, type, Valueify(vp));

Your reign of blood continues and another ambiguously-spec-related utility falls.  While I am tempted to be silent and have the joy of slaughter for myself, I must point out that you haven't removed js_TryValueOf in this patch.

::: js/src/jsdate.cpp
@@ +485,5 @@
>   * end of ECMA 'support' functions
>   */
>  
> +static JSBool
> +date_convert(JSContext *cx, JSObject *obj, JSType hint, Value *vp)

As discussed, perhaps we can just reuse DefaultValue with JSTYPE_VOID converted to JSTYPE_STRING.

@@ +2140,4 @@
>  
>      /* Step 2. */
>      Value &tv = vp[0];
> +    if (!obj->defaultValue(cx, JSTYPE_NUMBER, &tv))

Following the precedent you have started, could you add:

  static JS_ALWAYS_INLINE bool
  ToPrimitive(JSObject &obj, JSType preferredType, Value *vp) {
      /* ES5 9.1, Table 10 (Object case) */
      return DefaultValue(cx, *obj, preferredType, vp);
  }

and use that here instead of defaultValue?  It'll match the ToObject in Step 1.

::: js/src/jsinterp.cpp
@@ +1141,3 @@
>          return false;
>  
> +    if (rvalue.isObject() && !rvalue.toObject().defaultValue(cx, JSTYPE_VOID, &rvalue))

Ditto above.  It would seem that everywhere you put defaultValue in this patch could be replaced by ToPrimitive; could you do that?

::: js/src/jsnum.cpp
@@ +1269,1 @@
>              break;

Feel free to ignore, but since we are making this all spec-y, how about s/ValueToNumber/ToNumber/?

::: js/src/jsobj.cpp
@@ +5920,1 @@
>              return false;

Awesome!  This "convert" hook call has definitely confused me before, I was just too superstitious at the time to consider that it might be *gasp* wrong.

@@ +5961,5 @@
>                  return false;
> +            if (v.isPrimitive()) {
> +                *vp = v;
> +                return true;
> +            }

Ok, this is the fourth instantiation of this pattern.  A quick scan of js_GetMethod sites shows this is the only instance of this pattern, so maybe you can just put a little static helper above that specifically references ES5 8.12.8.  If you keep the v.isPrimitive() check *out* of the helper, then you can avoid the tri-bool return value annoyance.

::: js/src/jsstr.cpp
@@ +429,4 @@
>          return cx->runtime->atomState.typeAtoms[JSTYPE_VOID];
>      vp += 2 + arg;
>  
> +    if (vp->isObject() && !vp->toObject().defaultValue(cx, JSTYPE_STRING, vp))

Instead of using the ToPrimitive() defined above, you could define an additional ToPrimitive() overload for all values:

  static JS_ALWAYS_INLINE bool
  ToPrimitive(JSType preferredType, Value *vp) {
      /* ES5 9.1 */
      if (vp->isObject())
          return vp->toObject().defaultValue(cx, JSTYPE_STRING, vp);
      return true;
  }

(Yeah, in/out params are gross, but otherwise its a pretty wasteful copy on hot paths.)
Looks like there are another 2 uses in this file and 2 in StubCalls.cpp.

::: js/src/jswrapper.cpp
@@ +714,5 @@
> +    if (!JSWrapper::defaultValue(cx, wrapper, hint, vp))
> +        return false;
> +
> +    call.leave();
> +    return call.origin->wrap(cx, vp);

Pre-existing style, but it seems like it could just be:

  {
     AutoCompartment call(...);
     ...
  }
  return cx->compartment->wrap(cx, vp);

Your choice.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Updated for comments, enough changes that it wants a last once-over

Regarding "fourth instantiation of this pattern": made the change, not convinced it really makes much difference, but doesn't really hurt.

Regarding ToNumber: sensible, but separate bug for sure.

Regarding "Pre-existing style": it seems to comport with the other stuff around it, left unchanged.

Regarding js_TryValueOf, oops, didn't realize I'd killed all users -- removed.

Comment 7

[Brendan Eich [:brendan]]

*Great* to see the pre-historic js_TryValueOf mystery meat go.

/be

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 538656 [details] [diff] [review]
Updated for comments, enough changes that it wants a last once-over

Review of attachment 538656 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good.  r+ assuming agreement with nits below.

(In reply to comment #6)
> Regarding "fourth instantiation of this pattern": made the change, not
> convinced it really makes much difference, but doesn't really hurt.

Are you kidding?  I much prefer the "after" (esp. after the 8 line reduction mentioned below).  Btw, this is the type of micro-factoring that I think would benefit bug 655192.

::: js/src/jsinterp.cpp
@@ +3426,5 @@
>              cond = lval.toInt32() OP rval.toInt32();                          \
>          } else {                                                              \
> +            if (!ToPrimitive(cx, JSTYPE_NUMBER, &lval))                       \
> +                goto error;                                                   \
> +            regs.sp[-2] = lval;                                               \

With stack scanning, regs.sp[-2] I don't see this assignment being necessary (maybe I'm missing a hidden aliasing?).  But if we are trying to write code that doesn't depend on stack scanning (future exact rooting), perhaps instead we change rval/lval to references to regs.sp slots?

@@ +3531,5 @@
>  #endif
>      {
> +        if (!ToPrimitive(cx, &regs.sp[-2]))
> +            goto error;
> +        lval = regs.sp[-2];

Same question.

::: js/src/jsobj.cpp
@@ +5961,4 @@
>              return false;
> +        if (rval.isPrimitive()) {
> +            *vp = rval;
> +            return true;

Can haz MaybeCallMethod(..., vp) ?  Saves 2 x 4 lines.

::: js/src/jsobj.h
@@ +1796,5 @@
> +static JS_ALWAYS_INLINE bool
> +ToPrimitive(JSContext *cx, JSObject &obj, JSType preferredType, Value *vp)
> +{
> +    ConvertOp op = obj.getClass()->convert;
> +    return (op == ConvertStub ? DefaultValue : op)(cx, &obj, preferredType, vp);

I liked this better in your first patch as JSObject::defaultValue; it is the spec-method [[DefaultValue]] on spec-objects after all; also it matches all the other similar-looking Class/ObjectOps-dispatching functions.  At the least it should be named DefaultValue.

Comment 9

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/d2250fc608cc

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d2250fc608cc