387522 - Native JSON support

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Dave Camp (:dcamp)]

I need a C/C++ JSON parser for parsing offline manifest files, and threw together a simple saxish parser.  It's still kinda rough, but rsayre wanted to take a look, so here it is :)

Comment 1

[Dave Camp (:dcamp)]

Attachment: SAX-style json parser (WIP)

Comment 2

[Dave Camp (:dcamp)]

Attachment: some test cases

Comment 3

[Robert Sayre]

I found an MIT licensed JSON library called json-c. We may want to use its tokenizer. It includes implementations of arrays and objects as well, but we don't need those. ;)

http://oss.metaparadigm.com/json-c/

Comment 4

[Robert Sayre]

Attachment: native JSON serializer

like http://google-caja.googlecode.com/svn/trunk/src/js/com/google/caja/JSON.js, but without the bugs, and no optFilter arg yet.

Comment 5

[Robert Sayre]

Attachment: native JSON serializer

this time with IDL

Comment 6

[Robert Sayre]

Attachment: testcases for use in xpcshell

-v 170 -f /path/to/file

These tests all pass, with the exception of the __iterator__ case, but I think I see how to make that work. For odd input, I compared results to Crockford's json.js. When I hook this up to a harness, I'll add that file to check against.

The optional optFilter argument isn't yet supported, but it should be easy to add. When I do so, I'll have to test nasty stuff like mutations of the objects we're iterating. I should also call GC() from the filter function tests. 

Depth and breadth DoS attacks will need to be accounted for as well, with simple numeric limits if nothing else.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Quick scan:
1. Should we expose this to content (a la XHR)?
2. Should we always use our DefaultFilter, even when passed a optFilter?
3. Comments in IDL are messed up for 2 methods (copy/paste?)

Comment 8

[Robert Sayre]

(In reply to comment #7)
> Quick scan:
> 1. Should we expose this to content (a la XHR)?

Hopefully, depends on how well-understood the security concerns are.

> 2. Should we always use our DefaultFilter, even when passed a optFilter?

We should probably expose DefaultFilter to JS, so it can be composed with other functions.

> 3. Comments in IDL are messed up for 2 methods (copy/paste?)
 
Yeah.

Comment 9

[Paul Querna]

You might also be interested in libjsox, rather than the parser presented in this patch:
http://code.google.com/p/libjsox/

It provides a SAX-like API to JSON and has worked quite well for me....

Comment 10

[Robert Sayre]

Attachment: updated tests, function names, output streams

Comment 11

[Robert Sayre]

Attachment: output streams in UTF-8/16/32, stream tests, fixes

Comment 12

[Mike Schroepfer]

Flagging with perf and setting on blocking list since this shows up on:

http://wiki.mozilla.org/Performance:FrontEnd

Comment 13

[Robert Sayre]

Recent developments in ES4 are pointing to qn API similar to http://www.json.org/json2.js. I have that mostly done, but I feel there are a few issues with the design.

First, it will "stringify" any argument. This means that the value returned when called with primitive arguments will not be valid JSON. Checking on the root doesn't allow a single, recursive, referentially transparent serialization function. I think there's a relevant H. L. Mencken quote. ;) For primitive inputs, I think the method should return undefined for the web-exposed string variant and throw an exception for EncodeToStream.

Second, I have __iterator__ et al. working now, by making the jsiter.h functions JS_FRIEND_API. This works well in general, but there are two exceptions:

js> var x = [1,2,3]
js> x.foo = "bar"

In this example, there are no prop flags to distinguish indexed properties, or at least JSPROP_INDEX isn't set when it should be. The other tricky case is 

js> var x = new String("N")

The old json.org code serialized this as {"0":"N"}, and the new one uses ["N"]. 

