Closed Bug 459161 Opened 11 years ago Closed 11 years ago

Process first argument to JSON stringify and parse methods as specified by ES3.1

Categories

(Core :: JavaScript Engine, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: mikeal, Assigned: sayrer)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 5 obsolete files)

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").
Flags: wanted1.9.1?
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.
Flags: blocking1.9.1?
(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.
Assignee: nobody → sayrer
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1?
Flags: blocking1.9.1+
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.
Assignee: sayrer → general
Component: DOM → JavaScript Engine
QA Contact: general → general
Restoring assignee, since sayrer is all over this.
Assignee: general → sayrer
Duplicate of this bug: 475421
This is a beta blocker, marking.
Priority: -- → P1
Stealing...
Assignee: sayrer → jwalden+bmo
Status: NEW → ASSIGNED
don't do that
Assignee: jwalden+bmo → sayrer
Summary: JSON.stringify does not encode numbers and strings as base objects → Process first argument to JSON stringify and encode as specified by ES3.1
Summary: Process first argument to JSON stringify and encode as specified by ES3.1 → Process first argument to JSON stringify and parse methods as specified by ES3.1
Attached patch As ES3.1 does (obsolete) — Splinter Review
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.
Attachment #362123 - Flags: review?(shaver)
Attached patch update (obsolete) — Splinter Review
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.
Attachment #362256 - Flags: review?(shaver)
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.
Attachment #362256 - Flags: review?(shaver) → review+
Attached patch as checked in (obsolete) — Splinter Review
Attachment #362295 - Attachment is patch: true
Attachment #362295 - Attachment mime type: application/octet-stream → text/plain
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
Attachment #362256 - Flags: review-
> >     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.
Attached patch Fix some of brendan's comments (obsolete) — Splinter Review
still need to get the error reporting.
Attachment #362123 - Attachment is obsolete: true
Attachment #362256 - Attachment is obsolete: true
Attachment #362295 - Attachment is obsolete: true
Attachment #362312 - Attachment is obsolete: true
Attachment #362123 - Flags: review?(shaver)
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.
Attachment #362320 - Attachment is obsolete: true
Attachment #364613 - Flags: review?(brendan)
Attachment #364613 - Flags: review?(brendan)
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.
Attachment #364613 - Flags: review?(jorendorff)
Attachment #364613 - Flags: review?(jorendorff) → review+
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
Keywords: 4xp
Whiteboard: fixed-in-tracemonkey
Keywords: 4xp
http://hg.mozilla.org/mozilla-central/rev/7d3ee90d99e7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
covered by json unit tests
Flags: in-testsuite+
Blocks: 549636
You need to log in before you can comment on or make changes to this bug.