Our native JSON API seems to allow for a (currently defective) whitelist argument. For SessionStore a blacklist would be handier, though, as we've got quite many different allowed keys and just two exceptions. Instead of adding an additional blacklist parameter, I'd be fine with a slightly more general approach to white-/blacklisting: * either accept a function which takes key name and value and either returns a new value or |undefined| if the key is to be omitted (this would also make a nice filter for e.g. serializing Dates or other Objects to strings on the fly) * or accept a regex which key names must pass (a blacklist would then only consist of a negative look-ahead anchored at the string's start) One or even both of these options could be used in addition to the whitelist array that's already spec'd.

We will follow the standard settled upon by the ES4/ES3.1 ECMA group. That will settle soon.

Robert: Has the API still not been settled upon for ES3.1, do we deem it inappropriate or do you still plan to eventually ship the same API as IE8 will?

(In reply to comment #2) > Robert: Has the API still not been settled upon for ES3.1, do we deem it > inappropriate or do you still plan to eventually ship the same API as IE8 will? I think it was finally settled at the Sep 22 meeting.

What's the latest status here, please?

Robert: Do you expect this bug to be fixed in time for Firefox 3.1 or should I implement a work-around for fixing bug 407110 (which I'd really like to see fixed)?

The current ES3.1 proposal has an optional function which can transform the data before JSON operates on it. Sayrer, is this something we need to get in before final?

(In reply to comment #6) > The current ES3.1 proposal has an optional function which can transform the > data before JSON operates on it. Sayrer, is this something we need to get in > before final? Yes, it's on my desk, so to speak.

JSON now lives in the JS Engine.

No patch here, and though I'm told one's waiting, it would need to be reviewed by today to get into b4. Should we be pushing this out of scope for 3.5 and taking it off the blocker list?

(In reply to comment #9) > No patch here, and though I'm told one's waiting, it would need to be reviewed > by today to get into b4. Should we be pushing this out of scope for 3.5 and > taking it off the blocker list? No.

Refactored to match ES5, almost ready, few pretty print things to do. more in a bit.

need to finish the gap/indent junk they added.

this is pretty fast. needs to add the stack bit from the spec, then it's done.

this can get reviewed while I add the stack piece, which won't be a lot of code

The gist of this patch is that it splits up stringify in to Str, JA, and JO (names from spec). This reorganization is needed to accommodate the specified replacer/indent bells and whistles. For example, an array replacer parameter is a whitelist for objects, but not arrays.

Looking, reading the part of the spec I'd put off till now. /be

Comment on attachment 374076 [details] [diff] [review] add replacer and whitespace stuff, per ES5 >+ ok = JS_Stringify(cx, vp, JSVAL_NULL, JSVAL_NULL, &WriteCallback, writer); Nit: no need for unary-& before function names, they are pointers already. >-JS_Stringify(JSContext *cx, jsval *vp, JSObject *replacer, >+JS_Stringify(JSContext *cx, jsval *vp, jsval replacer, jsval space, > JSONWriteCallback callback, void *data) Seems like the more precise type for replacer is JSObject * -- either way, caller must root. But since the spec requires either a function or an array object, jsval is too broad. If reviver can be more precisely typed in the JS API, it should be too. Otherwise, are you trying to maintain symmetry between reviver and replacer? >+ // XXX This can never happen to nsJSON.cpp, but the JSON object Nit/question: is nsJSON.cpp a left-over? >+ // needs to support returning undefined. So this is a little awkward >+ // for the API, because we want to support streaming writers. >+ if (wc.didWrite) { >+ JSStringBuffer *sb = &wc.sb; >+ JSString *s = JS_NewUCStringCopyN(cx, sb->base, STRING_BUFFER_OFFSET(sb)); >+ if (!s) >+ return JS_FALSE; >+ *vp = STRING_TO_JSVAL(s); >+ } > >+ else >+ *vp = JSVAL_VOID; Nit: Prevailing style cuddles else to previous } and braces both clauses of if-else if either is braced. >@@ -200,222 +210,408 @@ write_string(JSContext *cx, JSONWriteCal > return JS_FALSE; > > if (!callback(&quote, 1, data)) > return JS_FALSE; > > return JS_TRUE; Just return callback(&quote, 1, data); here. >+JO(JSContext *cx, jsval *vp, StringifyContext *scx) > { >+ JSObject *obj = JSVAL_TO_OBJECT(*vp); > >+ jschar c = jschar('{'); >+ if (!scx->callback(&c, 1, scx->data)) > return JS_FALSE; > > jsval outputValue = JSVAL_VOID; > JSAutoTempValueRooter tvr(cx, 1, &outputValue); > >+ JSObject *iterObj = NULL; >+ jsval *keySource = vp; keySource must point to a rooted jsval. It starts out equal to vp, which is a root. >+ JSBool usingWhitelist = JS_FALSE; >+ >+ // if the replacer is an array, we use the keys from it >+ if (!JSVAL_IS_PRIMITIVE(*scx->replacer) && JS_IsArrayObject(cx, JSVAL_TO_OBJECT(*scx->replacer))) { >+ usingBlacklist = JS_TRUE; BTW, we've been using true and false with good success; bool too for types other than passed-by-explicit-reference parameters, and return values. >+ keySource = scx->replacer; But who makes sure the GC will scan the word addressed by scx->replacer? More below. >+ } >+ >+ if (!js_ValueToIterator(cx, JSITER_ENUMERATE, keySource)) >+ return JS_FALSE; >+ iterObj = JSVAL_TO_OBJECT(*keySource); >+ > jsval key; Who roots key? > JSBool memberWritten = JS_FALSE; >+ JSBool ok; >+ > do { > outputValue = JSVAL_VOID; >+ ok = js_CallIteratorNext(cx, iterObj, &key); >+ if (!ok) >+ break; >+ if (key == JSVAL_HOLE) >+ break; > >+ jsuint index = 0; >+ if (usingWhitelist) { >+ // skip non-index properties >+ if (!js_IdIsIndex(key, &index)) >+ continue; >+ >+ jsval newKey; >+ if (!OBJ_GET_PROPERTY(cx, JSVAL_TO_OBJECT(*scx->replacer), key, &newKey)) > return JS_FALSE; Here it is important for key to be rooted, since a resolve hook could run GC. >+ key = newKey; newKey must be rooted. >+ } >+ >+ JSString *ks; >+ if (JSVAL_IS_STRING(key)) { >+ ks = JSVAL_TO_STRING(key); > } else { >+ ks = js_ValueToString(cx, key); Here ks could be a conversion of non-string jsval 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) || This could do a last-ditch GC and collect ks. >+ !js_HasOwnProperty(cx, obj->map->ops->lookupProperty, obj, id, &v)) { This could do a last-ditch GC too, which would not collect id but could collect ks. >+ ok = JS_FALSE; >+ break; >+ } >+ >+ if (v != JSVAL_TRUE) >+ continue; >+ >+ ok = JS_GetPropertyById(cx, obj, id, &outputValue); >+ if (!ok) >+ break; ks could be collected under JS_GetPropertyById. If ks is an atomized string, ks == id. >+ >+ if (JSVAL_IS_OBJECT(outputValue)) { > ok = js_TryJSON(cx, &outputValue); > if (!ok) > break; > } > > JSType type = JS_TypeOfValue(cx, outputValue); >- This blank line seems better for readability than its lack, leading smack into: >+ >+ if (memberWritten && !WriteIndent(cx, scx, scx->depth - 1)) >+ return JS_FALSE; >+ >+ c = jschar('}'); >+ if (!scx->callback(&c, 1, scx->data)) >+ return JS_FALSE; > >+ return JS_TRUE; Just return scx->callback(...); again. >+ if (outputValue == JSVAL_VOID) { >+ JSString *s = JS_NewStringCopyN(cx, "null", 4); >+ if (!s) >+ return JS_FALSE; >+ if (!scx->callback(JS_GetStringChars(s), JS_GetStringLength(s), scx->data)) >+ return JS_FALSE; Why create a new JSString here? Just use a static const jschar null_ucstr[] = {'n', 'u', 'l', 'l'}; and use that instead. >+ } >+ >+ if (i < (length - 1)) { Nit: don't overparenthesize - against <. >+ c = jschar(','); >+ if (!scx->callback(&c, 1, scx->data)) >+ return JS_FALSE; >+ if (!WriteIndent(cx, scx, scx->depth)) >+ return JS_FALSE; >+ } >+ } >+ >+ if (length > 0 && !WriteIndent(cx, scx, scx->depth - 1)) Nit: for unsigned length, length != 0 is better, since length can't be < 0. >+ return JS_FALSE; >+ >+ c = jschar(']'); >+ if (!scx->callback(&c, 1, scx->data)) >+ return JS_FALSE; >+ >+ return JS_TRUE; return scx->callback(&c, 1, scx->data); >+} >+ >+static JSBool >+Str(JSContext *cx, jsval *vp, jsid id, JSObject *holder, StringifyContext *scx) Nit: usually we put the out-params last, so vp at end? >+ if (!JSVAL_IS_PRIMITIVE(*scx->replacer) && js_IsCallable(JSVAL_TO_OBJECT(*scx->replacer), cx)) { >+ jsval value = *vp; >+ jsval vec[2] = {ID_TO_VALUE(id), value}; >+ if (!JS_CallFunctionValue(cx, holder, *scx->replacer, 2, vec, vp)) >+ return JS_FALSE; The jsval value = *vp inits a single-use variable (value) and obscures the requirement that vp is a root. Remove... >+ // catches string and number objects with no toJSON >+ if (!JSVAL_IS_PRIMITIVE(*vp)) { >+ JSClass *clasp = OBJ_GET_CLASS(cx, JSVAL_TO_OBJECT(*vp)); >+ >+ if (clasp == &js_StringClass) { >+ JSString *s = js_ValueToString(cx, *vp); >+ if (!s) >+ return JS_FALSE; >+ *vp = STRING_TO_JSVAL(s); >+ } else if (clasp == &js_NumberClass) { >+ jsdouble d = js_ValueToNumber(cx, vp); >+ if (JSVAL_IS_NULL(*vp)) >+ return JS_FALSE; >+ *vp = DOUBLE_TO_JSVAL(&d); >+ } Simplify this to a single if testing for either clasp value, then just *vp = JSVAL_TO_OBJECT(*vp)->fslots[JSSLOT_PRIVATE]; >+ } >+ >+ JSType type = JS_TypeOfValue(cx, *vp); >+ JSString *s; >+ if (type == JSTYPE_STRING) { >+ s = js_ValueToString(cx, *vp); No need for JS_TypeOfValue or js_ValueToString, just test JSVAL_IS_STRING and use JSVAL_TO_STRING. >+ if (!s) >+ return JS_FALSE; Avoiding this dead error check. >+ return write_string(cx, scx->callback, scx->data, JS_GetStringChars(s), JS_GetStringLength(s)); >+ } >+ >+ if (JSVAL_IS_NULL(*vp)) { >+ s = JS_NewStringCopyN(cx, "null", 4); >+ if (!s) >+ return JS_FALSE; >+ return scx->callback(JS_GetStringChars(s), JS_GetStringLength(s), scx->data); Ditto above comment about null_ucstr -- make it file-static so it can be used anywhere? >+ } >+ >+ if (type == JSTYPE_BOOLEAN) { JSVAL_IS_BOOLEAN(*vp) >+ s = js_ValueToString(cx, *vp); >+ if (!s) >+ return JS_FALSE; >+ return scx->callback(JS_GetStringChars(s), JS_GetStringLength(s), scx->data); Similarly: const jschar *boolean_ucstr[] = {{'f','a','l','s','e'}, {'t','r','u','e'}}; and index using JSVAL_TO_BOOLEAN(*vp). >+ } >+ >+ if (type == JSTYPE_NUMBER) { Just test JSVAL_IS_NUMBER(*vp). >+ if (JSVAL_IS_DOUBLE(*vp)) { >+ jsdouble d = *JSVAL_TO_DOUBLE(*vp); >+ if (!JSDOUBLE_IS_FINITE(d)) { >+ s = JS_NewStringCopyN(cx, "null", 4); >+ return scx->callback(JS_GetStringChars(s), JS_GetStringLength(s), scx->data); null_ucstr, no new string assigned to s. >+ } >+ } >+ >+ s = js_ValueToString(cx, *vp); Avoid allocating a JSString from the GC heap, use JS_dtostr and inflate to a jschar stack buffer, since we have an upper bound on length of result string. >+ if (!s) >+ return JS_FALSE; >+ return scx->callback(JS_GetStringChars(s), JS_GetStringLength(s), scx->data); >+ } >+ >+ if (JSVAL_IS_OBJECT(*vp) && type != JSTYPE_FUNCTION && type != JSTYPE_XML) { Use VALUE_IS_FUNCTION and VALUE_IS_XML to exclude. Have to break here, more soon. New patch welcome. /be

Comment on attachment 374076 [details] [diff] [review] add replacer and whitespace stuff, per ES5 > // output a comma unless this is the first member to write > if (memberWritten) { >+ c = jschar(','); >+ ok = scx->callback(&c, 1, scx->data); > if (!ok) > break; > } > memberWritten = JS_TRUE; > >+ // Be careful below, this string is weakly rooted > JSString *s; >+ s = js_ValueToString(cx, key); Initialize the declaration, and move this down (see below) to shorten the live range of weakly-rooted s. >+ if (!s) { >+ ok = JS_FALSE; >+ break; > } > >+ if (!WriteIndent(cx, scx, scx->depth)) >+ return JS_FALSE; Move the indentation writing up to after the comma writing code, before js_ValueToString. >+ >+ ok = write_string(cx, scx->callback, scx->data, JS_GetStringChars(s), JS_GetStringLength(s)); >+ if (!ok) >+ break; IINM s has no more uses here, so the only GC safety thread could be under write_string. It's clear from the above that key needs to be rooted too, so instead of outputValue, may as well use jsval pair[2]; rooted by the tvr, with pair[0] the key and pair[1] the (output) value. /be

(In reply to comment #17) > > Seems like the more precise type for replacer is JSObject * -- either way, > caller must root. But since the spec requires either a function or an array > object, jsval is too broad. > > If reviver can be more precisely typed in the JS API, it should be too. > Otherwise, are you trying to maintain symmetry between reviver and replacer? See comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=476374#c10 Narrowing type to JSObject makes it possible to lose closure information. > > >+ // XXX This can never happen to nsJSON.cpp, but the JSON object > > Nit/question: is nsJSON.cpp a left-over? No, it's stating that the COM functions in nsJSON.cpp will never hit this condition, because they don't allow primitive arguments, only things that start with "{" and "[". > >+ // needs to support returning undefined. So this is a little awkward > >+ // for the API, because we want to support streaming writers. > >+ if (wc.didWrite) { > >+ JSStringBuffer *sb = &wc.sb; > >+ JSString *s = JS_NewUCStringCopyN(cx, sb->base, STRING_BUFFER_OFFSET(sb)); > >+ if (!s) > >+ return JS_FALSE; > >+ *vp = STRING_TO_JSVAL(s); > >+ } > > > >+ else > >+ *vp = JSVAL_VOID; > > Nit: Prevailing style cuddles else to previous } and braces both clauses of > if-else if either is braced. Fixed. > >@@ -200,222 +210,408 @@ write_string(JSContext *cx, JSONWriteCal > > return JS_FALSE; > > > > if (!callback(&quote, 1, data)) > > return JS_FALSE; > > > > return JS_TRUE; > > Just return callback(&quote, 1, data); here. Fixed. > >+JO(JSContext *cx, jsval *vp, StringifyContext *scx) > > { > >+ JSObject *obj = JSVAL_TO_OBJECT(*vp); > > > >+ jschar c = jschar('{'); > >+ if (!scx->callback(&c, 1, scx->data)) > > return JS_FALSE; > > > > jsval outputValue = JSVAL_VOID; > > JSAutoTempValueRooter tvr(cx, 1, &outputValue); > > > >+ JSObject *iterObj = NULL; > >+ jsval *keySource = vp; > > keySource must point to a rooted jsval. It starts out equal to vp, which is a > root. > > >+ JSBool usingWhitelist = JS_FALSE; > >+ > >+ // if the replacer is an array, we use the keys from it > >+ if (!JSVAL_IS_PRIMITIVE(*scx->replacer) && JS_IsArrayObject(cx, JSVAL_TO_OBJECT(*scx->replacer))) { > >+ usingBlacklist = JS_TRUE; > > BTW, we've been using true and false with good success; bool too for types > other than passed-by-explicit-reference parameters, and return values. > > >+ keySource = scx->replacer; > > But who makes sure the GC will scan the word addressed by scx->replacer? More > below. > > >+ } > >+ > >+ if (!js_ValueToIterator(cx, JSITER_ENUMERATE, keySource)) > >+ return JS_FALSE; > >+ iterObj = JSVAL_TO_OBJECT(*keySource); > >+ > > jsval key; > > Who roots key? > > > JSBool memberWritten = JS_FALSE; > >+ JSBool ok; > >+ > > do { > > outputValue = JSVAL_VOID; > >+ ok = js_CallIteratorNext(cx, iterObj, &key); > >+ if (!ok) > >+ break; > >+ if (key == JSVAL_HOLE) > >+ break; > > > >+ jsuint index = 0; > >+ if (usingWhitelist) { > >+ // skip non-index properties > >+ if (!js_IdIsIndex(key, &index)) > >+ continue; > >+ > >+ jsval newKey; > >+ if (!OBJ_GET_PROPERTY(cx, JSVAL_TO_OBJECT(*scx->replacer), key, &newKey)) > > return JS_FALSE; > > Here it is important for key to be rooted, since a resolve hook could run GC. > > >+ key = newKey; > > newKey must be rooted. > > >+ } > >+ > >+ JSString *ks; > >+ if (JSVAL_IS_STRING(key)) { > >+ ks = JSVAL_TO_STRING(key); > > } else { > >+ ks = js_ValueToString(cx, key); > > Here ks could be a conversion of non-string jsval 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) || > > This could do a last-ditch GC and collect ks. > > >+ !js_HasOwnProperty(cx, obj->map->ops->lookupProperty, obj, id, &v)) { > > This could do a last-ditch GC too, which would not collect id but could collect > ks. > > >+ ok = JS_FALSE; > >+ break; > >+ } > >+ > >+ if (v != JSVAL_TRUE) > >+ continue; > >+ > >+ ok = JS_GetPropertyById(cx, obj, id, &outputValue); > >+ if (!ok) > >+ break; > > ks could be collected under JS_GetPropertyById. If ks is an atomized string, ks > == id. > > >+ > >+ if (JSVAL_IS_OBJECT(outputValue)) { > > ok = js_TryJSON(cx, &outputValue); > > if (!ok) > > break; > > } > > > > JSType type = JS_TypeOfValue(cx, outputValue); > >- > > This blank line seems better for readability than its lack, leading smack into: > > >+ > >+ if (memberWritten && !WriteIndent(cx, scx, scx->depth - 1)) > >+ return JS_FALSE; > >+ > >+ c = jschar('}'); > >+ if (!scx->callback(&c, 1, scx->data)) > >+ return JS_FALSE; > > > >+ return JS_TRUE; > > Just return scx->callback(...); again. > > >+ if (outputValue == JSVAL_VOID) { > >+ JSString *s = JS_NewStringCopyN(cx, "null", 4); > >+ if (!s) > >+ return JS_FALSE; > >+ if (!scx->callback(JS_GetStringChars(s), JS_GetStringLength(s), scx->data)) > >+ return JS_FALSE; > > Why create a new JSString here? Just use a static const jschar null_ucstr[] = > {'n', 'u', 'l', 'l'}; and use that instead. > > >+ } > >+ > >+ if (i < (length - 1)) { > > Nit: don't overparenthesize - against <. > > >+ c = jschar(','); > >+ if (!scx->callback(&c, 1, scx->data)) > >+ return JS_FALSE; > >+ if (!WriteIndent(cx, scx, scx->depth)) > >+ return JS_FALSE; > >+ } > >+ } > >+ > >+ if (length > 0 && !WriteIndent(cx, scx, scx->depth - 1)) > > Nit: for unsigned length, length != 0 is better, since length can't be < 0. > > >+ return JS_FALSE; > >+ > >+ c = jschar(']'); > >+ if (!scx->callback(&c, 1, scx->data)) > >+ return JS_FALSE; > >+ > >+ return JS_TRUE; > > return scx->callback(&c, 1, scx->data); > > >+} > >+ > >+static JSBool > >+Str(JSContext *cx, jsval *vp, jsid id, JSObject *holder, StringifyContext *scx) > > Nit: usually we put the out-params last, so vp at end? > > >+ if (!JSVAL_IS_PRIMITIVE(*scx->replacer) && js_IsCallable(JSVAL_TO_OBJECT(*scx->replacer), cx)) { > >+ jsval value = *vp; > >+ jsval vec[2] = {ID_TO_VALUE(id), value}; > >+ if (!JS_CallFunctionValue(cx, holder, *scx->replacer, 2, vec, vp)) > >+ return JS_FALSE; > > The jsval value = *vp inits a single-use variable (value) and obscures the > requirement that vp is a root. Remove... > > >+ // catches string and number objects with no toJSON > >+ if (!JSVAL_IS_PRIMITIVE(*vp)) { > >+ JSClass *clasp = OBJ_GET_CLASS(cx, JSVAL_TO_OBJECT(*vp)); > >+ > >+ if (clasp == &js_StringClass) { > >+ JSString *s = js_ValueToString(cx, *vp); > >+ if (!s) > >+ return JS_FALSE; > >+ *vp = STRING_TO_JSVAL(s); > >+ } else if (clasp == &js_NumberClass) { > >+ jsdouble d = js_ValueToNumber(cx, vp); > >+ if (JSVAL_IS_NULL(*vp)) > >+ return JS_FALSE; > >+ *vp = DOUBLE_TO_JSVAL(&d); > >+ } > > Simplify this to a single if testing for either clasp value, then just > > *vp = JSVAL_TO_OBJECT(*vp)->fslots[JSSLOT_PRIVATE]; > > > >+ } > >+ > >+ JSType type = JS_TypeOfValue(cx, *vp); > >+ JSString *s; > >+ if (type == JSTYPE_STRING) { > >+ s = js_ValueToString(cx, *vp); > > No need for JS_TypeOfValue or js_ValueToString, just test JSVAL_IS_STRING and > use JSVAL_TO_STRING. > > >+ if (!s) > >+ return JS_FALSE; > > Avoiding this dead error check. > > >+ return write_string(cx, scx->callback, scx->data, JS_GetStringChars(s), JS_GetStringLength(s)); > >+ } > >+ > >+ if (JSVAL_IS_NULL(*vp)) { > >+ s = JS_NewStringCopyN(cx, "null", 4); > >+ if (!s) > >+ return JS_FALSE; > >+ return scx->callback(JS_GetStringChars(s), JS_GetStringLength(s), scx->data); > > Ditto above comment about null_ucstr -- make it file-static so it can be used > anywhere? > > >+ } > >+ > >+ if (type == JSTYPE_BOOLEAN) { > > JSVAL_IS_BOOLEAN(*vp) > > >+ s = js_ValueToString(cx, *vp); > >+ if (!s) > >+ return JS_FALSE; > >+ return scx->callback(JS_GetStringChars(s), JS_GetStringLength(s), scx->data); > > Similarly: const jschar *boolean_ucstr[] = {{'f','a','l','s','e'}, > {'t','r','u','e'}}; and index using JSVAL_TO_BOOLEAN(*vp). > > >+ } > >+ > >+ if (type == JSTYPE_NUMBER) { > > Just test JSVAL_IS_NUMBER(*vp). > > >+ if (JSVAL_IS_DOUBLE(*vp)) { > >+ jsdouble d = *JSVAL_TO_DOUBLE(*vp); > >+ if (!JSDOUBLE_IS_FINITE(d)) { > >+ s = JS_NewStringCopyN(cx, "null", 4); > >+ return scx->callback(JS_GetStringChars(s), JS_GetStringLength(s), scx->data); > > null_ucstr, no new string assigned to s. > > >+ } > >+ } > >+ > >+ s = js_ValueToString(cx, *vp); > > Avoid allocating a JSString from the GC heap, use JS_dtostr and inflate to a > jschar stack buffer, since we have an upper bound on length of result string. > > >+ if (!s) > >+ return JS_FALSE; > >+ return scx->callback(JS_GetStringChars(s), JS_GetStringLength(s), scx->data); > >+ } > >+ > >+ if (JSVAL_IS_OBJECT(*vp) && type != JSTYPE_FUNCTION && type != JSTYPE_XML) { > > Use VALUE_IS_FUNCTION and VALUE_IS_XML to exclude. > > Have to break here, more soon. New patch welcome. > > /be

(In reply to comment #19) > (In reply to comment #17) > > > > Seems like the more precise type for replacer is JSObject * -- either way, > > caller must root. But since the spec requires either a function or an array > > object, jsval is too broad. > > > > If reviver can be more precisely typed in the JS API, it should be too. > > Otherwise, are you trying to maintain symmetry between reviver and replacer? > > See comment here: > > https://bugzilla.mozilla.org/show_bug.cgi?id=476374#c10 > > Narrowing type to JSObject makes it possible to lose closure information. No, that's about not using JSFunction* -- specifically, the f format code to JS_ConvertArguments. The particular key ingredient, not present here, is the use of the FUN_OBJECT macro -- that downcasts from JSObject to JSFunction but only for a compiler-created function. If you take a jsval replacer *or* a JSObject *replacer, you can't and shouldn't use that macro, and you wouldn't use JSFunction * in any event. Using a jsval v that must satisfy JSVAL_IS_OBJECT(v) is the same as using a JSObject *obj = JSVAL_TO_OBJECT(v) but of course with too wide a type. /be

IOW, JSVAL_IS_OBJECT(v) implies (JSObject*)v == JSVAL_TO_OBJECT(v). You're no better off using jsval as the type, in terms of safety from the caller-side and independent risk of a loss of closure identity, and worse in allowing booleans, ints, etc. in through the API whose parameter type is in question here. The deal with functions is that fun is-a obj and closure = clone(fun) is-a obj but !(closure is-a fun). Using function objects as given is safe. Using anything like JS_ValueToFunction or (equivalently) the f format specifier to JS_ConvertArguments is unsafe. But again, this has nothing to do with the type of replacer. /be

oops, I didn't mean to submit that comment. That's what I thought it was at first.

(In reply to comment #22) > oops, I didn't mean to submit that comment. That's what I thought it was at > first. The parameter was a JSFunction originally, then I switched to a jsval. JSObject is a little better.

Comment on attachment 374076 [details] [diff] [review] add replacer and whitespace stuff, per ES5 > js_json_stringify(JSContext *cx, uintN argc, jsval *vp) > { > jsval *argv = vp + 2; >+ jsval replacer = JSVAL_VOID; >+ jsval space = JSVAL_VOID; >+ jsval vec[2] = { replacer, space }; >+ JSAutoTempValueRooter(cx, 2, vec); > > // Must throw an Error if there isn't a first arg >+ if (!JS_ConvertArguments(cx, argc, argv, "v / v v", vp, &replacer, &space)) > return JS_FALSE; > >- if (!js_TryJSON(cx, vp)) >+ WriterContext wc(cx); >+ >+ if (!js_Stringify(cx, vp, replacer, space, &WriteCallback, &wc)) > return JS_FALSE; [no more replacer uses here...] > } This is ok since JS_ConvertArguments roots using argv (vp), but it makes replacer a weak root peered by the strong root at argv[1]/vp[3]. But nothing then abuses &replacer assuming it is a root. Note the distinction between "a root" == pointer to jsval or more narrowly-typed GC-thing type that the GC knows to dereference when marking" vs. "rooted value" / "weakly rooted value" / just "value" in the case of a jsval variable such as replacer in the above code. It happens that replacer does not vary after JS_ConvertArguments returns, so argv[1] keeps it live if it is set by JS_ConvertArguments at all (if there are more than one arguments). But this is arguably suboptimal style since it leaves open the possibility of &replacer abuse, and indeed that happens later in similar code. Still, it's hard to avoid jsval replacer here, because we don't have a guaranteed argv[1] to use unconditionally (a JSNative or "slow native" would, but JSFastNatives do not receive undefined when fewer actuals than expected formals are passed by a call). > JSBool >+js_Stringify(JSContext *cx, jsval *vp, jsval replacer, jsval space, >+ JSONWriteCallback callback, void *data) > { [snip] >+ StringifyContext scx(callback, &replacer, data); >+ if (!InitializeGap(cx, space, &scx.gap)) >+ return JS_FALSE; >+ >+ JSObject *obj = js_NewObject(cx, &js_ObjectClass, NULL, NULL, 0); >+ if (!obj) >+ return JS_FALSE; >+ >+ if (!OBJ_DEFINE_PROPERTY(cx, obj, ATOM_TO_JSID(cx->runtime->atomState.emptyAtom), >+ *vp, NULL, NULL, JSPROP_ENUMERATE, NULL)) { >+ return JS_FALSE; > } > >+ return Str(cx, vp, ATOM_TO_JSID(cx->runtime->atomState.emptyAtom), obj, &scx); > } Here is trouble: &replacer is passed to scx and it will be used as a root, via the keySource alias passed to js_ValueToIterator in JO. But &replacer is not a root -- replacer is not a rooted value which, however often it is updated, the GC will always mark. So the js_ValueToIterator result will not be rooted. It's true that the replacer actual parameter value to js_Stringify must be rooted by the caller (whether of jsval or JSObject * type). But that protects only its initial value when passed in. In general making pointer aliases to rooted jsvals is not helpful. Making jsval copies of rooted values, whose address is then taken and passed off, is also problematic. /be

(In reply to comment #24) > This is ok since JS_ConvertArguments roots using argv (vp), but it makes > replacer a weak root peered by the strong root at argv[1]/vp[3]. But nothing > then abuses &replacer assuming it is a root. > > Note the distinction between "a root" == pointer to jsval or more > narrowly-typed GC-thing type that the GC knows to dereference when marking... Of course I didn't use the jargon correctly in the first paragraph. English and other languages make this kind of referent-for-reference substitution easy and commonplace, mostly without harm. In reasoning about GC-safety however, it's not good practice. /be

(In reply to comment #24) > In general making pointer aliases to rooted jsvals is not helpful. Making jsval > copies of rooted values, whose address is then taken and passed off, is also > problematic. The latter is strictly worse, but still can be ok (and hard to avoid) as in js_json_stringify. /be

So, I set gczeal to 11 (spinal tap!) in js_InitJSONClass. Everything looks good, except for test_replacer.js, of course. I'm puzzled by the errors. I suppose I'll understand the GC better than I did before when this is fixed.

probably not correct yet, but it does pass my tests with gczeal cranked up.

Comment on attachment 376063 [details] [diff] [review] passes unit tests under gczeal high >+ jsval key = JSVAL_VOID; >+ jsval replacer = JSVAL_VOID; >+ jsval outputValue = JSVAL_VOID; >+ jsval vec[3] = {key, replacer, outputValue}; >+ JSAutoTempValueRooter tvr(cx, 3, vec); > >+ JSObject *iterObj = NULL; >+ jsval *keySource = vp; >+ bool usingWhitelist = false; >+ >+ // if the replacer is an array, we use the keys from it >+ if (scx->replacer && JS_IsArrayObject(cx, scx->replacer)) { >+ usingWhitelist = true; >+ replacer = OBJECT_TO_JSVAL(scx->replacer); >+ keySource = &replacer; &replacer is not a root -- i.e., replacer is not rooted. vec[1] is a root, but now the value there may have diverged from the value in replacer. If you want well-named aliases for vec[0], vec[1], and vec[2], then do this: jsval vec[3] = {JSVAL_VOID, JSVAL_VOID, JSVAL_VOID}; JSAutoTempValueRooter tvr(cx, 3, vec); jsval& key = &vec[0]; jsval& replacer = &vec[1]; jsval& outputValue = &vec[2]; >+ if (JS_IsArrayObject(cx, JSVAL_TO_OBJECT(*vp))) { >+ ok = JA(cx, vp, scx); >+ } else { >+ ok = JO(cx, vp, scx); >+ } Customary in prevailing style to avoid over-bracing but also avoid if-else in favor of ok = (JS_IsArrayObject(cx, JSVAL_TO_OBJECT(*vp)) ? JA : JO)(cx, vp, scx); I need to take a closer look but thought I'd pass this along, solicit a new patch, maybe something in the way of a gczeal testcase to trigger the problem above with unrooted jsvals that copy values from rooted jsvals but then can diverge. /be

Comment on attachment 376147 [details] [diff] [review] avoid an extra root value, since caller must root scx->replacer botched that patch

Comment on attachment 376188 [details] [diff] [review] fix rooting >+class StringifyContext >+{ >+public: >+ StringifyContext(JSONWriteCallback callback, JSObject *replacer, void *data) >+ : callback(callback), replacer(replacer), data(data), depth(0) Nit: style for similar C++ wants the : initialisers half-indented (two spaces before :). >+ jsval vec[3] = {JSVAL_VOID, JSVAL_VOID}; Bug: need three initializers here. Nit here and elsewhere: use JSVAL_NULL, it's just as good and a faster/shorter constant to synthesize. >+ JSAutoTempValueRooter tvr(cx, 2, vec); >+ jsval& key = vec[0]; >+ jsval& outputValue = vec[1]; > >+ JSObject *iterObj = NULL; >+ jsval *keySource = vp; >+ bool usingWhitelist = false; >+ >+ // if the replacer is an array, we use the keys from it >+ if (scx->replacer && JS_IsArrayObject(cx, scx->replacer)) { >+ usingWhitelist = true; >+ *keySource = OBJECT_TO_JSVAL(scx->replacer); Need to set keySource = &vec[2] here first! Otherwise you overwrite *vp, unrooting obj. >+ // Be careful below, this string is weakly rooted > JSString *s; >+ s = js_ValueToString(cx, key); Initialize s's declaration. . . . > } while (ok); > > if (iterObj) { > // Always close the iterator, but make sure not to stomp on OK > ok &= js_CloseIterator(cx, *vp); Here before js_CloseIterator you could assert OBJECT_TO_JSVAL(iterObj) == *vp. >+static JSBool >+Str(JSContext *cx, jsid id, JSObject *holder, StringifyContext *scx, jsval *vp) >+{ >+ JS_CHECK_RECURSION(cx, return JS_FALSE); >+ >+ if (!OBJ_GET_PROPERTY(cx, holder, id, vp)) >+ return JS_FALSE; Note factoring opportunity for js_Stringify comments below. >+ if (!JSVAL_IS_PRIMITIVE(*vp) && !js_TryJSON(cx, vp)) >+ return JS_FALSE; . . . >+ JSString *s; Nit: reusing s hoists it up one level, but lots of JSBool ok decls are not hoisted for same reason -- hoist both or neither. I'm fine with both. >+ if (JSVAL_IS_STRING(*vp)) { >+ s = JSVAL_TO_STRING(*vp); >+ return write_string(cx, scx->callback, scx->data, JS_GetStringChars(s), JS_GetStringLength(s)); >+ } >+ >+ if (JSVAL_IS_NULL(*vp)) { >+ return scx->callback(null_ucstr, 4, scx->data); Could use JS_ARRAY_LENGTH(null_ucstr) but no big. >+ } >+ >+ if (JSVAL_IS_BOOLEAN(*vp)) { >+ uint32 len = 4; >+ const jschar *chars = true_ucstr; >+ JSBool b = JSVAL_TO_BOOLEAN(*vp); >+ >+ if (!b) { >+ chars = false_ucstr; >+ len = 5; Ditto for 4 and 5 hardcodings. >+ } >+ >+ return scx->callback(chars, len, scx->data); >+ } >+ >+ if (JSVAL_IS_NUMBER(*vp)) { >+ if (JSVAL_IS_DOUBLE(*vp)) { >+ jsdouble d = *JSVAL_TO_DOUBLE(*vp); >+ if (!JSDOUBLE_IS_FINITE(d)) { >+ return scx->callback(null_ucstr, 4, scx->data); >+ } Nit: overbraced consequent. >+ } >+ >+ char numBuf[DTOSTR_STANDARD_BUFFER_SIZE], *numStr; >+ jsdouble d = JSVAL_IS_INT(*vp) ? (jsdouble)JSVAL_TO_INT(*vp) : *JSVAL_TO_DOUBLE(*vp); Nit: jsdouble(...) style cast. >+ numStr = JS_dtostr(numBuf, sizeof numBuf, DTOSTR_STANDARD, 0, d); >+ if (!numStr) { >+ JS_ReportOutOfMemory(cx); >+ return JS_FALSE; >+ } >+ >+ jschar dstr[DTOSTR_STANDARD_BUFFER_SIZE]; >+ size_t dbufSize = DTOSTR_STANDARD_BUFFER_SIZE; Nit: s/dbufSize/dbufLength/ (or dbufLen -- len(gth) for count of elements, vs. size for sizeof/bytes). >+ if (!js_InflateStringToBuffer(cx, numStr, strlen(numStr), dstr, &dbufSize)) >+ return JS_FALSE; >+ >+ JSBool ok = scx->callback(dstr, dbufSize, scx->data); >+ >+ return ok; Just return scx->callback here, no need for ok. >+} >+static JSBool >+InitializeGap(JSContext *cx, jsval space, JSStringBuffer *sb) Nit: one blank line between functions. >+js_Stringify(JSContext *cx, jsval *vp, JSObject *replacer, jsval space, >+ JSONWriteCallback callback, void *data) > { >+ // XXX stack >+ JSObject *stack = JS_NewArrayObject(cx, 0, NULL); >+ if (!stack) >+ return JS_FALSE; > >+ StringifyContext scx(callback, replacer, data); >+ if (!InitializeGap(cx, space, &scx.gap)) >+ return JS_FALSE; >+ >+ JSObject *obj = js_NewObject(cx, &js_ObjectClass, NULL, NULL, 0); >+ if (!obj) >+ return JS_FALSE; >+ >+ if (!OBJ_DEFINE_PROPERTY(cx, obj, ATOM_TO_JSID(cx->runtime->atomState.emptyAtom), >+ *vp, NULL, NULL, JSPROP_ENUMERATE, NULL)) { >+ return JS_FALSE; > } > >+ return Str(cx, ATOM_TO_JSID(cx->runtime->atomState.emptyAtom), obj, &scx, vp); > } The spec is wasteful here, there's no point in making an object just to hold the value to stringify under the empty-string identifier. Suggest factoring the [[Get]] out of Str and calling the lower-level factored StrHelper or whatever it should be called (Str is a poor name!). /be

> > The spec is wasteful here, there's no point in making an object just to hold > the value to stringify under the empty-string identifier. It does that so that the replacer function gets a temporary object for "this" in the root case, and always gets a key and value. Necessary? I don't know, I found it kind of strange to code to while writing test cases, but not a big deal.

I didn't do that last refactor. Did you want it done solely to avoid an OBJ_GET_PROPERTY in the root case? I still have to make the holder object in order to present the right thing to replacer functions and other script.

(In reply to comment #37) > Created an attachment (id=376278) [details] > address brendan's comments > > I didn't do that last refactor. Did you want it done solely to avoid an > OBJ_GET_PROPERTY in the root case? Split out to a separate bug, 'sok. > I still have to make the holder object in > order to present the right thing to replacer functions and other script. Only if (scx->replacer && js_IsCallable(scx->replacer, cx)), right? For stringify calls that do not whitelist or replace, there's no need for this object. For leaf values with no replacer, it's probably a high cost. /be

So I'm working on documenting this, but have run into a snag. I can't get the little example I tried to put together, based on code in the patch, to work. Here's my code, which I'm trying to use in the error console window: var foo = ["Mozilla", "box", 45, "car", 7]; function replacer(k, v) {if (typeof(v) == "string") { return null; }}; JSON.stringify(foo, replacer); I've been banging my head on this for 30 minutes without any luck, so I'm obviously missing something dumb. Can anyone tell me what that is (hopefully gently)? Thanks.

there's a bug open on this, bug 509184... the replacer is broken for object keys. will be fixed in 1.9.1.5 :(

OK, I added a note about this to my doc, which is here. Feel free to correct it or whine about inaccuracies: https://developer.mozilla.org/En/Using_native_JSON#Converting_objects_into_JSON