json2.js handles both of these cases like so:

                if (typeof value.length === 'number' &&
                        !(value.propertyIsEnumerable('length'))) {

We could do this too, but I'm not sure we want to live with this choice forever, and we're back to the old XXX comment in jarray.h concerning js_IsArrayLike.

IMHO, this is a lot of handwringing over some corner cases in the thoroughly-tested serializer, and shouldn't block the (imminent!) review of this feature.

Comment 14

[Mike Connor [:mconnor]]

Robert, is there an ETA for this patch other than "soon" ?

Comment 15

[Robert Sayre]

On vacation till 11/27. Updates on tuesday.

Comment 16

[Robert Sayre]

Attachment: native json

more tests and some cleanup in a bit

Comment 17

[Robert Sayre]

Attachment: native json

Comment 18

[Robert Sayre]

Attachment: native json

btw, this patch does nothing with the optional JS second args proposed by Crockford. We'll add those if/when we expose this to content.

Comment 19

[Brian Crowder]

Comment on attachment 291284 [details] [diff] [review]
native json

Looks good to me, should get a review from a DOM peer, too though.

Comment 20

[Robert Sayre]

Attachment: native json

Fixed crowder's comments via IRC (mostly smart pointer usage) and fixed some nits.

This patch omits support for:

The decoding filter.
The encoding whitelist.
Typed exceptions for content.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 291301 [details] [diff] [review]
native json

- In dom/src/json/Makefile.in:

+include $(topsrcdir)/config/rules.mk
\ No newline at end of file

Add a newline?

- In nsJSON::EncodeInternal():

+  nsresult rv;
+  nsCOMPtr<nsIXPConnect> xpc =
+    do_GetService(kXPConnectServiceCID, &rv);
+  NS_ENSURE_SUCCESS(rv, rv);

Since this code is part of layout etc, you can use nsContentUtils::XPConnect() here, and no need for nsCOMPtr then.

- In nsJSON::EncodeObject():

+    ok = js_CallIteratorNext(cx, iterObj, &key);
+    if (ok && key != JSVAL_HOLE) {
+      jsval outputValue = JSVAL_VOID;
+      JSString *ks = JSVAL_TO_STRING(key);

Is there code somewhere that I missed that says js_CallIteratorNext() always returns a string key? If so, then this is works, otherwise you'd need to deal properly with non-string keys here.

+      // Check if this a key we should use
+      if (ks) {
+        ok = JS_GetProperty(cx, obj, JS_GetStringBytes(ks), &outputValue);

Do we care about non-ASCII strings here? Any reason not to use JS_GetUCProperty() here?

- In nsJSON::ToJSON():

+  if (JSVAL_IS_OBJECT(value) && (obj = JSVAL_TO_OBJECT(value))) {

If you use !JSVAL_IS_PRIMITIVE() here instead of JSVAL_IS_OBJECT() you can drop the obj null check, and move the JSVAL_TO_OBJECT inside the if. Up to you...

... more in a bit

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 291301 [details] [diff] [review]
native json

- In nsJSON::DecodeInternal():

+  nsCOMPtr<nsIXPConnect> xpc =
+    do_GetService(kXPConnectServiceCID, &rv);

nsContentUtils::XPConnect() here too.

- In nsJSONListener::PopState():

+{
+ mStatep--;
+ if (mStatep < mStateStack) {

One space indentation? :)

- In dom/src/json/nsJSON.cpp, and nsJSON.h, and a bunch of other files:

\ No newline at end of file

r=jst with all that fixed.

Comment 23

[Damon Sicore (:damons)]

What's left on this?  Did brendan give you any feedback, sayre?

Comment 24

[Mike Schroepfer]

Setting TM to M11 - we are past b2 code freeze and unlikely that SS or places can get wired up in time.  We should land this right after tree re-opens to get that going.  Dietrich/Sayre adjust if my understanding is wrong.

Comment 25

[Robert Sayre]

It should be trivial to change the callers for this--the API is the same as the JS code we're using now. FWIW, I thought the plan was:

"Code freeze for Firefox 3 Beta 2 is currently scheduled for tomorrow, Tues Dec 4. The goal is to be code complete by Friday in order to ship Beta 2 prior to Christmas."

and

"* After the tree is closed, only existing P1 bugs will be allowed to land. Everything else requires explicit approval from endgame drivers."

Comment 26

[Robert Sayre]

Attachment: address jst's comments

Comment 27

[Robert Sayre]

(In reply to comment #21)
> 
> Is there code somewhere that I missed that says js_CallIteratorNext() always
> returns a string key? If so, then this is works, otherwise you'd need to deal
> properly with non-string keys here.

Good catch. The native iterators always return string keys, but it's possible to yield something else from __iterator__. I've added to the tests for this case.

Comment 28

[Mike Schroepfer]

(In reply to comment #25)
> It should be trivial to change the callers for this--the API is the same as the
> JS code we're using now. FWIW, I thought the plan was:
> 
> "Code freeze for Firefox 3 Beta 2 is currently scheduled for tomorrow, Tues Dec
> 4. The goal is to be code complete by Friday in order to ship Beta 2 prior to
> Christmas."
> 
> and
> 
> "* After the tree is closed, only existing P1 bugs will be allowed to land.
> Everything else requires explicit approval from endgame drivers."
> 

Yes that's the plan.  It is Thursday - one day before the final freeze and this + the dependent bugs have to land to make this work.   FWIW this can land all on its own either way - I was just chatting to Dietrich on IRC last night and he seemed skeptical that everything could get wired up in time (see dependent bugs for more detail).  If we can get it done and done safely I'm all for it - I want this *badly* but we are running out of time...

Comment 29

[Dietrich Ayala (:dietrich)]

(In reply to comment #28)
> Yes that's the plan.  It is Thursday - one day before the final freeze and this
> + the dependent bugs have to land to make this work.   FWIW this can land all
> on its own either way - I was just chatting to Dietrich on IRC last night and
> he seemed skeptical that everything could get wired up in time (see dependent
> bugs for more detail).  If we can get it done and done safely I'm all for it -
> I want this *badly* but we are running out of time...
> 

Given the lateness of the cycle and the nature of the Places changes, I'm not going to land our bits in B2. We have zero "i lost all my bookmarks" bugs, so I'm favoring caution.

Comment 30

[Robert Sayre]

(In reply to comment #29)
> 
> Given the lateness of the cycle and the nature of the Places changes, I'm not
> going to land our bits in B2. We have zero "i lost all my bookmarks" bugs, so
> I'm favoring caution.

No reason to rush it, then.

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 291897 [details] [diff] [review]
address jst's comments

>+  //XXX NS_OutputStreamIsBuffered(aStream) asserts on file streams...
>+  //if (!NS_OutputStreamIsBuffered(aStream)) {

Bug on file, cited by FIXME instead of XXX here?

>+  inputVal = argv[firstArg];

Local copy is probably just more register pressure for the compiler to hassle over, and sets of GC safety alerts -- avoid inputVal (see below for more).

>+
>+  JSObject *whitelist = nsnull;

Blank line here for readability?

>+  // If there's a second argument here, it should be an array.
>+  if (argc >= firstArg + 2 &&
>+      !(JSVAL_IS_OBJECT(argv[firstArg + 1]) &&
>+        (whitelist = JSVAL_TO_OBJECT(argv[firstArg + 1])) &&
>+        JS_IsArrayObject(cx, whitelist))) {
>+     whitelist = nsnull; // bogus whitelists are ignored
>+  }
>+
>+  inputVal = ToJSON(cx, inputVal);

So inputVal is live across the whitelist error-check/placeholder code but unchanged from argv[firstArg]. Since argv[firstArg] is a local GC root by definition, it would be better to pass &argv[firstArg] to ToJSON as an in/out param, so ToJSON could use that local root to protect the tree of GC-things it allocates.

>+  if (!(JSVAL_IS_OBJECT(inputVal) &&
>+        (inputObj = JSVAL_TO_OBJECT(inputVal))) ||
>+      JS_ObjectIsFunction(cx, inputObj)) {
>+    // return if it's not something we can deal with
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+
>+  return EncodeObject(cx, inputObj, writer, whitelist, 0);

Then we can be sure that the JSON object root passed to EncodeObject is protected against (nesting) GC here.

>+nsresult
>+nsJSON::EncodeObject(JSContext *cx, JSObject *obj, nsJSONWriter *writer,
>+                     JSObject *whitelist, PRUint32 depth)
>+{
>+  NS_ENSURE_ARG(obj);
>+  NS_ENSURE_ARG(writer);
>+
>+  if (depth > JSON_MAX_DEPTH) {
>+    return NS_ERROR_FAILURE;

Could do the XPConnect exception jazz to get a nicer JS error here (EncodingError). Followup bug fodder? Again FIXME: nnnnnn beats XXX.

>+  }
>+
>+  nsresult rv;
>+  JSBool isArray = JS_IsArrayObject(cx, obj);
>+  PRUnichar output;
>+  if (isArray) {
>+    output = PRUnichar('[');
>+  } else {
>+    output = PRUnichar('{');
>+  }

Six lines to one w/o loss of clarity if you initialize output using a ?: expr. Could even common the cast.

>+  JSBool ok = JS_TRUE;
>+  JSObject *iterObj = nsnull;
>+  jsval iterVal = OBJECT_TO_JSVAL(obj);
>+  JSAutoTempValueRooter tvr(cx, 1, &iterVal);

If the caller passes in a rooted jsval location by reference, then this is not necessary. Since EncodeObject is an internal helper, it seems acceptable to require the internal callers (EncodeInternal, EncodeObject itself) to do that. So both EncodeObject and ToJSON would take a jsval *vp in/"out" parameter, and use it as a rooted location. This parameter would not be a true out param, of course, but the indirection is needed to get the rooting.

>+  ok = js_ValueToIterator(cx, JSITER_ENUMERATE, &iterVal);

Thus obj flows into js_ValueToIterator via vp, and the result which strongly references obj on successful (ok) return is protected via *vp.

>+  if (ok)
>+    iterObj = JSVAL_TO_OBJECT(iterVal);

Want prompt failure if !ok, preserving JS exception pending in cx. More below.

>+  jsval key;
>+  PRBool memberWritten = PR_FALSE;
>+  do {
>+    ok = js_CallIteratorNext(cx, iterObj, &key);

if !ok after the js_ValueToIterator, this line passes null iterObj to js_CallIteratorNext, which null-deref crashes; but if it didn't, the false ok would be stomped by the return from js_CallIteratorNext. Better to deal with failures early.

Of course after we are supposed to close the iterator when done, so early return means break with cleanup, or an auto-storage-class C++ helper to use the RAII pattern and automate the close.

>+    if (ok && key != JSVAL_HOLE) {

With ok test removed (dealt with promptly earlier), you could unindent normal-case code by continuing if key == JSVAL_HOLE (no else after continue of course -- see below).

>+      jsval outputValue = JSVAL_VOID;
>+      JSString *ks;
>+
>+      if (JSVAL_IS_STRING(key))
>+        ks = JSVAL_TO_STRING(key);
>+      else
>+        ks = JS_ValueToString(cx, key);

Need to deal with null ks return from JS_ValueToString as an already-pending JS exception, not to be overwritten by an XPConnect generic-for-bad-nsresult exception.

>+
>+      if (ks) {

So this test, inverted, should be in the else clause above -- the JSVAL_IS_STRING case doesn't want it.

>+        ok = JS_GetUCProperty(cx, obj, JS_GetStringChars(ks),
>+                              JS_GetStringLength(ks), &outputValue);

This ok assignment seems to want to accumulate a veto'ing false in ok, but carry on. I think again we want immediate failure on first JS API failure, with care taken to preserve the pending JS exception.

Here is where that tvr is needed, to protect outputValue.

>+      }
>+
>+      // if this is an array, holes are transmitted as null
>+      if (isArray && outputValue == JSVAL_VOID)
>+        outputValue = JSVAL_NULL;
>+      else if (JSVAL_IS_OBJECT(outputValue))
>+        outputValue = ToJSON(cx, outputValue);

So &outputValue, which is rooted by tvr, flows into the revised ToJSON.

>+      // elide undefined values
>+      if (ok && outputValue != JSVAL_VOID) {

No ok test here and again, continue if JSVAL_IS_VOID(outputValue) to unindent common-case code.

>+        // output a comma unless this is the first member to write
>+        if (memberWritten) {
>+          output = PRUnichar(',');
>+          rv = writer->Write(&output, 1);
>+        }
>+        memberWritten = PR_TRUE;
>+        
>+        JSType type = JS_TypeOfValue(cx, outputValue);
>+        if (NS_SUCCEEDED(rv) && type != JSTYPE_FUNCTION && type != JSTYPE_XML) {

Shouldn't failure in rv be propagated promptly?

Any type that can't be encoded should be an EncodingError, at least in a future revision. Or are we following the json.js precedent and silently dropping values that can't be encoded? If possible cast these out with early break or return.

>+          // If this isn't an array, we need to output a key
>+          if (!isArray) {
>+            nsAutoString keyOutput;
>+            JSString *keyString = nsnull;
>+            keyString = JS_ValueToString(cx, key);

No need for useless null init -- just set from JS_ValueToString.

Could use a common JSString *str local here and later.

>+            if (!keyString)
>+              break;
>+            rv = writer->WriteString((PRUnichar*)JS_GetStringChars(keyString),
>+                                     JS_GetStringLength(keyString));
>+            if (NS_FAILED(rv))
>+              break;
>+            output = PRUnichar(':');
>+            rv = writer->Write(&output, 1);
>+            if (NS_FAILED(rv))
>+              break;
>+          }

If the key contains \0, this will write out \u0000 -- which seems like invalid JSON. EncodingError?

>+          if (!JSVAL_IS_PRIMITIVE(outputValue)) {
>+            // recurse
>+            rv = EncodeObject(cx, JSVAL_TO_OBJECT(outputValue), writer,

Revised to pass &outputValue for formal jsval *vp parameter.

>+                              whitelist, depth + 1);
>+          } else {
>+            nsAutoString valueOutput;
>+            JSString *s = JS_ValueToString(cx, outputValue);
>+            if (!s) {
>+              return NS_ERROR_OUT_OF_MEMORY;
>+            }
>+            if (type == JSTYPE_STRING) {
>+              rv = writer->WriteString((PRUnichar*)JS_GetStringChars(s),
>+                                       JS_GetStringLength(s));

This will write a string with an embedded NUL char. EncodingError?

>+              continue;
>+            } else

No else after continue non-sequitur.

>         ... if (type == JSTYPE_NUMBER) {
>+              if (JSVAL_IS_DOUBLE(outputValue)) {
>+                jsdouble d = *JSVAL_TO_DOUBLE(outputValue);
>+                if (JSDOUBLE_IS_INFINITE(d))

Should use !JSDOUBLE_IS_FINITE(d) here to censor NaN too. Or really, throw EncodingError (RFC 4627 says "... are not permitted").

>+                  valueOutput.Append(NS_LITERAL_STRING("null"));
>+                else
>+                  valueOutput.Append((PRUnichar*)JS_GetStringChars(s));
>+              } else {
>+                valueOutput.Append((PRUnichar*)JS_GetStringChars(s));
>+              }
>+            } else if (type == JSTYPE_BOOLEAN) {
>+              valueOutput.Append((PRUnichar*)JS_GetStringChars(s));

Would like to common all these Append(chars-of-s) calls if possible.

>+            } else {
>+              valueOutput.Append(NS_LITERAL_STRING("null"));

EncodingError?

>+            }
>+
>+            rv = writer->Write(valueOutput.get(), valueOutput.Length());
>+          }
>+        }
>+      }
>+    }
>+  } while (key != JSVAL_HOLE && ok && NS_SUCCEEDED(rv));
>+
>+  ok = js_CloseIterator(cx, iterVal);
>+  if (!ok)
>+    rv = NS_ERROR_FAILURE;
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (isArray) {
>+    output = PRUnichar(']');
>+  } else {
>+    output = PRUnichar('}');
>+  }

Use ?: again.

>+jsval
>+nsJSON::ToJSON(JSContext *cx, jsval value)
>+{
>+  // Now we check to see whether the return value implements toJSON()
>+  jsval retval = value;
>+  JSBool ok = JS_FALSE;
>+  char *toJSON = "toJSON";
>+
>+  if (!JSVAL_IS_PRIMITIVE(value)) {
>+    JSObject *obj = JSVAL_TO_OBJECT(value);
>+    jsval toJSONVal = nsnull;
>+    ok = JS_GetProperty(cx, obj, toJSON, &toJSONVal);
>+    if (ok && (toJSONVal != JSVAL_VOID) &&

Is the JSVAL_VOID test a premature optimization?

Generally don't need () to protect ==/!=/etc. against &&/||.

>+        (JS_TypeOfValue(cx, toJSONVal) == JSTYPE_FUNCTION)) {
>+      ok = JS_CallFunctionValue(cx, obj, toJSONVal, 0, nsnull, &retval);

Could just call if you get an object value back. Callable objects are not all of type function according to typeof (cf. ES4 (x is Callable) structural type test).

>+    }
>+  }
>+
>+  if (!ok) {
>+    retval = nsnull;

JSVAL_NULL not nsnull, but with a jsval *vp in/out (true in and out) param, ToJSON can return boolean to reflect error explicitly.

>+  }
>+
>+  return retval;
>+}

Yeah, really want ToJSON to return JSBool or PRBool (bool in Moz2 ;-).

>+nsresult
>+nsJSONWriter::SetCharset(const char* aCharset)
>+{
>+  nsresult rv = NS_OK;
>+  if (mStream) {
>+    nsCOMPtr<nsICharsetConverterManager> ccm =
>+      do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = ccm->GetUnicodeEncoder(aCharset, getter_AddRefs(mEncoder));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = mEncoder->SetOutputErrorBehavior(nsIUnicodeEncoder::kOnError_Signal,
>+                                          nsnull, nsnull);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>+  return rv;
>+}
>+static const PRUnichar quote = PRUnichar('"');
>+static const PRUnichar backslash = PRUnichar('\\');
>+static const PRUnichar unicodeEscape[] = {'\\', 'u', '0', '0', '\0'};

Breathe with a blank line separating the methods from the statics here?

>+nsresult
>+nsJSONWriter::WriteString(const PRUnichar *aBuffer, PRUint32 aLength)
>+{
>+  nsresult rv;
>+  rv = Write(&quote, 1);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRUint32 mark = 0;
>+  PRUint32 i;
>+  for (i = 0; i < aLength; ++i) {
>+    if (aBuffer[i] == quote || aBuffer[i] == backslash) {
>+      rv = Write(&aBuffer[mark], i - mark);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = Write(&backslash, 1);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      rv = Write(&aBuffer[i], 1);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      mark = i + 1;
>+    } else if (aBuffer[i] <= 31 || aBuffer[i] == 127) {
>+      rv = Write(&aBuffer[mark], i - mark);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      nsAutoString unicode;
>+      unicode.Append(unicodeEscape);
>+      nsAutoString charCode;
>+      charCode.AppendInt(aBuffer[i], 16);
>+      if (charCode.Length() == 1)
>+        unicode.Append('0');
>+      unicode.Append(charCode);
>+      rv = Write(unicode.get(), unicode.Length());
>+      NS_ENSURE_SUCCESS(rv, rv);
>+      mark = i + 1;
>+    }

What about Unicode points > 127?

>+  //XXX this stream pattern should be consolidated in netwerk

FIXME citation to bug on file?

>+  rv = jsonListener->OnStartRequest(jsonChannel, nsnull);
>+  if (NS_FAILED(rv))
>+    jsonChannel->Cancel(rv);

Any point continuing here?

>+
>+  nsresult status;
>+  jsonChannel->GetStatus(&status);
>+  PRUint32 offset = 0;
>+  while (NS_SUCCEEDED(rv) && NS_SUCCEEDED(status)) {

With prompt failure propagation, this loop could test only status.

>+    PRUint32 available;
>+    rv = aStream->Available(&available);
>+    if (rv == NS_BASE_STREAM_CLOSED) {
>+      rv = NS_OK;
>+      available = 0;

Could just break here instead of available = 0 and flow thru.

>+    }
>+    if (NS_FAILED(rv)) {
>+      jsonChannel->Cancel(rv);
>+      break;
>+    }
>+    if (!available)
>+      break; // blocking input stream has none available when done
>+
>+    rv = jsonListener->OnDataAvailable(jsonChannel, nsnull,
>+                                       aStream, offset, available);
>+    if (NS_SUCCEEDED(rv))
>+      offset += available;
>+    else
>+      jsonChannel->Cancel(rv);

Invert rv test sense to cancel and bail?

>+    jsonChannel->GetStatus(&status);
>+  }
>+
>+  rv = jsonListener->OnStopRequest(jsonChannel, nsnull, status);

Failure in rv clobbered here? Or is it propagated to status via Cancel|GetStatus. Confusing, if so.

>+  jsval result = JSVAL_VOID;
>+
>+  if (NS_SUCCEEDED(rv)) {
>+    JSObject *obj = jsonListener->GetResult();
>+    if (obj)
>+      result = OBJECT_TO_JSVAL(obj);
>+  }
>+
>+  JSAutoTempValueRooter tvr(cx, 1, &result);
>+
>+  jsval *retvalPtr;
>+  rv = cc->GetRetValPtr(&retvalPtr);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  *retvalPtr = result;
>+  rv = cc->SetReturnValueWasSet(PR_TRUE);
>+  NS_ENSURE_SUCCESS(rv, rv);

Reorder this so the rval ptr is propagated down, to avoid the global GC root allocation for mRootObject altogether, as well as the tvr here.

>+nsJSONListener::~nsJSONListener()
>+{
>+  if (mRootObject && mCx) {
>+    JSAutoRequest ar(mCx);
>+    JS_RemoveRoot(mCx, &mRootObject);

Just FYI, avoid potential leaks via JS_RemoveRootRT passing in XPConnect's one true JSRuntime ptr.


>+NS_IMETHODIMP
>+nsJSONListener::OnStopRequest(nsIRequest *aRequest, nsISupports *aContext,
>+                              nsresult aStatusCode)
>+{
>+  nsresult rv;
>+
>+  // This can happen with short UTF-8 messages
>+  if (!mSniffBuffer.IsEmpty()) {
>+    rv = ProcessBytes(mSniffBuffer.get(), mSniffBuffer.Length());
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+
>+  if (mObjectStack.IsEmpty() && *mStatep == JSON_PARSE_STATE_FINISHED) {
>+    return NS_OK;
>+  }
>+
>+  return NS_ERROR_FAILURE;

Style nit: return failure in then clause with if condition inverted and De Morgan's theorem applied, unindent NS_OK return.

>+    // We should have a unicode charset by now
>+    rv = CheckCharset(charset.get());
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCOMPtr<nsICharsetConverterManager> ccm =
>+        do_GetService(NS_CHARSETCONVERTERMANAGER_CONTRACTID, &rv);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = ccm->GetUnicodeDecoderRaw(charset.get(), getter_AddRefs(mDecoder));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    // consume the sniffed bytes
>+    rv = ConsumeConverted(mSniffBuffer.get(), mSniffBuffer.Length());
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    mSniffBuffer.Truncate();

Could use a blank line (before the comment "consume the sniffed bytes" maybe) to breathe a bit.

>+nsresult
>+nsJSONListener::ConsumeConverted(const char* aBuffer, PRUint32 aByteLength)
>+{
>+  nsresult rv;
>+  PRInt32 unicharLength = 0;
>+  PRInt32 srcLen = aByteLength;
>+  rv = mDecoder->GetMaxLength(aBuffer, srcLen, &unicharLength);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  nsAutoArrayPtr<PRUnichar> ustr(new PRUnichar[unicharLength]);
>+  NS_ENSURE_TRUE(ustr, NS_ERROR_OUT_OF_MEMORY);
>+  rv = mDecoder->Convert(aBuffer, &srcLen, ustr, &unicharLength);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = Consume(ustr.get(), unicharLength);
>+  return rv;

Some blank lines here would aid readability IMHO.

I skimmed the parser, looks good.

>+JSBool
>+nsJSONListener::PushValue(JSObject *aParent, jsval *aValue)
>+{
>+  JSBool ok;
>+  if (JS_IsArrayObject(mCx, aParent)) {
>+    jsuint len;
>+    ok = JS_GetArrayLength(mCx, aParent, &len);
>+    if (ok) {
>+      ok = JS_SetElement(mCx, aParent, len, aValue);
>+    }
>+  } else {
>+    ok = JS_DefineUCProperty(mCx, aParent, (jschar *) mObjectKey.get(),
>+                             mObjectKey.Length(), *aValue,

Could pass jsval aValue, since it's not an in/out or out param, and just pass &aValue to JS_SetElement to meet that old API's form&fit (pre-historic JS allowed setters to mutate assignment op results -- verboten by ES1).

>+JSBool
>+nsJSONListener::PushObject(JSObject *aObj)
>+{
>+  if (mObjectStack.Length() >= JSON_MAX_DEPTH)
>+    return JS_FALSE;
>+
>+  JSBool ok;
>+  JSObject *parent = mObjectStack.ElementAt(mObjectStack.Length() - 1);
>+  jsval objVal = OBJECT_TO_JSVAL(aObj);
>+  ok = PushValue(parent, &objVal);
>+  return (ok && (mObjectStack.AppendElement(aObj)));

What protects objects on the stack, other than the root, from GC?

Suggest a tvr with a custom trace callback.

>+nsresult
>+nsJSONListener::OpenObject()
>+{
>+  JSBool ok = JS_TRUE;
>+  if (!mRootObject) {
>+    mRootObject = JS_NewObject(mCx, NULL, NULL, NULL);
>+    if (!mRootObject || !JS_AddRoot(mCx, &mRootObject) ||
>+        !mObjectStack.AppendElement(mRootObject))
>+      return NS_ERROR_OUT_OF_MEMORY;
>+  } else

else after return non-sequitur.

>      ... if (*mStatep == JSON_PARSE_STATE_VALUE ||
>+             *mStatep == JSON_PARSE_STATE_OBJECT) {
>+    JSObject *obj = JS_NewObject(mCx, NULL, NULL, NULL);
>+    if (!obj)
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    ok = PushObject(obj);
>+  } else {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  return ok ? NS_OK : NS_ERROR_FAILURE;

Arbitrary or incoherent control flow here -- with mRootObject elimination via retval ptr, and custom trace callback to protect all mObjectStack elements, this should simplify to cast errors out early. In light of custom tracer, don't need special case for null mRootObject. Also might do without early retval ptr access and propagation (revising my comments earlier), if you can avoid a GC hazard before the tracer has an mObjectStack to scan.

>+nsresult
>+nsJSONListener::OpenArray()
>+{
>+  JSBool ok = JS_TRUE;
>+  if (!mRootObject) {
>+    // Add a new root
>+    mRootObject = JS_NewArrayObject(mCx, 0, NULL);
>+    if (!mRootObject || !JS_AddRoot(mCx, &mRootObject) ||
>+        !mObjectStack.AppendElement(mRootObject))
>+      return NS_ERROR_OUT_OF_MEMORY;
>+  } else if (*mStatep == JSON_PARSE_STATE_VALUE ||
>+             *mStatep == JSON_PARSE_STATE_ARRAY) {
>+    // Add an array to an existing array or object
>+    JSObject *arr = JS_NewArrayObject(mCx, 0, NULL);
>+    if (!arr)
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    ok = PushObject(arr);
>+  } else {
>+    return NS_ERROR_FAILURE;
>+  }
>+  
>+  return ok ? NS_OK : NS_ERROR_FAILURE;

Similar comments here.

>+nsresult
>+nsJSONListener::HandleString()
>+{
>+  JSBool ok = JS_TRUE;
>+  if (*mStatep == JSON_PARSE_STATE_OBJECT_IN_PAIR) {
>+    mObjectKey = mStringBuffer;
>+  } else {
>+    JSObject *obj = mObjectStack.ElementAt(mObjectStack.Length() - 1);
>+    JSString *str = JS_NewUCStringCopyN(mCx, (jschar *) mStringBuffer.get(),
>+                                        mStringBuffer.Length());
>+    if (!str)
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    jsval strVal = STRING_TO_JSVAL(str);
>+    ok = PushValue(obj, &strVal);

Need to fret about GC-safety in PushValue now, since a setter on Array.prototype, e.g., could run and might be able to force a GC after disconnecting str from its newborn root.

>+nsresult
>+nsJSONListener::HandleNumber()
>+{
>+  JSBool ok = JS_TRUE;
>+  JSObject *obj = mObjectStack.ElementAt(mObjectStack.Length() - 1);
>+  JSString *str = JS_NewUCStringCopyN(mCx, (jschar *) mStringBuffer.get(),
>+                                      mStringBuffer.Length());
>+  if (!str)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  jsval strVal = STRING_TO_JSVAL(str);
>+  jsval numVal;
>+  ok = JS_ConvertValue(mCx, strVal, JSTYPE_NUMBER, &numVal);

Avoid string temporary and GC safety worries about str by going from JS_strtod, then JS_NewNumberValue.

>+  if (ok)
>+    ok = PushValue(obj, &numVal);

Again either PushValue must worry about rooting its value parameter, or it must oblige its callers to do so. If the former, could use a tvr in PushValue. The latter can win, as we saw with ToJSON, if the callers are all in one module, so in on the rules / easy to audit, and (also want this condition to apply) they have a root handy anyway. These Handle{String,Number} cases do not have a root handy, so simplest code would be to use a tvr in PushValue.

In summary, GC safety with exact GC and a C API (never mind an old API with design flaws yet to be fixed, such as the JS API!), combined with JS API and XPConnect mixed-layer coding, entails:

1. Prompt error propagation, taking care not to stomp on pending JS exception where set, otherwise making a good JS or XPC exception pend.

2. Rooting early and propagating local roots for use by an in-module control flow graph anchored to one or a few entry points.

3. Where possible and winning, root in caller. This conserves roots and also guarantees no hazard as a value passes out of a callee, via an unprotected out param or naked return value.

Whew! Sorry for delayed review, I will be faster next time.

/be

Comment 32

[Brendan Eich [:brendan]]

(In reply to comment #31)
> Local copy is probably just more register pressure for the compiler to hassle
> over, and sets of GC safety alerts -- avoid inputVal (see below for more).

s/sets of/&f/ of course.

/be

Comment 33

[Robert Sayre]

(In reply to comment #31)
> 
> >+  JSBool ok = JS_TRUE;
> >+  JSObject *iterObj = nsnull;
> >+  jsval iterVal = OBJECT_TO_JSVAL(obj);
> >+  JSAutoTempValueRooter tvr(cx, 1, &iterVal);
> 
> If the caller passes in a rooted jsval location by reference, then this is not
> necessary.

I've made most of the changes suggested, and rooting outputValue may still be necessary, but this doesn't seem right. The following will trigger a crash if iterVal isn't rooted, by disconnecting a nested object from the root:

var Ci = Components.interfaces;
var Cc = Components.classes;

var nativeJSON = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON);

function deletingIter(x) {
  return function() {
    yield "dd";
    print("after first yield");
    delete x["a"]["c"];
    gc();
    print("about to yield second");
    yield "ddddd";
  }
}

function deleteDuringEncode() {
  var x = {};
  x.a = {
    b: 1,
    bb: 2,
    bbb: 3,
    c: {
      cc: 2,
      ccc: 3,
      d: {
        dd: 2,
        ddd: 3,
        __iterator__: deletingIter(x),
        dddd: 4,
        ddddd: 5
      },
      cccc: 4,
      ccccc: 5
    },
    bbbb: 4,
    bbbbb: 5,
    bbbbbb: 6
  };
  var z = nativeJSON.encode(x);
  print(z);
  
}

deleteDuringEncode();

Comment 34

[Brendan Eich [:brendan]]

(In reply to comment #33)
> (In reply to comment #31)
> > 
> > >+  JSBool ok = JS_TRUE;
> > >+  JSObject *iterObj = nsnull;
> > >+  jsval iterVal = OBJECT_TO_JSVAL(obj);
> > >+  JSAutoTempValueRooter tvr(cx, 1, &iterVal);
> > 
> > If the caller passes in a rooted jsval location by reference, then this is not
> > necessary.
> 
> I've made most of the changes suggested,

Including passing a rooted jsval in by reference, and using that here, instead of the caller passing in obj?

> and rooting outputValue may still be necessary,

It is, no doubts -- right?

> but this doesn't seem right. The following will trigger a crash if
> iterVal isn't rooted,

Never said not to use a root -- suggested the caller passes it in instead of passing obj by value, and you use that rooted jsval instead of a tvr protecting iterVal.

If I'm missing something, perhaps shoot me the revised patch by email, or attach here?

/be

Comment 35

[Brendan Eich [:brendan]]

IOW, any control flow of the form:

obj = loadFromRoot();
foo(obj)
... init tvr to protect someVal
... someVal = OBJECT_TO_JSVAL(obj);
... getMeAnIteratorPlease(&someVal);
... stuff that might GC
... closeIterator(&someVal)

(where ... means code in foo nesting in the caller) is suspect. At the least, such a pattern overallocates a root (the inner one, tvr). It may leave other callers to foo erroneously failing to protect their actual obj params. The "caller roots" rule is better, as it avoids inadvertent GC hazards with "in" params, momentary hazards with "out"/"retval" params, plus it minimizes roots overall.

As noted, it's too bad the JS API does not set a good example here. We should change it to do so. As Igor advises, if compatibility must yield to safety and we're better off not keeping old APIs, so be it. But this is for other bugs, at least bug 313437 but probably another one not yet on file. Cc'ing Igor (I thought he was already cc'ed here for some reason).

/be

Comment 36

[Robert Sayre]

(In reply to comment #34)
> > 
> > I've made most of the changes suggested,
> 
> Including passing a rooted jsval in by reference, and using that here, instead
> of the caller passing in obj?

Right, that's why I thought it should work. Turns out the mistake was simple. I left in the iterVal local, when I should have been calling js_ValueToIterator on vp. Patch in a few.

> > and rooting outputValue may still be necessary,
> 
> It is, no doubts -- right?

Right.

Comment 37

[Robert Sayre]

Attachment: address brendan's comments

I think this covers all of the comments. The rooting in the decoder is pretty ugly, but seems to work. Maybe there is a prettier way?

>>+        (JS_TypeOfValue(cx, toJSONVal) == JSTYPE_FUNCTION)) {
>>+      ok = JS_CallFunctionValue(cx, obj, toJSONVal, 0, nsnull, &retval);
>
> Could just call if you get an object value back. Callable objects are not 
> all of type function according to typeof (cf. ES4 (x is Callable) structural 
> type test).

Crockford's code uses this test. Don't want compatiblity problems with
x = {"toJSON":{hi:5}}


> Any type that can't be encoded should be an EncodingError, at least in a 
> future revision. Or are we following the json.js precedent and silently 
> dropping values that can't be encoded?

Silently dropping. Don't want to require callers to check for full JSON fidelity before calling. If they do want it, they can check for themselves ahead of time. Exceptions should be reserved for things like resource
consumption errors.

> If the key contains \0, this will write out \u0000 -- which seems like 
> invalid JSON. EncodingError?
...
> This will write a string with an embedded NUL char. EncodingError?

\u0000 is valid--but maybe I'm not seeing the bug?


>>         ... if (type == JSTYPE_NUMBER) {
>>+              if (JSVAL_IS_DOUBLE(outputValue)) {
>>+                jsdouble d = *JSVAL_TO_DOUBLE(outputValue);
>>+                if (JSDOUBLE_IS_INFINITE(d))
>
> Should use !JSDOUBLE_IS_FINITE(d) here to censor NaN too. Or really, throw
> EncodingError (RFC 4627 says "... are not permitted").

null output matches Crockford. Another case were we don't want to require callers to check for full JSON fidelity before calling.


>>+                  valueOutput.Append(NS_LITERAL_STRING("null"));
>>+                else
>>+                  valueOutput.Append((PRUnichar*)JS_GetStringChars(s));
>>+              } else {
>>+                valueOutput.Append((PRUnichar*)JS_GetStringChars(s));
>>+              }
>>+            } else if (type == JSTYPE_BOOLEAN) {
>>+              valueOutput.Append((PRUnichar*)JS_GetStringChars(s));
>
> Would like to common all these Append(chars-of-s) calls if possible.

Could reorder, but I think it would make the choices harder to decipher.


>>+nsresult
>>+nsJSONWriter::WriteString(const PRUnichar *aBuffer, PRUint32 aLength)
>>+{
>
> What about Unicode points > 127?
>

If the target is a string, these are just appended. If the target is a stream, they are converted to the proper character set in WriteToStream. The purpose of WriteString is to avoid problems with quotes and escapes, and to prevent the generation of UTF-8 streams containing ASCII control characters. The latter effort is not strictly necessary, but it eliminates many attacks if
we hook up something like JSONRequest (where we would force the request charset to UTF-8).


>>+  //XXX this stream pattern should be consolidated in netwerk
>
> FIXME citation to bug on file?

peterv mentioned one. I will find out what it is.

Comment 38

[Brendan Eich [:brendan]]

Comment on attachment 293560 [details] [diff] [review]
address brendan's comments

Sorry, I was mistaken in thinking embedded NULs were not allowed in JSON identifiers.

>+  JSBool ok = ToJSON(cx, &argv[firstArg]);
>+  JSType type;
>+  if (!(ok && !JSVAL_IS_PRIMITIVE(argv[firstArg]) &&
>+        (type = JS_TypeOfValue(cx, argv[firstArg])) && 
>+        type != JSTYPE_FUNCTION && type != JSTYPE_XML)) {
>+    return NS_ERROR_INVALID_ARG;
>+  }
>+
>+  return EncodeObject(cx, &argv[firstArg], writer, whitelist, 0);

Only thought here is to use jsval *vp = &argv[firstArg] to common explicitly and use the canonical name (if it's available in this scope).


>+    if (key == JSVAL_HOLE)
>+      continue;

This may as well break to avoid forcing the compiler to optimize away the redundant JSVAL_HOLE test in the loop condition (which seems unnecessary if this breaks).

>+    jsval outputValue = JSVAL_VOID;
>+    JSAutoTempValueRooter tvr(cx, 1, &outputValue);
>+
>+    JSString *ks;
>+    if (JSVAL_IS_STRING(key)) {
>+      ks = JSVAL_TO_STRING(key);
>+    } else {
>+      ks = JS_ValueToString(cx, key);
>+      if (!ks) {
>+        ok = JS_FALSE;
>+        break;
>+      }
>+    }
>+
>+    ok = JS_GetUCProperty(cx, obj, JS_GetStringChars(ks),
>+                          JS_GetStringLength(ks), &outputValue);

No need for the outputValue decl and tvr until just before the JS_GetUCProperty -- nit, but if JS_ValueToString did cause a GC, we'd avoid scanning JSVAL_VOID.

>+  } while (key != JSVAL_HOLE && ok && NS_SUCCEEDED(rv));

Looks like the loop cond should test only NS_SUCCEEDED(rv).

>+  if (iterObj)
>+    ok = js_CloseIterator(cx, * vp);

This could still clobber a false ok, it seems to me.

>+  if (!ok)
>+    rv = NS_ERROR_FAILURE; // encoding error or propagate?
>+  NS_ENSURE_SUCCESS(rv, rv);

Do we know (I used to, but I forget now) that XPConnect will do the right thing with pending JS exception already set due to !ok, instead of clobbering that with a rv-based exception? Check with jst and mrbkap.

>+typedef struct TempDecodeStackRoot {
>+  JSTempValueRooter tvr;
>+  nsTArray<JSObject *> *stack;
>+} TempDecodeStackRoot;
>+
>+class nsJSONListener : public nsIStreamListener
>+{
. . .
>+  nsTArray<JSObject *> mObjectStack;
>+  TempDecodeStackRoot *mTDSR;

Why not avoid the extra member word allocated for stack in mTDSR so it can point to mObjectStack, and just make the nsTArray<JSObject *> a member of mTDSR?

/be

Comment 39

[Brendan Eich [:brendan]]

Looks good otherwise, a once-over from Igor would be great. Explicit GC rooting is never pretty, but this is about as good as it gets. That JSString *s may have a long-ish live range, but no call-outs jeopardize its newborn root. Might want to comment it being unrooted otherwise, as a warning to anyone who comes later.

/be

Comment 40

[Robert Sayre]

(In reply to comment #38)
> (From update of attachment 293560 [details] [diff] [review])
> 
> >+  if (!ok)
> >+    rv = NS_ERROR_FAILURE; // encoding error or propagate?
> >+  NS_ENSURE_SUCCESS(rv, rv);
> 
> Do we know (I used to, but I forget now) that XPConnect will do the right thing
> with pending JS exception already set due to !ok, instead of clobbering that
> with a rv-based exception? Check with jst and mrbkap.

It may have been recently patched to handle that correctly. However, I should have put a FIXME in the patch saying I wasn't going to try and get exception types correct this round. I'll have a followup for the DOM binding, and I'll check them there. Sound ok?

Comment 41

[Brendan Eich [:brendan]]

(In reply to comment #38)

> >+    jsval outputValue = JSVAL_VOID;
> >+    JSAutoTempValueRooter tvr(cx, 1, &outputValue);
. . .
> No need for the outputValue decl and tvr until just before the JS_GetUCProperty
> -- nit, but if JS_ValueToString did cause a GC, we'd avoid scanning JSVAL_VOID.

On second thought, who cares about a little extra mark-phase work, compared to time encoding? This root could be hoisted, and the tvr pushing and popping too, for once per ply of object properties, instead of tvr'ing in the loop body.

> It may have been recently patched to handle that correctly. However, I should
> have put a FIXME in the patch saying I wasn't going to try and get exception
> types correct this round. I'll have a followup for the DOM binding, and I'll
> check them there. Sound ok?

Sure.

/be

Comment 42

[Robert Sayre]

Attachment: address more of brendan's comments

Comment 43

[Brendan Eich [:brendan]]

Comment on attachment 293691 [details] [diff] [review]
address more of brendan's comments

>+  // XXX FIXME: bug 408838. Get exception types sorted out

Just an aside: FIXME should be enough (XXX is overused in my experience, not a good grep pattern).

>+  if (!(ok && !JSVAL_IS_PRIMITIVE(*vp) &&
>+        (type = JS_TypeOfValue(cx, *vp)) && 

(Nit: trailing space on last line above -- search and destroy elsewhere... ;-)

Oops, missed this: what is the non-zero test of type for? All you want is to nest the assignment to type in the next condition's left != term:

>+        type != JSTYPE_FUNCTION && type != JSTYPE_XML)) {

Or, if you really mean to test against JSTYPE_VOID, say that explicitly.

>+    // Be careful below, this string is unrooted.

"weakly rooted" or "only newborn rooted" would be more precise, FWIW.

>+  // Always close the iterator, but make sure not to stomp on OK
>+  JSBool closeOK = JS_TRUE;
>+  if (iterObj)
>+    closeOK = js_CloseIterator(cx, * vp);  
>+  ok = ok ? closeOK : ok;

The last should be just ok = closeOK && ok, but that suggests a simpler approach we use in the JS engine, and others have used with nsresults (to accumulate the sign bit):

  if (iterObj)
    ok &= js_CloseIterator(cx, *vp);

to avoid closeOK altogether.

(Nit about space after * before vp.)

>+typedef struct TempDecodeStackRoot {
>+  JSTempValueRooter tvr;
>+  nsTArray<JSObject *> stack;
>+} TempDecodeStackRoot;

But now this is more than a temporary root, it's the object array stack thingie. Suggest new type and instance variable name to match (nsJSONObjectStack, mObjectStack?). The GC-tracing safety is a detail -- important but not decisive for the name.

Can you inherit from nsTArray<JSObject *> to avoid explicitly forwarding to the member? That would rule.

Looks good to go otherwise.

/be

Comment 44

[Brendan Eich [:brendan]]

Two more things:

iterObj is null-initialized for no reason, never used before set again (after an early error return). Lose this useless initialiser.

After iterObj is set, it is assumed (correctly) non-null (no need to check or assert this), so the

  if (iterObj)
    ok &= js_CloseIterator(cx, iterObj);

should lose the if (iterObj).

/be

Comment 45

[Robert Sayre]

Attachment: address more of brendan's comments

Comment 46

[Brendan Eich [:brendan]]

Comment on attachment 293742 [details] [diff] [review]
address more of brendan's comments

>+  nsJSONObjectStack *tmp = (nsJSONObjectStack *)tvr;
. . .
>+class nsJSONObjectStack : public nsTArray<JSObject *>
>+{
>+public:
>+  JSTempValueRooter tvr;

Is this right? C-style cast instead of static_cast, I mean? If C++ puts the nsTArray<JSObject *> first and the tvr member second, a reinterpret cast would be bad.

/be

Comment 47

[Robert Sayre]

Attachment: fix cast

Comment 48

[Brendan Eich [:brendan]]

Comment on attachment 293770 [details] [diff] [review]
fix cast

Looks good, if you want to iterate for better HandleNubmer / PushValue GC paranoia, feel free to do another patch.

One final nit: the nsJSONListener dtor does not need to null-check mCx; the ctor should insist cx is non-null and the one caller requires that anyway. All friends here so an assertion is enough.

/be

Comment 49

[Robert Sayre]

Attachment: updated for checkin

I didn't figure out a way to change the decoder rooting without creating new problems, so we'll stick with what I have for now.

Comment 50

[Robert Sayre]

Marking fixed. New patches can go in new bugs. Bug 410005 tracks a test failure for Windows outputstreams.

Comment 51

[Bob Clary [:bc] (inactive)]

rsayre added the tests to dom/src/json/test

Comment 52

[Eric Shepherd [:sheppy]]

An initial draft of documentation for this is at:

http://developer.mozilla.org/en/docs/nsIJSON

There are a couple of issues with the doc (I don't know what "whitelist" and "optFilter" parameters are for; the tests don't make it clear, and neither does the source).  Also, I need to put in a couple more samples, but I hesitate to do that until I understand those two parameters.  If anyone out there does, please let me know.