554550 - remove lingering defaultValue calls with hint JSTYPE_OBJECT or JSTYPE_FUNCTION

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

I was wandering around defaultValue-land today and came across a couple of places where defaultValue is called with hint = JSTYPE_OBJECT or JSTYPE_FUNCTION.  Apparently, without liveconnect around our necks, this is no longer needed and they can/should be removed.

Uses include (but may not limited to):
js_ValueToObject, js_ValueToFunction, fun_toStringHelper, js_fun_call, js_fun_apply

Comment 2

[Andreas Gal :gal]

This is in the way of my iterator work.

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 438390 [details] [diff] [review]
patch

> JS_FRIEND_DATA(JSClass) js_CallClass = {
>     "Call",
>     JSCLASS_HAS_PRIVATE |
>     JSCLASS_HAS_RESERVED_SLOTS(CALL_CLASS_FIXED_RESERVED_SLOTS) |
>     JSCLASS_NEW_RESOLVE | JSCLASS_IS_ANONYMOUS | JSCLASS_MARK_IS_TRACE,
>     JS_PropertyStub,    JS_PropertyStub,
>     JS_PropertyStub,    JS_PropertyStub,
>     call_enumerate,     (JSResolveOp)call_resolve,
>-    call_convert,       NULL,
>+    JS_ConvertStub,     NULL,

Use NULL not JS_ConvertStub -- we should crash if anyone gets its hands on a Call object and tries to convert it to anything.

> js_ValueToFunction(JSContext *cx, jsval *vp, uintN flags)
> {
>     jsval v;
>     JSObject *obj;
> 
>     v = *vp;
>+    if (!VALUE_IS_FUNCTION(cx, v)) {
>         js_ReportIsNotFunction(cx, vp, flags);
>         return NULL;
>     }
>+    return GET_FUNCTION_PRIVATE(cx, JSVAL_TO_OBJECT(v));

Great -- just wondering whether proxies will change this again.

>@@ -358,18 +358,18 @@ js_ValueToIterator(JSContext *cx, uintN 
> 
>     JS_ASSERT(!(flags & ~(JSITER_ENUMERATE | JSITER_FOREACH | JSITER_KEYVALUE)));
> 
>     /* JSITER_KEYVALUE must always come with JSITER_FOREACH */
>     JS_ASSERT(!(flags & JSITER_KEYVALUE) || (flags & JSITER_FOREACH));
> 
>     AutoValueRooter tvr(cx);
> 
>-    /* XXX work around old valueOf call hidden beneath js_ValueToObject */
>     if (!JSVAL_IS_PRIMITIVE(*vp)) {
>+        /* Common case. */
>         obj = JSVAL_TO_OBJECT(*vp);
>     } else {
>         /*
>          * Enumerating over null and undefined gives an empty enumerator.
>          * This is contrary to ECMA-262 9.9 ToObject, invoked from step 3 of
>          * the first production in 12.6.4 and step 4 of the second production,
>          * but it's "web JS" compatible.

Add "ES5 fixed for-in to match this de-facto standard." or words to that effect.

ECMA-262 Fifth Edition 12.6.4 step 3 (ignore the "experValue" typos).

[js_DefaultValue:]
>       default:
>         if (!obj->getClass()->convert(cx, obj, hint, &v))
>             return JS_FALSE;
>         if (!JSVAL_IS_PRIMITIVE(v)) {
>             JSType type = JS_TypeOfValue(cx, v);
>-            if (type == hint ||
>-                (type == JSTYPE_FUNCTION && hint == JSTYPE_OBJECT)) {
>-                goto out;
>-            }
>-            if (!js_TryMethod(cx, obj, cx->runtime->atomState.toStringAtom, 0,
>-                              NULL, &v)) {
>+            if (type != hint &&
>+                !js_TryMethod(cx, obj, cx->runtime->atomState.toStringAtom, 0, NULL, &v)) {

If type != hint but !JSVAL_IS_PRIMITIVE(v) and type = JS_TypeOfValue(cx, v), then type must be JSTYPE_OBJECT or JSTYPE_FUNCTION. But by the assertion at the top of js_DefaultValue that this patch adds, hint cannot be JSTYPE_OBJECT or JSTYPE_FUNCTION, therefore type != hint is always true.

What's really going on here is that we want to fall back on js_TryMethod(...toString...) if convert (AKA tryValueOf) succeeded (didn't throw/report/return-false) but did not give a primitive result (of any primitive type).

So maybe just assert that type != hint and try toString.

/be

Comment 5

[Andreas Gal :gal]

Attachment: patch

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 438392 [details] [diff] [review]
patch

r=me, looks great -- but what is up with that test failure you mentioned on IRC?

/be

Comment 7

[Brendan Eich [:brendan]]

Cite it! :-P

http://hg.mozilla.org/tracemonkey/rev/3de0a7da3a8e

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> (From update of attachment 438392 [details] [diff] [review])
> r=me, looks great -- but what is up with that test failure you mentioned on
> IRC?

Anyone? Bueller?

/be

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3de0a7da3a8e