Bug 645468 - Remove js_TryMethod: its semantics aren't what most of its users want, and its utility is limited. r=luke
authorJeff Walden <jwalden@mit.edu>
Mon, 28 Mar 2011 20:01:53 -0700
changeset 67921 0906d9490eafecf27156c2d38b2a78dd1c45df0a
parent 67920 5170b2b6bc72896b82327eaa0e33753138f3c794
child 67922 0cb3a41065e1bef7c7cfa87f3df61f958634cf45
push id1
push userroot
push dateTue, 26 Apr 2011 22:38:44 +0000
treeherdermozilla-beta@bfdb6e623a36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersluke
bugs645468
milestone2.2a1pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 645468 - Remove js_TryMethod: its semantics aren't what most of its users want, and its utility is limited. r=luke
js/src/jsarray.cpp
js/src/jsobj.cpp
js/src/jsobj.h
js/src/json.cpp
js/src/jsstr.cpp
js/src/jsxml.cpp
js/src/tests/js1_7/regress/regress-351503-01.js
js/src/tests/js1_7/regress/regress-351503-02.js
--- a/js/src/jsarray.cpp
+++ b/js/src/jsarray.cpp
@@ -1275,26 +1275,24 @@ array_toString_sub(JSContext *cx, JSObje
         /* Use rval to locally root each element value. */
         JSBool hole;
         if (!JS_CHECK_OPERATION_LIMIT(cx) ||
             !GetElement(cx, obj, index, &hole, rval)) {
             goto out;
         }
 
         /* Get element's character string. */
-        if (!(hole || rval->isNullOrUndefined())) {
+        if (!hole && !rval->isNullOrUndefined()) {
             if (locale) {
                 /* Work on obj.toLocalString() instead. */
-                JSObject *robj;
-
-                if (!js_ValueToObjectOrNull(cx, *rval, &robj))
+                JSObject *robj = ToObject(cx, rval);
+                if (!robj)
                     goto out;
-                rval->setObjectOrNull(robj);
-                JSAtom *atom = cx->runtime->atomState.toLocaleStringAtom;
-                if (!js_TryMethod(cx, robj, atom, 0, NULL, rval))
+                jsid id = ATOM_TO_JSID(cx->runtime->atomState.toLocaleStringAtom);
+                if (!robj->callMethod(cx, id, 0, NULL, rval))
                     goto out;
             }
 
             if (!ValueToStringBuffer(cx, *rval, sb))
                 goto out;
         }
 
         /* Append the separator. */
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -5475,16 +5475,24 @@ JSObject::reportNotConfigurable(JSContex
 bool
 JSObject::reportNotExtensible(JSContext *cx, uintN report)
 {
     return js_ReportValueErrorFlags(cx, report, JSMSG_OBJECT_NOT_EXTENSIBLE,
                                     JSDVG_IGNORE_STACK, ObjectValue(*this),
                                     NULL, NULL, NULL);
 }
 
+bool
+JSObject::callMethod(JSContext *cx, jsid id, uintN argc, Value *argv, Value *vp)
+{
+    Value fval;
+    return js_GetMethod(cx, this, id, JSGET_NO_METHOD_BARRIER, &fval) &&
+           ExternalInvoke(cx, ObjectValue(*this), fval, argc, argv, vp);
+}
+
 JSBool
 js_SetPropertyHelper(JSContext *cx, JSObject *obj, jsid id, uintN defineHow,
                      Value *vp, JSBool strict)
 {
     int protoIndex;
     JSObject *pobj;
     JSProperty *prop;
     const Shape *shape;
@@ -5894,22 +5902,31 @@ DefaultValue(JSContext *cx, JSObject *ob
             ClassMethodIsNative(cx, obj,
                                  &js_StringClass,
                                  ATOM_TO_JSID(cx->runtime->atomState.toStringAtom),
                                  js_str_toString)) {
             *vp = obj->getPrimitiveThis();
             return true;
         }
 
-        if (!js_TryMethod(cx, obj, cx->runtime->atomState.toStringAtom, 0, NULL, &v))
+        Value fval;
+        jsid id = ATOM_TO_JSID(cx->runtime->atomState.toStringAtom);
+        if (!js_GetMethod(cx, obj, id, JSGET_NO_METHOD_BARRIER, &fval))
             return false;
-        if (!v.isPrimitive()) {
-            if (!obj->getClass()->convert(cx, obj, hint, &v))
+        if (js_IsCallable(fval)) {
+            if (!ExternalInvoke(cx, ObjectValue(*obj), fval, 0, NULL, &v))
                 return false;
+            if (v.isPrimitive()) {
+                *vp = v;
+                return true;
+            }
         }
+
+        if (!obj->getClass()->convert(cx, obj, hint, &v))
+            return false;
     } else {
         /* Optimize (new String(...)).valueOf(). */
         Class *clasp = obj->getClass();
         if ((clasp == &js_StringClass &&
              ClassMethodIsNative(cx, obj, &js_StringClass,
                                  ATOM_TO_JSID(cx->runtime->atomState.valueOfAtom),
                                  js_str_toString)) ||
             (clasp == &js_NumberClass &&
@@ -5919,18 +5936,28 @@ DefaultValue(JSContext *cx, JSObject *ob
             *vp = obj->getPrimitiveThis();
             return true;
         }
 
         if (!obj->getClass()->convert(cx, obj, hint, &v))
             return false;
         if (v.isObject()) {
             JS_ASSERT(hint != TypeOfValue(cx, v));
-            if (!js_TryMethod(cx, obj, cx->runtime->atomState.toStringAtom, 0, NULL, &v))
+            Value fval;
+            jsid id = ATOM_TO_JSID(cx->runtime->atomState.toStringAtom);
+            if (!js_GetMethod(cx, obj, id, JSGET_NO_METHOD_BARRIER, &fval))
                 return false;
+            if (js_IsCallable(fval)) {
+                if (!ExternalInvoke(cx, ObjectValue(*obj), fval, 0, NULL, &v))
+                    return false;
+                if (v.isPrimitive()) {
+                    *vp = v;
+                    return true;
+                }
+            }
         }
     }
     if (v.isObject()) {
         /* Avoid recursive death when decompiling in js_ReportValueError. */
         JSString *str;
         if (hint == JSTYPE_STRING) {
             str = JS_InternString(cx, obj->getClass()->name);
             if (!str)
@@ -6238,45 +6265,31 @@ js_ValueToNonNullObject(JSContext *cx, c
     if (!obj)
         js_ReportIsNullOrUndefined(cx, JSDVG_SEARCH_STACK, v, NULL);
     return obj;
 }
 
 JSBool
 js_TryValueOf(JSContext *cx, JSObject *obj, JSType type, Value *rval)
 {
-    Value argv[1];
-
-    argv[0].setString(cx->runtime->atomState.typeAtoms[type]);
-    return js_TryMethod(cx, obj, cx->runtime->atomState.valueOfAtom,
-                        1, argv, rval);
-}
-
-JSBool
-js_TryMethod(JSContext *cx, JSObject *obj, JSAtom *atom,
-             uintN argc, Value *argv, Value *rval)
-{
-    JS_CHECK_RECURSION(cx, return JS_FALSE);
-
-    /*
-     * Report failure only if an appropriate method was found, and calling it
-     * returned failure.  We propagate failure in this case to make exceptions
-     * behave properly.
-     */
-    JSErrorReporter older = JS_SetErrorReporter(cx, NULL);
-    jsid id = ATOM_TO_JSID(atom);
     Value fval;
-    JSBool ok = js_GetMethod(cx, obj, id, JSGET_NO_METHOD_BARRIER, &fval);
-    JS_SetErrorReporter(cx, older);
-    if (!ok)
+    jsid id = ATOM_TO_JSID(cx->runtime->atomState.valueOfAtom);
+    if (!js_GetMethod(cx, obj, id, JSGET_NO_METHOD_BARRIER, &fval))
         return false;
-
-    if (fval.isPrimitive())
-        return JS_TRUE;
-    return ExternalInvoke(cx, ObjectValue(*obj), fval, argc, argv, rval);
+    if (js_IsCallable(fval)) {
+        Value v;
+        Value argv[] = { StringValue(cx->runtime->atomState.typeAtoms[type]) };
+        if (!ExternalInvoke(cx, ObjectValue(*obj), fval, JS_ARRAY_LENGTH(argv), argv, &v))
+            return false;
+        if (v.isPrimitive()) {
+            *rval = v;
+            return true;
+        }
+    }
+    return true;
 }
 
 #if JS_HAS_XDR
 
 JSBool
 js_XDRObject(JSXDRState *xdr, JSObject **objp)
 {
     JSContext *cx;
--- a/js/src/jsobj.h
+++ b/js/src/jsobj.h
@@ -1181,16 +1181,24 @@ struct JSObject : js::gc::Cell {
     bool allocSlot(JSContext *cx, uint32 *slotp);
     bool freeSlot(JSContext *cx, uint32 slot);
 
   public:
     bool reportReadOnly(JSContext* cx, jsid id, uintN report = JSREPORT_ERROR);
     bool reportNotConfigurable(JSContext* cx, jsid id, uintN report = JSREPORT_ERROR);
     bool reportNotExtensible(JSContext *cx, uintN report = JSREPORT_ERROR);
 
+    /*
+     * Get the property with the given id, then call it as a function with the
+     * given arguments, providing this object as |this|. If the property isn't
+     * callable a TypeError will be thrown. On success the value returned by
+     * the call is stored in *vp.
+     */
+    bool callMethod(JSContext *cx, jsid id, uintN argc, js::Value *argv, js::Value *vp);
+
   private:
     js::Shape *getChildProperty(JSContext *cx, js::Shape *parent, js::Shape &child);
 
     /*
      * Internal helper that adds a shape not yet mapped by this object.
      *
      * Notes:
      * 1. getter and setter must be normalized based on flags (see jsscope.cpp).
@@ -1880,20 +1888,16 @@ ToObject(JSContext *cx, js::Value *vp)
  */
 extern JSObject *
 js_ValueToNonNullObject(JSContext *cx, const js::Value &v);
 
 extern JSBool
 js_TryValueOf(JSContext *cx, JSObject *obj, JSType type, js::Value *rval);
 
 extern JSBool
-js_TryMethod(JSContext *cx, JSObject *obj, JSAtom *atom,
-             uintN argc, js::Value *argv, js::Value *rval);
-
-extern JSBool
 js_XDRObject(JSXDRState *xdr, JSObject **objp);
 
 extern void
 js_PrintObjectSlotName(JSTracer *trc, char *buf, size_t bufsize);
 
 extern void
 js_ClearNative(JSContext *cx, JSObject *obj);
 
--- a/js/src/json.cpp
+++ b/js/src/json.cpp
@@ -165,25 +165,29 @@ js_json_stringify(JSContext *cx, uintN a
     }
 
     return JS_TRUE;
 }
 
 JSBool
 js_TryJSON(JSContext *cx, Value *vp)
 {
-    // Checks whether the return value implements toJSON()
-    JSBool ok = JS_TRUE;
+    if (!vp->isObject())
+        return true;
 
-    if (vp->isObject()) {
-        JSObject *obj = &vp->toObject();
-        ok = js_TryMethod(cx, obj, cx->runtime->atomState.toJSONAtom, 0, NULL, vp);
+    JSObject *obj = &vp->toObject();
+    Value fval;
+    jsid id = ATOM_TO_JSID(cx->runtime->atomState.toJSONAtom);
+    if (!js_GetMethod(cx, obj, id, JSGET_NO_METHOD_BARRIER, &fval))
+        return false;
+    if (js_IsCallable(fval)) {
+        if (!ExternalInvoke(cx, ObjectValue(*obj), fval, 0, NULL, vp))
+            return false;
     }
-
-    return ok;
+    return true;
 }
 
 
 static const char quote = '\"';
 static const char backslash = '\\';
 static const char unicodeEscape[] = "\\u00";
 
 static JSBool
--- a/js/src/jsstr.cpp
+++ b/js/src/jsstr.cpp
@@ -3870,21 +3870,27 @@ js_ValueToSource(JSContext *cx, const Va
             /* NB: _ucNstr rather than _ucstr to indicate non-terminated. */
             static const jschar js_negzero_ucNstr[] = {'-', '0'};
 
             return js_NewStringCopyN(cx, js_negzero_ucNstr, 2);
         }
         return js_ValueToString(cx, v);
     }
 
-    JSAtom *atom = cx->runtime->atomState.toSourceAtom;
-    AutoValueRooter tvr(cx);
-    if (!js_TryMethod(cx, &v.toObject(), atom, 0, NULL, tvr.addr()))
-        return NULL;
-    return js_ValueToString(cx, tvr.value());
+    Value rval = NullValue();
+    Value fval;
+    jsid id = ATOM_TO_JSID(cx->runtime->atomState.toSourceAtom);
+    if (!js_GetMethod(cx, &v.toObject(), id, JSGET_NO_METHOD_BARRIER, &fval))
+        return false;
+    if (js_IsCallable(fval)) {
+        if (!ExternalInvoke(cx, v, fval, 0, NULL, &rval))
+            return false;
+    }
+
+    return js_ValueToString(cx, rval);
 }
 
 namespace js {
 
 bool
 EqualStrings(JSContext *cx, JSString *str1, JSString *str2, JSBool *result)
 {
     if (str1 == str2) {
--- a/js/src/jsxml.cpp
+++ b/js/src/jsxml.cpp
@@ -4862,17 +4862,21 @@ xml_deleteProperty(JSContext *cx, JSObje
 
     rval->setBoolean(true);
     return true;
 }
 
 JSBool
 xml_convert(JSContext *cx, JSObject *obj, JSType type, Value *rval)
 {
-    return js_TryMethod(cx, obj, cx->runtime->atomState.toStringAtom, 0, NULL, rval);
+    JSString *str = js_ValueToString(cx, ObjectValue(*obj));
+    if (!str)
+        return false;
+    *rval = StringValue(str);
+    return true;
 }
 
 static JSBool
 xml_enumerate(JSContext *cx, JSObject *obj, JSIterateOp enum_op, Value *statep, jsid *idp)
 {
     JSXML *xml;
     uint32 length, index;
     JSXMLArrayCursor *cursor;
--- a/js/src/tests/js1_7/regress/regress-351503-01.js
+++ b/js/src/tests/js1_7/regress/regress-351503-01.js
@@ -75,17 +75,17 @@ try
   actual = 'No Error';
 }
 catch(ex)
 {
   actual = ex + '';
 }
 reportCompare(expect, actual, summary + ': 3');
 
-expect = 'TypeError: ({}) is not a function';
+expect = "TypeError: can't convert ({toString:{}}) to primitive type";
 try
 {
   3 + ({toString:({}) }) ;
   actual = 'No Error';
 }
 catch(ex)
 {
   actual = ex + '';
--- a/js/src/tests/js1_7/regress/regress-351503-02.js
+++ b/js/src/tests/js1_7/regress/regress-351503-02.js
@@ -79,17 +79,17 @@ function test()
     actual = 'No Error';
   }
   catch(ex)
   {
     actual = ex + '';
   }
   reportCompare(expect, actual, summary + ': 3');
 
-  expect = 'TypeError: ({}) is not a function';
+  expect = "TypeError: can't convert ({toString:{}}) to primitive type";
   try
   {
     3 + ({toString:({}) }) ;
     actual = 'No Error';
   }
   catch(ex)
   {
     actual = ex + '';