JSON.stringify only properly encodes {} and [] as base objects. Firebug Console Log:: >>> JSON.stringify({'asdf':1}) "{"asdf":1}" >>> JSON.stringify(['asdf']) "["asdf"]" >>> JSON.stringify('asdf') "{"0":"a","1":"s","2":"d","3":"f"}" >>> JSON.stringify(1) "{}" >>> JSON.stringify(1.2) "{}" If this is meant to replace framework JSON encoders and json2.js by Crockford it will need to support this.

Yeah, it purposely doesn't right now. Here's what json2.js does? JSON.stringify(5) 5 JSON.stringify(4.2) 4.2 That's not JSON... what is the use case?

Your console is printing the output of the string json2 returns you. >> x = json2.JSON.stringify(1.2) 1.2 >> typeof(x) string json2.JSON.stringify is returning a string of properly encoded json for that number, we are not. There are lots of use cases for encoding a single object without knowing it's type. Note the grammar in 15.12.2 of the newest es3.1 spec "stringify ( value, replacer, space ) The stringify function produces a JSON formatted string that captures information from a JavaScript value. It can take three parameters. The first parameter is required. The value parameter is a JavaScript value, usually an object or array. " Although it mentions that "usually" the value is an object or array it does not preclude other types from being passed. http://wiki.ecmascript.org/doku.php?id=es3.1:es3.1_proposal_working_draft BTW, true and false also do not work: >>> JSON.stringify(true) "{}" >>> JSON.stringify(false) "{}"

I cleared this up on an ECMA conference call. The RFC's grammar states that JSON documents must begin with "{" or "[", ruling out primitive root objects. The nascent 3.1 spec stated that stringify() must produce strings that are valid according to that grammar. That was not intended: stringify() should produce strings that match the "value" production of the grammar in the JSON RFC.

This breaks all apps that use json2.js might get bitten by this bug in Firefox 3.1: json2.js carefully only defines JSON if isn't defined already, but since the native implementation throws InvalidArgument when (e.g.) a string is passed in and the JS implementation doesn't, that makes the behavior much more restrictive. If I'm reading sayrer right, the "correct" version of this according to the 3.1 spec will allow that, so it seems to make little sense to break it now (the referenced value production from RFC 4627 is "value = false / null / true / object / array / number / string").

This should be considered for blocking for 1.9.1. The current behavior breaks an implementation by Douglas Crockford, json2.js, that seems to be widely used. json2.js defines the JSON object if it's not predefined (by the built-in JSON implementation), but the json2.js implementation accepts strings and numbers, whereas the current native implementation only accepts lists or objects; this results in 3.1b2 breaking all applications relying on json2.js. According to sayrer's comment #3, the nascent JS 3.1 spec stated that the current-native implementation was correct, but after checking it seems that that was not the *intended* behavior. The intended behavior would be to accept everything matching the RFC's value grammar, as json2.js does. To sum up: the current behavior breaks a widely implemented script/behavior, and is against the intention of the specification (although it follows the letter of some version of that specification). I personally think that the native implementation should get fixed, but obviously if the spec ends up going in the other direction, it should be kept as-is. It would be quite bad, though, to break scripts now and to have to go back and break them again later.

