Closed
Bug 646129
Opened 14 years ago
Closed 13 years ago
[[DefaultValue]] on Date objects is wrong when called with no hint
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: Waldo, Assigned: Waldo)
References
()
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
See URL. Note the crazy Date-specific behavior at the end of 8.12.8 in ES5.
Assignee | ||
Comment 1•14 years ago
|
||
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.
Assignee | ||
Comment 2•14 years ago
|
||
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.
Attachment #524311 -
Attachment is obsolete: true
Attachment #524429 -
Flags: review?(jorendorff)
Assignee | ||
Comment 3•14 years ago
|
||
Attachment #524429 -
Attachment is obsolete: true
Attachment #524429 -
Flags: review?(jorendorff)
Attachment #524595 -
Flags: review?(jorendorff)
Comment 4•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Attachment #524595 -
Flags: review?(luke)
![]() |
||
Comment 5•13 years ago
|
||
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.
Attachment #524595 -
Flags: review?(luke) → review+
Assignee | ||
Comment 6•13 years ago
|
||
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.
Attachment #524595 -
Attachment is obsolete: true
Attachment #524595 -
Flags: review?(jorendorff)
Attachment #538656 -
Flags: review?(luke)
![]() |
||
Comment 8•13 years ago
|
||
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, ®s.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.
Attachment #538656 -
Flags: review?(luke) → review+
Assignee | ||
Comment 9•13 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/d2250fc608cc
Whiteboard: fixed-in-tracemonkey
Comment 11•13 years ago
|
||
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/d2250fc608cc
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Depends on: CVE-2012-4193
You need to log in
before you can comment on or make changes to this bug.
Description
•