Last Comment Bug 646129 - [[DefaultValue]] on Date objects is wrong when called with no hint
: [[DefaultValue]] on Date objects is wrong when called with no hint
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
javascript: var d = new Date(2011, 1,...
: 476624 (view as bug list)
Depends on: 692503 CVE-2012-4193
Blocks: 476624
  Show dependency treegraph
 
Reported: 2011-03-29 10:57 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-05-22 09:33 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch, enable test that already landed (or is ahead of this in my mq) to test it (28.16 KB, patch)
2011-04-06 17:12 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Patch without perf loss (28.51 KB, patch)
2011-04-07 11:00 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
A few more adjustments, be more careful about cross-compartment [[DefaultValue]] (and test it) (41.75 KB, patch)
2011-04-08 00:41 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Updated for comments, enough changes that it wants a last once-over (46.36 KB, patch)
2011-06-10 18:42 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-03-29 10:57:54 PDT
See URL.  Note the crazy Date-specific behavior at the end of 8.12.8 in ES5.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-06 17:12:29 PDT
Created attachment 524311 [details] [diff] [review]
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] (remove +bmo to email) 2011-04-07 11:00:14 PDT
Created attachment 524429 [details] [diff] [review]
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] (remove +bmo to email) 2011-04-08 00:41:31 PDT
Created attachment 524595 [details] [diff] [review]
A few more adjustments, be more careful about cross-compartment [[DefaultValue]] (and test it)
Comment 4 Brian Hackett (:bhackett) 2011-06-08 11:15:17 PDT
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] 2011-06-09 20:11:57 PDT
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] (remove +bmo to email) 2011-06-10 18:42:49 PDT
Created attachment 538656 [details] [diff] [review]
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] 2011-06-10 19:09:52 PDT
*Great* to see the pre-historic js_TryValueOf mystery meat go.

/be
Comment 8 Luke Wagner [:luke] 2011-06-10 20:31:12 PDT
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] (remove +bmo to email) 2011-06-13 18:18:26 PDT
http://hg.mozilla.org/tracemonkey/rev/d2250fc608cc
Comment 10 Tom Schuster [:evilpie] 2011-06-17 12:41:59 PDT
*** Bug 476624 has been marked as a duplicate of this bug. ***
Comment 11 Chris Leary [:cdleary] (not checking bugmail) 2011-06-20 17:04:18 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/d2250fc608cc

Note You need to log in before you can comment on or make changes to this bug.