(In reply to comment #5) > > According to sayrer's comment #3, the nascent JS 3.1 spec stated that the > current-native implementation was correct, but after checking it seems that > that was not the *intended* behavior. Right.

I'm guessing the underlying cause is the same, but: JSON.parse("true") and JSON.parse("1") both throw exceptions as of FF3.1b2 (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2 ID:20081201061100) which is kind of annoying.

Is there anything that needs fixing here? Comment 3 and the first half of comment 6 made it sound like that wasn't the case, but the bug is still open. (though it has a wanted1.9.1-) Would be great to get clarified what, if anything, needs to be done here. (for what it's worth I think it'd be great if you could pass native values through JSON.stringify/JSON.parse, but of course we should follow the spec)

Yes, it still needs to be fixed. According to the latest working draft at http://wiki.ecmascript.org/doku.php?id=es3.1:es3.1_proposal_working_draft (22nd December 2008): "The value parameter is a JavaScript value is usually an object or array, although it can also be a string, boolean, number or null." This makes it pretty clear that JSON.stringify() should handle strings, booleans, numbers and null as well as {} and []. Similarly JSON.parse needs updating. A real-world example is CouchDB's Futon admin console, which currently relies on being able to stringify/parse strings, booleans, numbers and null via Crockford's json2.js, but fails to work in FF 3.1b2.

This bug is marked blocking1.9.1+ and it will be fixed for that release. /be

I've now bumped in to this problem in 3 separate projects that use json2 and were having Minefield compatibility problems because json2.js won't define JSON if it already exists and Minefield was rejecting these values. If we shipped like this I would imagine we'd been in for a large flood of public complaints.

Yes, as per previous comments, we won't be shipping without this bug fixed. There's no need to make a case for it.

Moving to the JS Engine component where this now lives.

Restoring assignee, since sayrer is all over this.

This is a beta blocker, marking.

Stealing...

don't do that

This processes JSON primitive values as demanded by the barbarians at the gate, and as the ES3.1 spec now dictates. Objects no longer have properties of their prototype serialized, again as ES3.1 now dictates.

Eliminate the possibility of silent failure by calling mutually static recursive functions from the API entry point, and reporting any errors there. Also fix up the argument handling of JSON.stringify.

Comment on attachment 362256 [details] [diff] [review] update r=shaver with fix for doubled error report and unification of recursion regardless of primitiveness, per IM.

Comment on attachment 362256 [details] [diff] [review] update > js_json_parse(JSContext *cx, uintN argc, jsval *vp) > { > JSString *s = NULL; > jsval *argv = vp + 2; > >+ JSBool ok = JS_FALSE; >+ JSONParser *jp; > >+ if (JS_ConvertArguments(cx, argc, argv, "S", &s)) { Initialize ok = JS_ConvertArguments(...), avoid useless JS_FALSE initial value. >+ jp = js_BeginJSONParse(cx, vp); >+ ok = jp != NULL; >+ if (ok) { >+ ok = js_ConsumeJSONText(cx, jp, JS_GetStringChars(s), JS_GetStringLength(s)); >+ ok &= js_FinishJSONParse(cx, jp); >+ } > } > > if (!ok) > JS_ReportError(cx, "Error parsing JSON"); This code should not call JS_ReportError after calling subroutines that will (or should) immediately report/set-pending their error/exceptions. > js_json_stringify(JSContext *cx, uintN argc, jsval *vp) > { > jsval *argv = vp + 2; >+ JSBool ok = JS_TRUE; >+ > // Must throw an Error if there isn't a first arg >+ if (!JS_ConvertArguments(cx, argc, argv, "v", vp)) > return JS_FALSE; > >+ ok = js_TryJSON(cx, vp); Init ok on first opportunity, avoid deadwood initializer. >+ > JSType type; >+ if (!ok || *vp == JSVAL_VOID || >+ ((type = JS_TypeOfValue(cx, *vp)) == JSTYPE_FUNCTION || type == JSTYPE_XML)) { > JS_ReportError(cx, "Invalid argument"); Another sometimes redundant report. Errors should generally be propagated as soon as possible, by early return if that wouldn't leak or fail to clean up (and JSAuto* stand ready to help). The *vp == JSVAL_VOID case could be reported using a new js.msg error number. Don't use JS_ReportError, you want to throw a SyntaxError (as crock's code does; this came up at the last TC39 f2f). Also don't capitalize error strings if you do use JS_ReportError -- but it should be confined to the shell, so js.msg catalogs the error messages and exception types. >+stringify_primitive(JSContext *cx, jsval *vp, >+ JSONWriteCallback callback, void *data, JSType type) { >+ Left brace opening function body in column one by itself please. >+static JSBool >+stringify(JSContext *cx, jsval *vp, JSObject *replacer, >+ JSONWriteCallback callback, void *data, uint32 depth) { Ditto. > if (depth > JSON_MAX_DEPTH) > return JS_FALSE; /* encoding error */ >+ >+ if (JSVAL_IS_PRIMITIVE(*vp)) >+ return stringify_primitive(cx, vp, callback, data, JS_TypeOfValue(cx, *vp)); > > JSBool ok = JS_TRUE; > JSObject *obj = JSVAL_TO_OBJECT(*vp); > JSBool isArray = JS_IsArrayObject(cx, obj); > jschar output = jschar(isArray ? '[' : '{'); > if (!callback(&output, 1, data)) > return JS_FALSE; > >@@ -271,18 +306,30 @@ js_Stringify(JSContext *cx, jsval *vp, J > } else { > ks = js_ValueToString(cx, key); > if (!ks) { > ok = JS_FALSE; > break; > } > } > >+ // Don't include prototype properties, since this operation is >+ // supposed to be implemented as if by ES3.1 Object.keys() >+ jsid id; >+ jsval v = JS_FALSE; >+ if (!js_ValueToStringId(cx, STRING_TO_JSVAL(ks), &id) || >+ !js_HasOwnProperty(cx, obj->map->ops->lookupProperty, obj, id, &v)) { >+ ok = JS_FALSE; >+ break; >+ } else if (v == JSVAL_TRUE) { Don't else after break. Do lose the break if you want to fall out of this inner if/else into the common if (!ok) break below. >+ ok = JS_GetUCProperty(cx, obj, JS_GetStringChars(ks), >+ JS_GetStringLength(ks), &outputValue); Should break here too. >+ } else { No need for else. >+ continue; >+ } > } > > if (!ok) > break; This if (!ok) break; is common tail fusion for the last ok assignments in the then and else clauses above, but with the new code it might be better to just have those do their own breaking from the loop. >@@ -296,17 +343,17 @@ js_Stringify(JSContext *cx, jsval *vp, J > if (outputValue == JSVAL_VOID) > continue; > > // output a comma unless this is the first member to write > if (memberWritten) { > output = jschar(','); > ok = callback(&output, 1, data); > if (!ok) >- break; >+ break; Indentation's off here on the if (!ok) line, not the break. >+JSBool >+js_Stringify(JSContext *cx, jsval *vp, JSObject *replacer, >+ JSONWriteCallback callback, void *data, uint32 depth) >+{ >+ if (!stringify(cx, vp, replacer, callback, data, depth)) { > JS_ReportError(cx, "Error during JSON encoding"); > return JS_FALSE; > } > >+ return JS_TRUE; Double report if stringify calls something that sets a pending exception. Why is JS_ReportError being used at all? > GetTopOfObjectStack(JSContext *cx, JSONParser *jp) > { > jsuint length; > if (!js_GetLengthProperty(cx, jp->objectStack, &length)) > return NULL; >- >+ >+ if (length == 0) >+ return NULL; This overloads null return to mean no error as well as error. > static JSBool > HandleNumber(JSContext *cx, JSONParser *jp, const jschar *buf, uint32 len) > { > const jschar *ep; > double val; > if (!js_strtod(cx, buf, buf + len, &ep, &val) || ep != buf + len) > return JS_FALSE; > > jsval numVal; > JSObject *obj = GetTopOfObjectStack(cx, jp); > >+ if (!JS_NewNumberValue(cx, val, &numVal)) >+ return JS_FALSE; >+ >+ if (obj) >+ return PushValue(cx, jp, obj, numVal); >+ >+ // nothing on the object stack, so number value as root >+ *jp->rootVal = numVal; >+ return JS_TRUE; > } > > static JSBool > HandleString(JSContext *cx, JSONParser *jp, const jschar *buf, uint32 len) > { > JSObject *obj = GetTopOfObjectStack(cx, jp); > JSString *str = js_NewStringCopyN(cx, buf, len); >- if (!obj || !str) >+ if (!str) > return JS_FALSE; > >- return PushValue(cx, jp, obj, STRING_TO_JSVAL(str)); >+ if (obj) >+ return PushValue(cx, jp, obj, STRING_TO_JSVAL(str)); >+ Null obj from GetTopOfObjectStack could mean exception pending, this code will leave it stuck without propagating failure: >+ // root value must be primitive >+ *jp->rootVal = STRING_TO_JSVAL(str); >+ return JS_TRUE; . . . >@@ -658,20 +704,22 @@ HandleKeyword(JSContext *cx, JSONParser > else if (buf[0] == 't') > keyword = JSVAL_TRUE; > else if (buf[0] == 'f') > keyword = JSVAL_FALSE; > else > return JS_FALSE; > > JSObject *obj = GetTopOfObjectStack(cx, jp); >+ if (obj) >+ return PushValue(cx, jp, obj, keyword); > >+ // root value must be primitive >+ *jp->rootVal = keyword; >+ return JS_TRUE; Ditto. > js_ConsumeJSONText(JSContext *cx, JSONParser *jp, const jschar *data, uint32 len) > { > uint32 i; > > if (*jp->statep == JSON_PARSE_STATE_INIT) { >- PushState(jp, JSON_PARSE_STATE_OBJECT_VALUE); >+ PushState(jp, JSON_PARSE_STATE_VALUE); > } > > for (i = 0; i < len; i++) { > jschar c = data[i]; > switch (*jp->statep) { > case JSON_PARSE_STATE_VALUE : > if (c == ']') { I spy overindented code. Case labels are like goto labels, half-indented. Also no space before : after the case expr. r- cuz of the error reporting duplication, stuck exceptions, and non-standard JS_ReportError calls. Style nits also worth fixing. Shaver! /be

> > if (depth > JSON_MAX_DEPTH) > > return JS_FALSE; /* encoding error */ > >+ > >+ if (JSVAL_IS_PRIMITIVE(*vp)) > >+ return stringify_primitive(cx, vp, callback, data, JS_TypeOfValue(cx, *vp)); > > > > JSBool ok = JS_TRUE; > > JSObject *obj = JSVAL_TO_OBJECT(*vp); > > JSBool isArray = JS_IsArrayObject(cx, obj); > > jschar output = jschar(isArray ? '[' : '{'); > > if (!callback(&output, 1, data)) > > return JS_FALSE; Left out comment here: more useless ok initializer, possibly useless ok in light of early false return (I didn't look lower past context). /be

(In reply to comment #23) > > Another sometimes redundant report. Actually, it's just wrong per spec. The spec seems to have things like js> JSON.stringify(undefined); js> JSON.stringify({tryJSON: function(){ return undefined; }}); js> JSON.stringify(function () { return 42 }); return undefined without throwing an error. I think that's pretty lame. > Don't use JS_ReportError, you want to throw a SyntaxError No, that's for parse(). > >+stringify_primitive(JSContext *cx, jsval *vp, > >+ JSONWriteCallback callback, void *data, JSType type) { > >+ > > Left brace opening function body in column one by itself please. > > >+static JSBool > >+stringify(JSContext *cx, jsval *vp, JSObject *replacer, > >+ JSONWriteCallback callback, void *data, uint32 depth) { > > Ditto. > > > if (depth > JSON_MAX_DEPTH) > > return JS_FALSE; /* encoding error */ > >+ > >+ if (JSVAL_IS_PRIMITIVE(*vp)) > >+ return stringify_primitive(cx, vp, callback, data, JS_TypeOfValue(cx, *vp)); > > > > JSBool ok = JS_TRUE; > > JSObject *obj = JSVAL_TO_OBJECT(*vp); > > JSBool isArray = JS_IsArrayObject(cx, obj); > > jschar output = jschar(isArray ? '[' : '{'); > > if (!callback(&output, 1, data)) > > return JS_FALSE; > > > > > >@@ -271,18 +306,30 @@ js_Stringify(JSContext *cx, jsval *vp, J > > } else { > > ks = js_ValueToString(cx, key); > > if (!ks) { > > ok = JS_FALSE; > > break; > > } > > } > > > >+ // Don't include prototype properties, since this operation is > >+ // supposed to be implemented as if by ES3.1 Object.keys() > >+ jsid id; > >+ jsval v = JS_FALSE; > >+ if (!js_ValueToStringId(cx, STRING_TO_JSVAL(ks), &id) || > >+ !js_HasOwnProperty(cx, obj->map->ops->lookupProperty, obj, id, &v)) { > >+ ok = JS_FALSE; > >+ break; > >+ } else if (v == JSVAL_TRUE) { > > Don't else after break. Do lose the break if you want to fall out of this inner > if/else into the common if (!ok) break below. > > >+ ok = JS_GetUCProperty(cx, obj, JS_GetStringChars(ks), > >+ JS_GetStringLength(ks), &outputValue); > > Should break here too. > > >+ } else { > > No need for else. > > >+ continue; > >+ } > > } > > > > if (!ok) > > break; > > This if (!ok) break; is common tail fusion for the last ok assignments in the > then and else clauses above, but with the new code it might be better to just > have those do their own breaking from the loop. > > >@@ -296,17 +343,17 @@ js_Stringify(JSContext *cx, jsval *vp, J > > if (outputValue == JSVAL_VOID) > > continue; > > > > // output a comma unless this is the first member to write > > if (memberWritten) { > > output = jschar(','); > > ok = callback(&output, 1, data); > > if (!ok) > >- break; > >+ break; > > Indentation's off here on the if (!ok) line, not the break. > > >+JSBool > >+js_Stringify(JSContext *cx, jsval *vp, JSObject *replacer, > >+ JSONWriteCallback callback, void *data, uint32 depth) > >+{ > >+ if (!stringify(cx, vp, replacer, callback, data, depth)) { > > JS_ReportError(cx, "Error during JSON encoding"); > > return JS_FALSE; > > } > > > >+ return JS_TRUE; > > Double report if stringify calls something that sets a pending exception. Why > is JS_ReportError being used at all? > > > GetTopOfObjectStack(JSContext *cx, JSONParser *jp) > > { > > jsuint length; > > if (!js_GetLengthProperty(cx, jp->objectStack, &length)) > > return NULL; > >- > >+ > >+ if (length == 0) > >+ return NULL; > > This overloads null return to mean no error as well as error. > > > static JSBool > > HandleNumber(JSContext *cx, JSONParser *jp, const jschar *buf, uint32 len) > > { > > const jschar *ep; > > double val; > > if (!js_strtod(cx, buf, buf + len, &ep, &val) || ep != buf + len) > > return JS_FALSE; > > > > jsval numVal; > > JSObject *obj = GetTopOfObjectStack(cx, jp); > > > >+ if (!JS_NewNumberValue(cx, val, &numVal)) > >+ return JS_FALSE; > >+ > >+ if (obj) > >+ return PushValue(cx, jp, obj, numVal); > >+ > >+ // nothing on the object stack, so number value as root > >+ *jp->rootVal = numVal; > >+ return JS_TRUE; > > } > > > > static JSBool > > HandleString(JSContext *cx, JSONParser *jp, const jschar *buf, uint32 len) > > { > > JSObject *obj = GetTopOfObjectStack(cx, jp); > > JSString *str = js_NewStringCopyN(cx, buf, len); > >- if (!obj || !str) > >+ if (!str) > > return JS_FALSE; > > > >- return PushValue(cx, jp, obj, STRING_TO_JSVAL(str)); > >+ if (obj) > >+ return PushValue(cx, jp, obj, STRING_TO_JSVAL(str)); > >+ > > Null obj from GetTopOfObjectStack could mean exception pending, this code will > leave it stuck without propagating failure: > > >+ // root value must be primitive > >+ *jp->rootVal = STRING_TO_JSVAL(str); > >+ return JS_TRUE; > . . . > >@@ -658,20 +704,22 @@ HandleKeyword(JSContext *cx, JSONParser > > else if (buf[0] == 't') > > keyword = JSVAL_TRUE; > > else if (buf[0] == 'f') > > keyword = JSVAL_FALSE; > > else > > return JS_FALSE; > > > > JSObject *obj = GetTopOfObjectStack(cx, jp); > >+ if (obj) > >+ return PushValue(cx, jp, obj, keyword); > > > >+ // root value must be primitive > >+ *jp->rootVal = keyword; > >+ return JS_TRUE; > > Ditto. > > > js_ConsumeJSONText(JSContext *cx, JSONParser *jp, const jschar *data, uint32 len) > > { > > uint32 i; > > > > if (*jp->statep == JSON_PARSE_STATE_INIT) { > >- PushState(jp, JSON_PARSE_STATE_OBJECT_VALUE); > >+ PushState(jp, JSON_PARSE_STATE_VALUE); > > } > > > > for (i = 0; i < len; i++) { > > jschar c = data[i]; > > switch (*jp->statep) { > > case JSON_PARSE_STATE_VALUE : > > if (c == ']') { > > I spy overindented code. Case labels are like goto labels, half-indented. Also > no space before : after the case expr. > > r- cuz of the error reporting duplication, stuck exceptions, and non-standard > JS_ReportError calls. Style nits also worth fixing. Shaver! > > /be

oops, hit commit early. sorry for the massive quoting.

still need to get the error reporting.

rule of thumb: any function JS_ or js_ (or your own statics layered on same) that takes a cx and returns boolean or a pointer with null meaning failure must already have thrown

parse now throws a SyntaxError, and stringify throws an Error in the rare cases it fails (usually it returns undefined). The error messages could be better, but they're not much different crock's, so I don't think they should block the beta.

Sorry, fried -- I diffed the saved patches and skimmed, looks good. You might want to get a fresher mrbkap or igor review, or me after sleep. /be

Comment on attachment 364613 [details] [diff] [review] address brendan's comments this handles unserializable inputs slightly wrong, but 442059 will fix that.

Comment on attachment 364613 [details] [diff] [review] address brendan's comments All the changes look good. You can JS_GetPropertyById instead of JS_GetUCProperty. I think the code could do without the "// unexpected" comments.

addressed jorendorff's comments http://hg.mozilla.org/tracemonkey/rev/7d3ee90d99e7

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9b9077513daf http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8d3715e719df

covered by json unit tests