Bug 636079 - Refactor JSON.stringify code to more closely conform to the specification, fixing a bunch of known problems in the process. r=pbiggar
authorJeff Walden <jwalden@mit.edu>
Fri, 18 Feb 2011 03:34:34 -0800
changeset 67930 67596937aa398b3fee032ef90e63eb6541f18749
parent 67929 1a6fd2901325072a720e5ff84366a406a7be91ab
child 67931 0756cd76cb066d9f7399612873bd21e3e0c7de26
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)
reviewerspbiggar
bugs636079
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 636079 - Refactor JSON.stringify code to more closely conform to the specification, fixing a bunch of known problems in the process. r=pbiggar Bugs fixed: * toJSON is now invoked with an argument list consisting of the property name. * In rare circumstances toJSON was invoked twice for a single object-valued property. This error has now been corrected. * Stringification no longer throws if the toJSON property of an object being stringified is an object but is not callable. * The replacer function is invoked exactly once for properties of objects when those properties are stringified. * If a replacer function is to be called, it will receive array indexes as strings instead of numbers, per ES5. Other improvements include: * Speedier internal methods are used, rather than slow external APIs. * Argument types are more specific (e.g. the "holder" argument is an object). * Logic to determine when to call the replacer function is unnecessary and has been removed.
js/src/Makefile.in
js/src/jsatom.h
js/src/json.cpp
js/src/shell/js.cpp
js/src/tests/ecma_5/JSON/jstests.list
js/src/tests/ecma_5/JSON/stringify-call-replacer-once.js
js/src/tests/ecma_5/JSON/stringify-call-toJSON-once.js
js/src/tests/ecma_5/JSON/stringify-ignore-noncallable-toJSON.js
js/src/tests/ecma_5/JSON/stringify-replacer-with-array-indexes.js
js/src/tests/ecma_5/JSON/stringify-toJSON-arguments.js
--- a/js/src/Makefile.in
+++ b/js/src/Makefile.in
@@ -209,16 +209,17 @@ INSTALLED_HEADERS = \
 		jscompartment.h \
 		jshash.h \
 		jsinterp.h \
 		jsinttypes.h \
 		jsiter.h \
 		jslock.h \
 		jslong.h \
 		jsmath.h \
+		jsnum.h \
 		jsobj.h \
 		jsobjinlines.h \
 		json.h \
 		jsopcode.tbl \
 		jsopcode.h \
 		jsopcodeinlines.h \
 		jsotypes.h \
 		jsparse.h \
--- a/js/src/jsatom.h
+++ b/js/src/jsatom.h
@@ -43,16 +43,17 @@
  * JS atom table.
  */
 #include <stddef.h>
 #include "jsversion.h"
 #include "jsapi.h"
 #include "jsprvtd.h"
 #include "jshash.h"
 #include "jshashtable.h"
+#include "jsnum.h"
 #include "jspubtd.h"
 #include "jsstr.h"
 #include "jslock.h"
 #include "jsvalue.h"
 
 #define ATOM_PINNED     0x1       /* atom is pinned against GC */
 #define ATOM_INTERNED   0x2       /* pinned variant for JS_Intern* API */
 #define ATOM_NOCOPY     0x4       /* don't copy atom string bytes */
@@ -109,16 +110,26 @@ IdToValue(jsid id)
 }
 
 static JS_ALWAYS_INLINE jsval
 IdToJsval(jsid id)
 {
     return Jsvalify(IdToValue(id));
 }
 
+static JS_ALWAYS_INLINE JSString *
+IdToString(JSContext *cx, jsid id)
+{
+    if (JSID_IS_STRING(id))
+        return JSID_TO_STRING(id);
+    if (JS_LIKELY(JSID_IS_INT(id)))
+        return js_IntToString(cx, JSID_TO_INT(id));
+    return js_ValueToString(cx, IdToValue(id));
+}
+
 }
 
 #if JS_BYTES_PER_WORD == 4
 # define ATOM_HASH(atom)          ((JSHashNumber)(atom) >> 2)
 #elif JS_BYTES_PER_WORD == 8
 # define ATOM_HASH(atom)          (((JSHashNumber)(jsuword)(atom) >> 3) ^     \
                                    (JSHashNumber)((jsuword)(atom) >> 32))
 #else
--- a/js/src/json.cpp
+++ b/js/src/json.cpp
@@ -273,20 +273,17 @@ public:
 
     StringBuffer &sb;
     StringBuffer gap;
     JSObject *replacer;
     uint32 depth;
     HashSet<JSObject *> objectStack;
 };
 
-static JSBool CallReplacerFunction(JSContext *cx, jsid id, JSObject *holder,
-                                   StringifyContext *scx, Value *vp);
-static JSBool Str(JSContext *cx, jsid id, JSObject *holder,
-                  StringifyContext *scx, Value *vp, bool callReplacer = true);
+static JSBool Str(JSContext *cx, const Value &v, StringifyContext *scx);
 
 static JSBool
 WriteIndent(JSContext *cx, StringifyContext *scx, uint32 limit)
 {
     if (!scx->gap.empty()) {
         if (!scx->sb.append('\n'))
             return JS_FALSE;
         for (uint32 i = 0; i < limit; i++) {
@@ -318,207 +315,74 @@ class CycleDetector
         objectStack.remove(obj);
     }
 
   private:
     HashSet<JSObject *> &objectStack;
     JSObject *const obj;
 };
 
-static JSBool
-JO(JSContext *cx, Value *vp, StringifyContext *scx)
+/*
+ * ES5 15.12.3 Str, steps 2-4, extracted to enable preprocessing of property
+ * values when stringifying objects in JO.
+ */
+static bool
+PreprocessValue(JSContext *cx, JSObject *holder, jsid key, Value *vp, StringifyContext *scx)
 {
-    JSObject *obj = &vp->toObject();
-
-    CycleDetector detect(scx, obj);
-    if (!detect.init(cx))
-        return JS_FALSE;
-
-    if (!scx->sb.append('{'))
-        return JS_FALSE;
-
-    Value vec[3] = { NullValue(), NullValue(), NullValue() };
-    AutoArrayRooter tvr(cx, JS_ARRAY_LENGTH(vec), vec);
-    Value& outputValue = vec[0];
-    Value& whitelistElement = vec[1];
-    AutoIdRooter idr(cx);
-    jsid& id = *idr.addr();
-
-    Value *keySource = vp;
-    bool usingWhitelist = false;
+    JSString *keyStr = NULL;
 
-    // if the replacer is an array, we use the keys from it
-    if (scx->replacer && JS_IsArrayObject(cx, scx->replacer)) {
-        usingWhitelist = true;
-        vec[2].setObject(*scx->replacer);
-        keySource = &vec[2];
-    }
-
-    JSBool memberWritten = JS_FALSE;
-    AutoIdVector props(cx);
-    if (!GetPropertyNames(cx, &keySource->toObject(), JSITER_OWNONLY, &props))
-        return JS_FALSE;
-
-    for (size_t i = 0, len = props.length(); i < len; i++) {
-        outputValue.setUndefined();
-
-        if (!usingWhitelist) {
-            if (!js_ValueToStringId(cx, IdToValue(props[i]), &id))
-                return JS_FALSE;
-        } else {
-            // skip non-index properties
-            jsuint index = 0;
-            if (!js_IdIsIndex(props[i], &index))
-                continue;
-
-            if (!scx->replacer->getProperty(cx, props[i], &whitelistElement))
-                return JS_FALSE;
+    /* Step 2. */
+    if (vp->isObject()) {
+        Value toJSON;
+        jsid id = ATOM_TO_JSID(cx->runtime->atomState.toJSONAtom);
+        if (!js_GetMethod(cx, &vp->toObject(), id, JSGET_NO_METHOD_BARRIER, &toJSON))
+            return false;
 
-            if (!js_ValueToStringId(cx, whitelistElement, &id))
-                return JS_FALSE;
-        }
-
-        // We should have a string id by this point. Either from 
-        // JS_Enumerate's id array, or by converting an element
-        // of the whitelist.
-        JS_ASSERT(JSID_IS_ATOM(id));
-
-        if (!JS_GetPropertyById(cx, obj, id, Jsvalify(&outputValue)))
-            return JS_FALSE;
-
-        if (outputValue.isObjectOrNull() && !js_TryJSON(cx, &outputValue))
-            return JS_FALSE;
-
-        // call this here, so we don't write out keys if the replacer function
-        // wants to elide the value.
-        if (!CallReplacerFunction(cx, id, obj, scx, &outputValue))
-            return JS_FALSE;
-
-        JSType type = JS_TypeOfValue(cx, Jsvalify(outputValue));
+        if (js_IsCallable(toJSON)) {
+            keyStr = IdToString(cx, key);
+            if (!keyStr)
+                return false;
 
-        // elide undefined values and functions and XML
-        if (outputValue.isUndefined() || type == JSTYPE_FUNCTION || type == JSTYPE_XML)
-            continue;
-
-        // output a comma unless this is the first member to write
-        if (memberWritten && !scx->sb.append(','))
-            return JS_FALSE;
-        memberWritten = JS_TRUE;
-
-        if (!WriteIndent(cx, scx, scx->depth))
-            return JS_FALSE;
+            LeaveTrace(cx);
+            InvokeArgsGuard args;
+            if (!cx->stack().pushInvokeArgs(cx, 1, &args))
+                return false;
 
-        // Be careful below, this string is weakly rooted
-        JSString *s = js_ValueToString(cx, IdToValue(id));
-        if (!s)
-            return JS_FALSE;
+            args.callee() = toJSON;
+            args.thisv() = *vp;
+            args[0] = StringValue(keyStr);
 
-        JS::Anchor<JSString *> anchor(s);
-        size_t length = s->length();
-        const jschar *chars = s->getChars(cx);
-        if (!chars)
-            return JS_FALSE;
-
-        if (!write_string(cx, scx->sb, chars, length) ||
-            !scx->sb.append(':') ||
-            !(scx->gap.empty() || scx->sb.append(' ')) ||
-            !Str(cx, id, obj, scx, &outputValue, true)) {
-            return JS_FALSE;
+            if (!Invoke(cx, args, 0))
+                return false;
+            *vp = args.rval();
         }
     }
 
-    if (memberWritten && !WriteIndent(cx, scx, scx->depth - 1))
-        return JS_FALSE;
-
-    return scx->sb.append('}');
-}
-
-static JSBool
-JA(JSContext *cx, Value *vp, StringifyContext *scx)
-{
-    JSObject *obj = &vp->toObject();
-
-    CycleDetector detect(scx, obj);
-    if (!detect.init(cx))
-        return JS_FALSE;
-
-    if (!scx->sb.append('['))
-        return JS_FALSE;
-
-    jsuint length;
-    if (!js_GetLengthProperty(cx, obj, &length))
-        return JS_FALSE;
-
-    if (length != 0 && !WriteIndent(cx, scx, scx->depth))
-        return JS_FALSE;
-
-    AutoValueRooter outputValue(cx);
-
-    jsid id;
-    jsuint i;
-    for (i = 0; i < length; i++) {
-        id = INT_TO_JSID(i);
-
-        if (!obj->getProperty(cx, id, outputValue.addr()))
-            return JS_FALSE;
-
-        if (!Str(cx, id, obj, scx, outputValue.addr()))
-            return JS_FALSE;
-
-        if (outputValue.value().isUndefined()) {
-            if (!scx->sb.append("null"))
-                return JS_FALSE;
+    /* Step 3. */
+    if (scx->replacer && scx->replacer->isCallable()) {
+        if (!keyStr) {
+            keyStr = IdToString(cx, key);
+            if (!keyStr)
+                return false;
         }
 
-        if (i < length - 1) {
-            if (!scx->sb.append(','))
-                return JS_FALSE;
-            if (!WriteIndent(cx, scx, scx->depth))
-                return JS_FALSE;
-        }
-    }
-
-    if (length != 0 && !WriteIndent(cx, scx, scx->depth - 1))
-        return JS_FALSE;
-
-    return scx->sb.append(']');
-}
+        LeaveTrace(cx);
+        InvokeArgsGuard args;
+        if (!cx->stack().pushInvokeArgs(cx, 2, &args))
+            return false;
 
-static JSBool
-CallReplacerFunction(JSContext *cx, jsid id, JSObject *holder, StringifyContext *scx, Value *vp)
-{
-    if (scx->replacer && scx->replacer->isCallable()) {
-        Value vec[2] = { IdToValue(id), *vp};
-        if (!JS_CallFunctionValue(cx, holder, OBJECT_TO_JSVAL(scx->replacer),
-                                  2, Jsvalify(vec), Jsvalify(vp))) {
-            return JS_FALSE;
-        }
-    }
-
-    return JS_TRUE;
-}
+        args.callee() = ObjectValue(*scx->replacer);
+        args.thisv() = ObjectValue(*holder);
+        args[0] = StringValue(keyStr);
+        args[1] = *vp;
 
-static JSBool
-Str(JSContext *cx, jsid id, JSObject *holder, StringifyContext *scx, Value *vp, bool callReplacer)
-{
-    JS_CHECK_RECURSION(cx, return false);
-
-    /*
-     * This method implements the Str algorithm in ES5 15.12.3, but we move
-     * property retrieval (step 1) into callers to stream the stringification
-     * process and avoid constantly copying strings.
-     */
-
-    /* Step 2. */
-    if (vp->isObject() && !js_TryJSON(cx, vp))
-        return false;
-
-    /* Step 3. */
-    if (callReplacer && !CallReplacerFunction(cx, id, holder, scx, vp))
-        return false;
+        if (!Invoke(cx, args, 0))
+            return false;
+        *vp = args.rval();
+    }
 
     /* Step 4. */
     if (vp->isObject()) {
         JSObject *obj = &vp->toObject();
         Class *clasp = obj->getClass();
         if (clasp == &js_NumberClass) {
             double d;
             if (!ValueToNumber(cx, *vp, &d))
@@ -526,87 +390,303 @@ Str(JSContext *cx, jsid id, JSObject *ho
             vp->setNumber(d);
         } else if (clasp == &js_StringClass) {
             JSString *str = js_ValueToString(cx, *vp);
             if (!str)
                 return false;
             vp->setString(str);
         } else if (clasp == &js_BooleanClass) {
             *vp = obj->getPrimitiveThis();
+            JS_ASSERT(vp->isBoolean());
         }
     }
 
+    return true;
+}
+
+/*
+ * Determines whether a value which has passed by ES5 150.2.3 Str steps 1-4's
+ * gauntlet will result in Str returning |undefined|.  This function is used to
+ * properly omit properties resulting in such values when stringifying objects,
+ * while properly stringifying such properties as null when they're encountered
+ * in arrays.
+ */
+static inline bool
+IsFilteredValue(const Value &v)
+{
+    return v.isUndefined() || js_IsCallable(v) || (v.isObject() && v.toObject().isXML());
+}
+
+/* ES5 15.12.3 JO. */
+static JSBool
+JO(JSContext *cx, JSObject *obj, StringifyContext *scx)
+{
+    /*
+     * This method implements the JO algorithm in ES5 15.12.3, but:
+     *
+     *   * The algorithm is somewhat reformulated to allow the final string to
+     *     be streamed into a single buffer, rather than be created and copied
+     *     into place incrementally as the ES5 algorithm specifies it.  This
+     *     requires moving portions of the Str call in 8a into this algorithm
+     *     (and in JA as well).
+     */
+
+    /* Steps 1-2, 11. */
+    CycleDetector detect(scx, obj);
+    if (!detect.init(cx))
+        return JS_FALSE;
+
+    if (!scx->sb.append('{'))
+        return JS_FALSE;
+
+    AutoIdRooter idr(cx);
+    jsid& id = *idr.addr();
+
+    /* Steps 5-7. */
+    /* XXX Bug 648471: Do this in js_Stringify, rename keySource. */
+    Value keySource = ObjectValue(*obj);
+    bool usingWhitelist = false;
+
+    // if the replacer is an array, we use the keys from it
+    if (scx->replacer && JS_IsArrayObject(cx, scx->replacer)) {
+        usingWhitelist = true;
+        keySource.setObject(*scx->replacer);
+    }
+
+    bool wroteMember = false;
+    AutoIdVector props(cx);
+    if (!GetPropertyNames(cx, &keySource.toObject(), JSITER_OWNONLY, &props))
+        return JS_FALSE;
+
+    /* Steps 8-10, 13. */
+    for (size_t i = 0, len = props.length(); i < len; i++) {
+        if (!usingWhitelist) {
+            if (!js_ValueToStringId(cx, IdToValue(props[i]), &id))
+                return JS_FALSE;
+        } else {
+            // skip non-index properties
+            jsuint index = 0;
+            if (!js_IdIsIndex(props[i], &index))
+                continue;
+
+            Value whitelistElement;
+            if (!scx->replacer->getProperty(cx, props[i], &whitelistElement))
+                return JS_FALSE;
+
+            if (!js_ValueToStringId(cx, whitelistElement, &id))
+                return JS_FALSE;
+        }
+
+        /*
+         * Steps 8a-8b.  Note that the call to Str is broken up into 1) getting
+         * the property; 2) processing for toJSON, calling the replacer, and
+         * handling boxed Number/String/Boolean objects; 3) filtering out
+         * values which process to |undefined|, and 4) stringifying all values
+         * which pass the filter.
+         */
+        Value outputValue;
+        if (!obj->getProperty(cx, id, &outputValue))
+            return JS_FALSE;
+        if (!PreprocessValue(cx, obj, id, &outputValue, scx))
+            return JS_FALSE;
+        if (IsFilteredValue(outputValue))
+            continue;
+
+        /* Output a comma unless this is the first member to write. */
+        if (wroteMember && !scx->sb.append(','))
+            return JS_FALSE;
+        wroteMember = true;
+
+        if (!WriteIndent(cx, scx, scx->depth))
+            return JS_FALSE;
+
+        // Be careful below: this string is weakly rooted!
+        JSString *s = js_ValueToString(cx, IdToValue(id));
+        if (!s)
+            return JS_FALSE;
+
+        JS::Anchor<JSString *> anchor(s);
+        size_t length = s->length();
+        const jschar *chars = s->getChars(cx);
+        if (!chars)
+            return JS_FALSE;
+
+        if (!write_string(cx, scx->sb, chars, length) ||
+            !scx->sb.append(':') ||
+            !(scx->gap.empty() || scx->sb.append(' ')) ||
+            !Str(cx, outputValue, scx)) {
+            return JS_FALSE;
+        }
+    }
+
+    if (wroteMember && !WriteIndent(cx, scx, scx->depth - 1))
+        return JS_FALSE;
+
+    return scx->sb.append('}');
+}
+
+/* ES5 15.12.3 JA. */
+static JSBool
+JA(JSContext *cx, JSObject *obj, StringifyContext *scx)
+{
+    /*
+     * This method implements the JA algorithm in ES5 15.12.3, but:
+     *
+     *   * The algorithm is somewhat reformulated to allow the final string to
+     *     be streamed into a single buffer, rather than be created and copied
+     *     into place incrementally as the ES5 algorithm specifies it.  This
+     *     requires moving portions of the Str call in 8a into this algorithm
+     *     (and in JO as well).
+     */
+
+    /* Steps 1-2, 11. */
+    CycleDetector detect(scx, obj);
+    if (!detect.init(cx))
+        return JS_FALSE;
+
+    if (!scx->sb.append('['))
+        return JS_FALSE;
+
+    /* Step 6. */
+    jsuint length;
+    if (!js_GetLengthProperty(cx, obj, &length))
+        return JS_FALSE;
+
+    /* Steps 7-10. */
+    if (length != 0) {
+        /* Steps 4, 10b(i). */
+        if (!WriteIndent(cx, scx, scx->depth))
+            return JS_FALSE;
+
+        /* Steps 7-10. */
+        Value outputValue;
+        for (jsuint i = 0; i < length; i++) {
+            jsid id = INT_TO_JSID(i);
+
+            /*
+             * Steps 8a-8c.  Again note how the call to the spec's Str method
+             * is broken up into getting the property, running it past toJSON
+             * and the replacer and maybe unboxing, and interpreting some
+             * values as |null| in separate steps.
+             */
+            if (!obj->getProperty(cx, id, &outputValue))
+                return JS_FALSE;
+            if (!PreprocessValue(cx, obj, id, &outputValue, scx))
+                return JS_FALSE;
+            if (IsFilteredValue(outputValue)) {
+                if (!scx->sb.append("null"))
+                    return JS_FALSE;
+            } else {
+                if (!Str(cx, outputValue, scx))
+                    return JS_FALSE;
+            }
+
+            /* Steps 3, 4, 10b(i). */
+            if (i < length - 1) {
+                if (!scx->sb.append(','))
+                    return JS_FALSE;
+                if (!WriteIndent(cx, scx, scx->depth))
+                    return JS_FALSE;
+            }
+        }
+
+        /* Step 10(b)(iii). */
+        if (!WriteIndent(cx, scx, scx->depth - 1))
+            return JS_FALSE;
+    }
+
+    return scx->sb.append(']');
+}
+
+static JSBool
+Str(JSContext *cx, const Value &v, StringifyContext *scx)
+{
+    /* Step 11 must be handled by the caller. */
+    JS_ASSERT(!IsFilteredValue(v));
+
+    JS_CHECK_RECURSION(cx, return false);
+
+    /*
+     * This method implements the Str algorithm in ES5 15.12.3, but:
+     *
+     *   * We move property retrieval (step 1) into callers to stream the
+     *     stringification process and avoid constantly copying strings.
+     *   * We move the preprocessing in steps 2-4 into a helper function to
+     *     allow both JO and JA to use this method.  While JA could use it
+     *     without this move, JO must omit any |undefined|-valued property per
+     *     so it can't stream out a value using the Str method exactly as
+     *     defined by ES5.
+     *   * We move step 11 into callers, again to ease streaming.
+     */
+
     /* Step 8. */
-    if (vp->isString()) {
-        JSString *str = vp->toString();
+    if (v.isString()) {
+        JSString *str = v.toString();
         size_t length = str->length();
         const jschar *chars = str->getChars(cx);
         if (!chars)
             return false;
         return write_string(cx, scx->sb, chars, length);
     }
 
     /* Step 5. */
-    if (vp->isNull())
+    if (v.isNull())
         return scx->sb.append("null");
 
     /* Steps 6-7. */
-    if (vp->isBoolean())
-        return vp->toBoolean() ? scx->sb.append("true") : scx->sb.append("false");
+    if (v.isBoolean())
+        return v.toBoolean() ? scx->sb.append("true") : scx->sb.append("false");
 
     /* Step 9. */
-    if (vp->isNumber()) {
-        if (vp->isDouble()) {
-            jsdouble d = vp->toDouble();
-            if (!JSDOUBLE_IS_FINITE(d))
+    if (v.isNumber()) {
+        if (v.isDouble()) {
+            if (!JSDOUBLE_IS_FINITE(v.toDouble()))
                 return scx->sb.append("null");
         }
 
         StringBuffer sb(cx);
-        if (!NumberValueToStringBuffer(cx, *vp, sb))
+        if (!NumberValueToStringBuffer(cx, v, sb))
             return false;
 
         return scx->sb.append(sb.begin(), sb.length());
     }
 
     /* Step 10. */
-    if (vp->isObject() && !IsFunctionObject(*vp) && !IsXML(*vp)) {
-        JSBool ok;
-
-        scx->depth++;
-        ok = (JS_IsArrayObject(cx, &vp->toObject()) ? JA : JO)(cx, vp, scx);
-        scx->depth--;
+    JS_ASSERT(v.isObject());
+    JSBool ok;
 
-        return ok;
-    }
+    scx->depth++;
+    ok = (JS_IsArrayObject(cx, &v.toObject()) ? JA : JO)(cx, &v.toObject(), scx);
+    scx->depth--;
 
-    /* Step 11. */
-    vp->setUndefined();
-    return true;
+    return ok;
 }
 
 JSBool
 js_Stringify(JSContext *cx, Value *vp, JSObject *replacer, const Value &space,
              StringBuffer &sb)
 {
     StringifyContext scx(cx, sb, replacer);
     if (!scx.initializeGap(cx, space) || !scx.initializeStack())
         return JS_FALSE;
 
     JSObject *obj = NewBuiltinClassInstance(cx, &js_ObjectClass);
     if (!obj)
         return JS_FALSE;
 
     AutoObjectRooter tvr(cx, obj);
-    if (!obj->defineProperty(cx, ATOM_TO_JSID(cx->runtime->atomState.emptyAtom),
-                             *vp, NULL, NULL, JSPROP_ENUMERATE)) {
+    jsid emptyId = ATOM_TO_JSID(cx->runtime->atomState.emptyAtom);
+    if (!obj->defineProperty(cx, emptyId, *vp, NULL, NULL, JSPROP_ENUMERATE))
         return JS_FALSE;
-    }
 
-    return Str(cx, ATOM_TO_JSID(cx->runtime->atomState.emptyAtom), obj, &scx, vp);
+    if (!PreprocessValue(cx, obj, emptyId, vp, &scx))
+        return JS_FALSE;
+    if (IsFilteredValue(*vp))
+        return JS_TRUE;
+    return Str(cx, *vp, &scx);
 }
 
 // helper to determine whether a character could be part of a number
 static JSBool IsNumChar(jschar c)
 {
     return ((c <= '9' && c >= '0') || c == '.' || c == '-' || c == '+' || c == 'e' || c == 'E');
 }
 
--- a/js/src/shell/js.cpp
+++ b/js/src/shell/js.cpp
@@ -264,19 +264,19 @@ class ToString {
     }
   private:
     JSContext *cx;
     JSString *mStr;
     JSBool mThrow;
     JSAutoByteString mBytes;
 };
 
-class IdToString : public ToString {
+class IdStringifier : public ToString {
 public:
-    IdToString(JSContext *cx, jsid id, JSBool aThrow = JS_FALSE)
+    IdStringifier(JSContext *cx, jsid id, JSBool aThrow = JS_FALSE)
     : ToString(cx, IdToJsval(id), aThrow)
     { }
 };
 
 static char *
 GetLine(FILE *file, const char * prompt)
 {
     size_t size;
@@ -5113,53 +5113,53 @@ static JSBool its_noisy;    /* whether t
 static JSBool its_enum_fail;/* whether to fail when enumerating it */
 
 static JSBool
 its_addProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
 {
     if (!its_noisy)
         return JS_TRUE;
 
-    IdToString idString(cx, id);
+    IdStringifier idString(cx, id);
     fprintf(gOutFile, "adding its property %s,", idString.getBytes());
     ToString valueString(cx, *vp);
     fprintf(gOutFile, " initial value %s\n", valueString.getBytes());
     return JS_TRUE;
 }
 
 static JSBool
 its_delProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
 {
     if (!its_noisy)
         return JS_TRUE;
 
-    IdToString idString(cx, id);
+    IdStringifier idString(cx, id);
     fprintf(gOutFile, "deleting its property %s,", idString.getBytes());
     ToString valueString(cx, *vp);
     fprintf(gOutFile, " initial value %s\n", valueString.getBytes());
     return JS_TRUE;
 }
 
 static JSBool
 its_getProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
 {
     if (!its_noisy)
         return JS_TRUE;
 
-    IdToString idString(cx, id);
+    IdStringifier idString(cx, id);
     fprintf(gOutFile, "getting its property %s,", idString.getBytes());
     ToString valueString(cx, *vp);
     fprintf(gOutFile, " initial value %s\n", valueString.getBytes());
     return JS_TRUE;
 }
 
 static JSBool
 its_setProperty(JSContext *cx, JSObject *obj, jsid id, JSBool strict, jsval *vp)
 {
-    IdToString idString(cx, id);
+    IdStringifier idString(cx, id);
     if (its_noisy) {
         fprintf(gOutFile, "setting its property %s,", idString.getBytes());
         ToString valueString(cx, *vp);
         fprintf(gOutFile, " new value %s\n", valueString.getBytes());
     }
 
     if (!JSID_IS_ATOM(id))
         return JS_TRUE;
@@ -5220,17 +5220,17 @@ its_enumerate(JSContext *cx, JSObject *o
     return JS_TRUE;
 }
 
 static JSBool
 its_resolve(JSContext *cx, JSObject *obj, jsid id, uintN flags,
             JSObject **objp)
 {
     if (its_noisy) {
-        IdToString idString(cx, id);
+        IdStringifier idString(cx, id);
         fprintf(gOutFile, "resolving its property %s, flags {%s,%s,%s}\n",
                idString.getBytes(),
                (flags & JSRESOLVE_QUALIFIED) ? "qualified" : "",
                (flags & JSRESOLVE_ASSIGNING) ? "assigning" : "",
                (flags & JSRESOLVE_DETECTING) ? "detecting" : "");
     }
     return JS_TRUE;
 }
@@ -5516,17 +5516,17 @@ JSClass global_class = {
 
 static JSBool
 env_setProperty(JSContext *cx, JSObject *obj, jsid id, JSBool strict, jsval *vp)
 {
 /* XXX porting may be easy, but these don't seem to supply setenv by default */
 #if !defined XP_OS2 && !defined SOLARIS
     int rv;
 
-    IdToString idstr(cx, id, JS_TRUE);
+    IdStringifier idstr(cx, id, JS_TRUE);
     if (idstr.threw())
         return JS_FALSE;
     ToString valstr(cx, *vp, JS_TRUE);
     if (valstr.threw())
         return JS_FALSE;
 #if defined XP_WIN || defined HPUX || defined OSF1 || defined IRIX
     {
         char *waste = JS_smprintf("%s=%s", idstr.getBytes(), valstr.getBytes());
@@ -5595,17 +5595,17 @@ env_resolve(JSContext *cx, JSObject *obj
             JSObject **objp)
 {
     JSString *valstr;
     const char *name, *value;
 
     if (flags & JSRESOLVE_ASSIGNING)
         return JS_TRUE;
 
-    IdToString idstr(cx, id, JS_TRUE);
+    IdStringifier idstr(cx, id, JS_TRUE);
     if (idstr.threw())
         return JS_FALSE;
 
     name = idstr.getBytes();
     value = getenv(name);
     if (value) {
         valstr = JS_NewStringCopyZ(cx, value);
         if (!valstr)
--- a/js/src/tests/ecma_5/JSON/jstests.list
+++ b/js/src/tests/ecma_5/JSON/jstests.list
@@ -1,6 +1,12 @@
 url-prefix ../../jsreftest.html?test=ecma_5/JSON/
 script cyclic-stringify.js
 script small-codepoints.js
-script trailing-comma.js
+script stringify-boxed-primitives.js
+script stringify-call-replacer-once.js
+script stringify-call-toJSON-once.js
 script stringify-gap.js
-script stringify-boxed-primitives.js
+script stringify-ignore-noncallable-toJSON.js
+script stringify-replacer-with-array-indexes.js
+script stringify-toJSON-arguments.js
+script trailing-comma.js
+
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/JSON/stringify-call-replacer-once.js
@@ -0,0 +1,34 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/licenses/publicdomain/
+
+var gTestfile = 'stringify-call-replacer-once.js';
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 584909;
+var summary = "Call replacer function exactly once per value";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var factor = 1;
+function replacer(k, v)
+{
+  if (k === "")
+    return v;
+
+  return v * ++factor;
+}
+
+var obj = { a: 1, b: 2, c: 3 };
+
+assertEq(JSON.stringify(obj, replacer), '{"a":2,"b":6,"c":12}');
+assertEq(factor, 4);
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/JSON/stringify-call-toJSON-once.js
@@ -0,0 +1,32 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/licenses/publicdomain/
+
+var gTestfile = 'stringify-call-toJSON-once.js';
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 584909;
+var summary = "Stringification of Boolean/String/Number objects";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var obj =
+  {
+    p: {
+         toJSON: function()
+         {
+           return { toJSON: function() { return 17; } };
+         }
+       }
+  };
+
+assertEq(JSON.stringify(obj), '{"p":{}}');
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/JSON/stringify-ignore-noncallable-toJSON.js
@@ -0,0 +1,28 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/licenses/publicdomain/
+
+var gTestfile = 'stringify-ignore-noncallable-toJSON.js';
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 584909;
+var summary = "If the toJSON property isn't callable, don't try to call it";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var obj =
+  {
+    p: { toJSON: null },
+    m: { toJSON: {} }
+  };
+
+assertEq(JSON.stringify(obj), '{"p":{"toJSON":null},"m":{"toJSON":{}}}');
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/JSON/stringify-replacer-with-array-indexes.js
@@ -0,0 +1,56 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/licenses/publicdomain/
+
+var gTestfile = 'stringify-replacer-with-array-indexes.js';
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 584909;
+var summary =
+  "Call the replacer function for array elements with stringified indexes";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var arr = [0, 1, 2, 3, 4];
+
+var seenTopmost = false;
+var index = 0;
+function replacer()
+{
+  assertEq(arguments.length, 2);
+
+  var key = arguments[0], value = arguments[1];
+
+  // Topmost array: ignore replacer call.
+  if (key === "")
+  {
+    assertEq(seenTopmost, false);
+    seenTopmost = true;
+    return value;
+  }
+
+  assertEq(seenTopmost, true);
+
+  assertEq(typeof key, "string");
+  assertEq(key === index, false);
+  assertEq(key === index + "", true);
+
+  assertEq(value, index);
+
+  index++;
+
+  assertEq(this, arr);
+
+  return value;
+}
+
+assertEq(JSON.stringify(arr, replacer), '[0,1,2,3,4]');
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/JSON/stringify-toJSON-arguments.js
@@ -0,0 +1,34 @@
+// Any copyright is dedicated to the Public Domain.
+// http://creativecommons.org/licenses/publicdomain/
+
+var gTestfile = 'stringify-toJSON-arguments.js';
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 584909;
+var summary = "Arguments when an object's toJSON method is called";
+
+print(BUGNUMBER + ": " + summary);
+
+/**************
+ * BEGIN TEST *
+ **************/
+
+var obj =
+  {
+    p: {
+         toJSON: function(key)
+         {
+           assertEq(arguments.length, 1);
+           assertEq(key, "p");
+           return 17;
+         }
+       }
+  };
+
+assertEq(JSON.stringify(obj), '{"p":17}');
+
+/******************************************************************************/
+
+if (typeof reportCompare === "function")
+  reportCompare(true, true);
+
+print("Tests complete");