Last Comment Bug 672063 - Complete nsIJSON implementation again
: Complete nsIJSON implementation again
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 7 Branch
: All All
: -- normal (vote)
: mozilla8
Assigned To: Nils Maier [:nmaier]
:
Mentors:
Depends on:
Blocks: 645922
  Show dependency treegraph
 
Reported: 2011-07-16 11:36 PDT by Nils Maier [:nmaier]
Modified: 2011-09-22 16:10 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed


Attachments
patch v1, re-implement encode/decode again (25.02 KB, patch)
2011-07-17 13:25 PDT, Nils Maier [:nmaier]
jwalden+bmo: review-
Details | Diff | Review
patch v2, re-implement encode/decode again (25.14 KB, patch)
2011-07-19 15:26 PDT, Nils Maier [:nmaier]
no flags Details | Diff | Review
patch, v3 (26.05 KB, patch)
2011-07-19 18:32 PDT, Nils Maier [:nmaier]
jwalden+bmo: review-
Details | Diff | Review
patch v4, re-implement encode/decode again (26.13 KB, patch)
2011-07-20 16:57 PDT, Nils Maier [:nmaier]
jwalden+bmo: review+
dmandelin: superreview+
christian: approval‑mozilla‑aurora+
Details | Diff | Review

Description Nils Maier [:nmaier] 2011-07-16 11:36:28 PDT
Bug 645922 removed .encode/.decode from nsIJSON/nsJSON, leaving the .encodeFromString/.decodeFromString methods.

https://mxr.mozilla.org/addons/search?string=nsIJSON shows more than 1000 for nsIJSON. Hence there a sensation of impending doom in the add-on land.

To avoid that havoc the removed methods should be re-implemented using either native JSON or said stream interfaces as a compatibility hack, whatever is easier.
Comment 1 Jorge Villalobos [:jorgev] 2011-07-16 15:05:57 PDT
The compatibility check for the validator is bug 670284, and we're giving forward notice to add-on developers on the blog and mailing list, probably on Monday.

Having said that, I would certainly prefer that these methods weren't removed right away, but deprecated for a couple of releases. We talked to Christian about this last week, but we haven't decided anything yet.
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-16 15:29:10 PDT
How hard is it to put them back?
Comment 3 Nils Maier [:nmaier] 2011-07-17 13:25:27 PDT
Created attachment 546435 [details] [diff] [review]
patch v1, re-implement encode/decode again

This patch re-implements a local version of JS_TryJSON and .encode/.decode
The old behavior is restored, also for .encodeToStream - which behaved differently than before as EncodeInternal(null) == "null" instead of |null|, as it was before.

This patch also restores the old interface uuid and the test cases.
Also it adds some tests, notably testToJSON.

The behavior is almost as before, with the only difference I know of being that a throwing toJSON() will now result in the caller seeing an XPCOM exception instead of the actual exception toJSON() threw. I guess this is acceptable however.
Comment 4 Boris Zbarsky [:bz] 2011-07-17 20:46:04 PDT
Some subset of brendan/jst/waldo (from bug 645922) are probably better reviewers than me here, since they've actually seen this code before.  I suggest starting with waldo.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-19 11:48:44 PDT
Comment on attachment 546435 [details] [diff] [review]
patch v1, re-implement encode/decode again

Review of attachment 546435 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely the JSAPI misuse would need to be fixed before we could run with this patch, so r- for that at the very least.

::: dom/interfaces/json/nsIJSON.idl
@@ +52,4 @@
>  /**
>   * Encode and decode JSON text.
>   */
> +[scriptable, uuid(a4d68b4e-0c0b-4c7c-b540-ef2f9834171f)]

I think you'd just be undoing the previous changes, here, so I'm not aware of a reason why you shouldn't revert to the previous UUID here.

@@ +56,4 @@
>  interface nsIJSON : nsISupports
>  {
> +  /* New users should use JSON.stringify
> +   * encode is only present for backward compatibility

/**
 * ...
 */

for the comment format.  And use complete sentences, with periods and such.  :-)

@@ +65,5 @@
>                        in boolean writeBOM
>                        /* in JSObject value */);
>  
> +  /* New users should use JSON.parse
> +   * encode is only present for backward compatibility

This comment should refer to "decode", not "encode".

::: dom/src/json/nsJSON.cpp
@@ +76,5 @@
>  {
>  }
>  
> +NS_IMETHODIMP
> +nsJSON::Encode(nsAString &aJSON)

If this is temporarily readded, I want the console spammed with a loud, obnoxious deprecation notice.  You see the crazy semantics this method implemented: nobody should have to think about any of that.  JSON.stringify is so much better it's not even funny.

@@ +262,5 @@
> +  
> +  // Backward compatibility:
> +  // nsIJSON does not allow to serialize anything other than objects
> +  JSObject *obj;
> +  if (!JSVAL_IS_OBJECT(*vp) || !(obj = JSVAL_TO_OBJECT(*vp)))

if (JSVAL_IS_PRIMITIVE(*vp)), and assign obj after the check.  See also below.

@@ +268,5 @@
> +
> +  /* Backward compatibility:
> +   * Manually call toJSON and check that the result, if any, is still
> +   * an object
> +   * Note: This is basically a reimplementation of JS_TryJSON, that only

Don't refer to a function that doesn't exist.  Either describe the full semantics in a comment, or just let the code speak for itself.  I think having the code alone is the better option, given that these semantics are all pretty broken, and this method is going to go away eventually.

@@ +277,5 @@
> +   * Note: Do not use JS_CallFunctionName directly, as this would push
> +   * incorrect error information
> +   */
> +  jsval toJSON;
> +  if (JS_GetMethod(cx, obj, "toJSON", NULL, &toJSON) &&

This isn't how the JSAPI works.  Except for certain "querying" JSAPI functions which are hopefully clearly documented as such, the return value of a JSAPI function indicates success (true, or a non-null pointer) or failure (false, or anull pointer).  If JS_GetMethod fails, you should be returning something like NS_ERROR_FAILURE.  JS_ObjectIsCallable is one of those querying functions (it can't both answer the question and indicate success, so it must simply answer the question), so you can just use it as you do here.  JS_CallFunctionValue returns success/failure, so if it fails, you should return a failure code.

And by "fails", what the JSAPI means is something that might have thrown a JS exception.  If an exception's been thrown, it's by definition "pending", and it isn't kosher to call JSAPI functions that might themselves throw exceptions when an exception is pending.  You have to handle the exception (by propagating it, or swallowing it, or whatever) before you could possibly have another one.

@@ +281,5 @@
> +  if (JS_GetMethod(cx, obj, "toJSON", NULL, &toJSON) &&
> +      JSVAL_IS_OBJECT(toJSON) &&
> +      JS_ObjectIsCallable(cx, JSVAL_TO_OBJECT(toJSON)) &&
> +      JS_CallFunctionValue(cx, obj, toJSON, 0, NULL, vp) && 
> +      !(JSVAL_IS_OBJECT(*vp) && JSVAL_TO_OBJECT(*vp)))

!(isobject and isnonnullobject) is much more clearly expressed as JSVAL_IS_PRIMITIVE.

These lines of odd semantics, plus the didn't-write-anything special-casing in Encode, are the rest of the reason this should be removed.  Anyone who thinks nsIJSON implements JSON semantics is sorely mistaken, and will only find out their mistake when it's too late.

@@ +410,4 @@
>  }
>  
>  NS_IMETHODIMP
> +nsJSON::Decode(const nsAString& json)

This too would need loud, obnoxious deprecation console-spamming.
Comment 6 Nils Maier [:nmaier] 2011-07-19 13:24:22 PDT
(In reply to comment #5)
> Comment on attachment 546435 [details] [diff] [review] [review]
> patch v1, re-implement encode/decode again
> 
> Review of attachment 546435 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Definitely the JSAPI misuse would need to be fixed before we could run with
> this patch, so r- for that at the very least.
> 
> ::: dom/interfaces/json/nsIJSON.idl
> @@ +52,4 @@
> >  /**
> >   * Encode and decode JSON text.
> >   */
> > +[scriptable, uuid(a4d68b4e-0c0b-4c7c-b540-ef2f9834171f)]
> 
> I think you'd just be undoing the previous changes, here, so I'm not aware
> of a reason why you shouldn't revert to the previous UUID here.

I did exactly that. This is the previous uuid.
http://mxr.mozilla.org/mozilla-beta/source/dom/interfaces/json/nsIJSON.idl#55

> ::: dom/src/json/nsJSON.cpp
> @@ +76,5 @@
> >  {
> >  }
> >  
> > +NS_IMETHODIMP
> > +nsJSON::Encode(nsAString &aJSON)
> 
> If this is temporarily readded, I want the console spammed with a loud,
> obnoxious deprecation notice.  You see the crazy semantics this method
> implemented: nobody should have to think about any of that.  JSON.stringify
> is so much better it's not even funny.

I'd like to handle an deprecation warnings in a follow-up bug, as it would require strings. It either lands in 7, too, and hence was never really gone, or the add-on extinction would happen anyway. It is my understanding that adding strings would complicate or even prevent a landing...

> @@ +277,5 @@
> > +   * Note: Do not use JS_CallFunctionName directly, as this would push
> > +   * incorrect error information
> > +   */
> > +  jsval toJSON;
> > +  if (JS_GetMethod(cx, obj, "toJSON", NULL, &toJSON) &&
> 
> This isn't how the JSAPI works.  Except for certain "querying" JSAPI
> functions which are hopefully clearly documented as such, the return value
> of a JSAPI function indicates success (true, or a non-null pointer) or
> failure (false, or anull pointer).  If JS_GetMethod fails, you should be
> returning something like NS_ERROR_FAILURE.  JS_ObjectIsCallable is one of
> those querying functions (it can't both answer the question and indicate
> success, so it must simply answer the question), so you can just use it as
> you do here.  JS_CallFunctionValue returns success/failure, so if it fails,
> you should return a failure code.

It is optional to provide .toJSON() and therefore GetMethod failures are OK. That just means to serialize the actual object and not the object returned by .toJSON(). Look at the removed js_TryJSON implementation to understand why this code works like it does. There are tests with and without .toJSON.
When the method exists but throws the exception could be propagated directly indeed, however the following object checks will cause Encode to fail anyway. There is a restored test for throwing .toJSON.

> And by "fails", what the JSAPI means is something that might have thrown a
> JS exception.  If an exception's been thrown, it's by definition "pending",
> and it isn't kosher to call JSAPI functions that might themselves throw
> exceptions when an exception is pending.  You have to handle the exception
> (by propagating it, or swallowing it, or whatever) before you could possibly
> have another one.

It is my understanding that I'm swallowing any exceptions here, as the code is supposed to. If not, can you please point me in the right direction? I'm not a JSAPI pro; actually this is the first time I even did something JSAPI other than reading it and debugging some crashes and other stuff.

> @@ +281,5 @@
> > +  if (JS_GetMethod(cx, obj, "toJSON", NULL, &toJSON) &&
> > +      JSVAL_IS_OBJECT(toJSON) &&
> > +      JS_ObjectIsCallable(cx, JSVAL_TO_OBJECT(toJSON)) &&
> > +      JS_CallFunctionValue(cx, obj, toJSON, 0, NULL, vp) && 
> > +      !(JSVAL_IS_OBJECT(*vp) && JSVAL_TO_OBJECT(*vp)))
> 
> !(isobject and isnonnullobject) is much more clearly expressed as
> JSVAL_IS_PRIMITIVE.

Ok, makes perfect sense. I never thought about it, I simply reused the code that was already there before. Will fix this accordingly.

> These lines of odd semantics, plus the didn't-write-anything special-casing
> in Encode, are the rest of the reason this should be removed.  Anyone who
> thinks nsIJSON implements JSON semantics is sorely mistaken, and will only
> find out their mistake when it's too late.

The intension is not to promote nsIJSON as a conforming json implementation, but to restore backwards compatibility so that hundreds of add-ons would not just stop working without a necessarily long grace period.
You don't just turn on strict mode for chrome or even the web for the very same reason.

Also I don't understand why you not only changed the (broken) semantics of the stream version of encode but left the stream versions in in the first place instead of entirely removing nsIJSON, if it all was that horrible. :p
Comment 7 Nils Maier [:nmaier] 2011-07-19 15:26:46 PDT
Created attachment 546928 [details] [diff] [review]
patch v2, re-implement encode/decode again

Comments and nits addressed, as they apply.

The patch now bails correctly on a failed CallFunctionValue and does not mask the original exception behind a failure code now, as it did before.

As stated, I'd like to address any deprecation warnings in another bug. Or actually somebody else should do this who is more familiar with all the stuff such warnings would touch. ;)
Comment 8 Tom Schuster [:evilpie] 2011-07-19 15:35:55 PDT
Comment on attachment 546928 [details] [diff] [review]
patch v2, re-implement encode/decode again

Review of attachment 546928 [details] [diff] [review]:
-----------------------------------------------------------------

I don't really know this kind of code, but i remembered from our doc sprint that there are IDL classes/methods declared as deprecated. So after a quick google search, i found bug eg. Bug 518440.

::: dom/interfaces/json/nsIJSON.idl
@@ +52,4 @@
>  /**
>   * Encode and decode JSON text.
>   */
> +[scriptable, uuid(a4d68b4e-0c0b-4c7c-b540-ef2f9834171f)]

[scriptable, deprecated, uuid(a4d68b4e-0c0b-4c7c-b540-ef2f9834171f)]
Not sure if the whole interface should be deprecated

@@ +58,5 @@
> +  /**
> +   * New users should use JSON.stringify!
> +   * The encode() method is only present for backward compatibility.
> +   */
> +  AString encode(/* in JSObject value */);

[deprecated] AString encode(/* in JSObject value */);

@@ +69,5 @@
> +  /**
> +   * New users should use JSON.parse!
> +   * The decode() method is only present for backward compatibility.
> +   */
> +  void /* JSObject */ decode(in AString str);

[deprecated] void /* JSObject */ decode(in AString str);
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-19 16:41:09 PDT
(In reply to comment #6)
> I did exactly that. This is the previous uuid.
> http://mxr.mozilla.org/mozilla-beta/source/dom/interfaces/json/nsIJSON.idl#55

Er, oops.  I misread the + line as the - line, and the - as the plus, when looking at this:

http://hg.mozilla.org/mozilla-central/rev/0908448636c5

That is indeed the old uuid.  Sorry for the mistake.  :-\
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-19 17:29:46 PDT
JS_GetMethod can throw if merely trying to retrieve the property in question throws.  The easiest way (tho not the only one) is if the property is a getter:

  jsonComp.encode({ get toJSON() { throw new Error("crash and burn"); } });

So a JS_GetMethod failure does indeed need to be addressed in the patch.

I am extremely frustrated at the string concern, for what is a purely developer-oriented string.  Developer-oriented strings aren't even uniformly held to a standard of requiring localization; JS engine warning and error messages have never been localized, for example.

I didn't remove the streaming nsIJSON methods because there was no drop-in replacement, and the only win I was aware of was conceptual purity.  encode/decode can be near-trivially replaced with JSON.parse/stringify.  (Every mozilla-central use could be replaced in that way.)  The stream methods didn't have such alternatives.  It's easy to code up the replacement functions, to be sure.  But conceptual purity didn't seem enough to go out of the way to break extension authors.

I'm pretty sure I'm excessively frustrated right now about the resistance to removing proprietary APIs in favor of standards-based ones, and I'm trying to not let that color what I say too much.  But given that I can't remember ever hearing a serious proposal to force strict mode on for extensions or for the web, I submit that the comparison between that change and removing two proprietary methods with standardized alternatives is not a fair one.
Comment 11 Nils Maier [:nmaier] 2011-07-19 17:50:55 PDT
(In reply to comment #10)
> JS_GetMethod can throw if merely trying to retrieve the property in question
> throws.  The easiest way (tho not the only one) is if the property is a
> getter:
> 
>   jsonComp.encode({ get toJSON() { throw new Error("crash and burn"); } });
> 
> So a JS_GetMethod failure does indeed need to be addressed in the patch.

Gotcha! v3 coming...

> I didn't remove the streaming nsIJSON methods because there was no drop-in
> replacement, and the only win I was aware of was conceptual purity. 
> encode/decode can be near-trivially replaced with JSON.parse/stringify. 
> (Every mozilla-central use could be replaced in that way.) The stream
> methods didn't have such alternatives.  It's easy to code up the replacement
> functions, to be sure.  But conceptual purity didn't seem enough to go out
> of the way to break extension authors.
> 
> I'm pretty sure I'm excessively frustrated right now about the resistance to
> removing proprietary APIs in favor of standards-based ones, and I'm trying
> to not let that color what I say too much.  But given that I can't remember
> ever hearing a serious proposal to force strict mode on for extensions or
> for the web, I submit that the comparison between that change and removing
> two proprietary methods with standardized alternatives is not a fair one.

See, now we're both excessively frustrated, I'm for seeing a bad but (unfortunately) heavily used API removed without even a proper grace period, and as an add-on developer I feel either ignored or even deprecated.
Granted, the strict mode comparison might not be fair and is exaggerated, but from my perspective it still fits: Both, the nsIJSON changes and forced strict mode for chrome would supposedly avoid odd and sometimes proprietary APIs, but by this also break a high number of third-party code without no apparent technical necessity or proper warning.
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-19 18:03:40 PDT
The proper grace period was the couple days from landing to merge to mozilla-aurora, the six weeks latent in mozilla-aurora, then the six weeks latent in mozilla-beta, then the release.  That's approximately twelve weeks, three months, a quarter year.  Perhaps I'm just confused (and beginning to tread off-topic, so I'll try to keep it short), but why isn't a quarter year a proper grace period?  I was under the impression that this was the API plan of record for awhile now, and I'm confused about the complaints about standard process.
Comment 13 Nils Maier [:nmaier] 2011-07-19 18:26:51 PDT
(In reply to comment #12)
> The proper grace period was the couple days from landing to merge to
> mozilla-aurora, the six weeks latent in mozilla-aurora, then the six weeks
> latent in mozilla-beta, then the release.  That's approximately twelve
> weeks, three months, a quarter year.  Perhaps I'm just confused (and
> beginning to tread off-topic, so I'll try to keep it short), but why isn't a
> quarter year a proper grace period?  I was under the impression that this
> was the API plan of record for awhile now, and I'm confused about the
> complaints about standard process.

I outlined my views on breaking changes and the new turbo release process as a comment to gerv's blog post, also linked from jorgev's blog post[1]. I read your comment to the latter, too, which made me a bit less frustrated. ;)
The first bug report (or a few to two different extensions of mine) I got on July 06[2], to the 12 weeks number is wrong. 

[1] http://xulforge.com/blog/2011/07/version-numbers-add-on-breakage/
[2] https://github.com/scriptish/scriptish/issues/420
Comment 14 Nils Maier [:nmaier] 2011-07-19 18:32:33 PDT
Created attachment 546962 [details] [diff] [review]
patch, v3

- Adding a check for throwing GetMethod + corresponding test in testThrowingToJSON
- Adding [deprecated] to encode/encodeFromStream/decode. However that annotation will only generate compile-time warnings, so does not affect js users (but still serves as documentation).
Comment 15 Gervase Markham [:gerv] 2011-07-20 11:38:05 PDT
(In reply to comment #12)
> The proper grace period was the couple days from landing to merge to
> mozilla-aurora, the six weeks latent in mozilla-aurora, then the six weeks
> latent in mozilla-beta, then the release.  That's approximately twelve
> weeks, three months, a quarter year.  Perhaps I'm just confused (and
> beginning to tread off-topic, so I'll try to keep it short), but why isn't a
> quarter year a proper grace period? 

Because for most add-on developers, the "grace period" starts when they get an email from AMO telling them their addon won't be automatically compatible with the next version, and could they fix it please? That period is the six weeks of beta, plus some of the six weeks of Aurora (not sure how much).

It would be entirely OK, I'm sure, for AMO to send deprecation warnings in such emails instead of removal warnings. Then in a couple of releases' time, we can remove it entirely.

Gerv
Comment 16 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-20 16:02:53 PDT
Comment on attachment 546962 [details] [diff] [review]
patch, v3

Review of attachment 546962 [details] [diff] [review]:
-----------------------------------------------------------------

mrbkap says it would be better for these methods to use the [implicit_jscontext] attribute, which would mean the methods in C++ would have a leading JSContext* argument added to them, avoiding the need for the song-and-dance routine to figure out a JSContext.  That seems like way more effort than I want to foist on volunteers, especially not for methods which are on the way out the door, and might go away before someone else doing that cleanup could remove them.  So don't worry about it -- but noting it anyways for completeness, and in case you have any interest in filing the followup bug (which would in no way be your responsibility to fix, to be completely clear).

::: dom/interfaces/json/nsIJSON.idl
@@ +73,5 @@
> +                                   /* in JSObject value */);
> +
> +  /**
> +   * New users should use JSON.parse!
> +   * The decode() methods a only present for backward compatibility.

Looks like you meant to say "are only present" here, maybe?

That said, spelling it out in two separate comments is probably the better move, like for encode{,ToStream}.  In the past (not sure if Mozilla-the-organization does it any longer, but I think some individuals do it, and Google searches will find them) we've used automated tools like doxygen to generate documentation from IDL files, and comments meant to discuss multiple attributes or methods don't translate well in that process, which only associates the one comment by a method/attribute with the method/attribute.

If we're going to have one comment per method, too, it probably makes sense to say, for the streaming methods, that the user should write the serialized string to the stream manually using nsIOutputStream, or should read the encoded string using nsIScriptableInputStream before using JSON.parse on it.  Basically, the comments should spell out what the preferred alternative mechanism would be -- it's clear for encode/decode, less so for the {To,From}Stream versions.

::: dom/src/json/nsJSON.cpp
@@ +78,5 @@
>  
> +NS_IMETHODIMP
> +nsJSON::Encode(nsAString &aJSON)
> +{
> +  // This function should only be called from JS.

...so the [deprecated] markers on stuff here are purely advisory and will never result in build-time warnings or anything like that, alas.  But as docs for the person who learns this stuff via IDL, they're still better than nothing.

@@ +274,5 @@
> +   */
> +  jsval toJSON;
> +  if (JS_GetMethod(cx, obj, "toJSON", NULL, &toJSON) &&
> +      JSVAL_IS_OBJECT(toJSON) &&
> +      JS_ObjectIsCallable(cx, JSVAL_TO_OBJECT(toJSON))) {

JSVAL_IS_OBJECT is a horrible misnomer, in precisely the same way |typeof v === "object"| is a misnomer.  (I think for the same reason, even -- the initial JS implementation in 1994 made a mistake, and we're stuck with it at least until some sort of breaky language update.)  If the toJSON property is null, then JSVAL_IS_OBJECT, and JS_ObjectIsCallable will try to dereference JSVAL_TO_OBJECT(toJSON), which will be a null pointer.  This was the reason for mentioning JSVAL_IS_PRIMITIVE earlier -- !JSVAL_IS_PRIMITIVE(v) means is-nonnull-object, which is usually what the unseasoned user of JSVAL_IS_OBJECT meant.

@@ +280,5 @@
> +    // If toJSON is implemented, it must not throw
> +    if (!JS_CallFunctionValue(cx, obj, toJSON, 0, NULL, vp)) {
> +      if (JS_IsExceptionPending(cx))
> +        // passing NS_OK will throw the original exception
> +        return NS_OK;

Actually, if a JSAPI method fails, you should be returning an error code, so this should just be:

if (!JS_CFV(...))
  return NS_ERROR_FAILURE;

That's actually generally how this should work: call one method at a time, return failure if it fails, do the next, repeat as needed.  The JSAPI success/failure strategy is entirely in-band, and it is not designed for writing code more "naturally" as you might in a language with out-of-band failure signaling via exceptions or something similar.

@@ +298,1 @@
>      return NS_OK;

This won't be necessary if you do the call-one-JSAPI-method/check-for-failure/continue thing I mention in the above comment.

@@ +307,2 @@
>    if (!JS_Stringify(cx, vp, NULL, JSVAL_NULL, WriteCallback, writer))
>      return NS_ERROR_FAILURE;

Just commenting, but note how JS_Stringify may cause a JS exception to be thrown, and if it fails NS_ERROR_FAILURE is returned.  That's how JSAPI uses should happen in the vast majority of cases, including here.
Comment 17 Nils Maier [:nmaier] 2011-07-20 16:16:46 PDT
(In reply to comment #16)
> ::: dom/src/json/nsJSON.cpp
> @@ +78,5 @@
> >  
> > +NS_IMETHODIMP
> > +nsJSON::Encode(nsAString &aJSON)
> > +{
> > +  // This function should only be called from JS.
> 
> ...so the [deprecated] markers on stuff here are purely advisory and will
> never result in build-time warnings or anything like that, alas. 

Actually [deprecated] will result in compile-time warnings on compilers that support it.
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nscore.h#257

> @@ +274,5 @@
> > +   */
> > +  jsval toJSON;
> > +  if (JS_GetMethod(cx, obj, "toJSON", NULL, &toJSON) &&
> > +      JSVAL_IS_OBJECT(toJSON) &&
> > +      JS_ObjectIsCallable(cx, JSVAL_TO_OBJECT(toJSON))) {
> 
> JSVAL_IS_OBJECT is a horrible misnomer, in precisely the same way |typeof v
> === "object"| is a misnomer.  (I think for the same reason, even -- the
> initial JS implementation in 1994 made a mistake, and we're stuck with it at
> least until some sort of breaky language update.)  If the toJSON property is
> null, then JSVAL_IS_OBJECT, and JS_ObjectIsCallable will try to dereference
> JSVAL_TO_OBJECT(toJSON), which will be a null pointer.  This was the reason
> for mentioning JSVAL_IS_PRIMITIVE earlier -- !JSVAL_IS_PRIMITIVE(v) means
> is-nonnull-object, which is usually what the unseasoned user of
> JSVAL_IS_OBJECT meant.

OK, JSVAL_IS_OBJECT is evil; noted ;)

> 
> @@ +280,5 @@
> > +    // If toJSON is implemented, it must not throw
> > +    if (!JS_CallFunctionValue(cx, obj, toJSON, 0, NULL, vp)) {
> > +      if (JS_IsExceptionPending(cx))
> > +        // passing NS_OK will throw the original exception
> > +        return NS_OK;
> 
> Actually, if a JSAPI method fails, you should be returning an error code, so
> this should just be:
> 
> if (!JS_CFV(...))
>   return NS_ERROR_FAILURE;

No, when returning an error code then this error code gets thrown by xpconnect, and not the pending exception. (should have written "pending" not "original").
There is a test to check explicitly this.

> @@ +298,1 @@
> >      return NS_OK;
> 
> This won't be necessary if you do the
> call-one-JSAPI-method/check-for-failure/continue thing I mention in the
> above comment.

No, this should only error if there is a pending exception after calling GetMethod, not if GetMethod fails because there simply is no such method.
Comment 18 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-20 16:42:35 PDT
(In reply to comment #16)
> ...so the [deprecated] markers on stuff here are purely advisory and will
> never result in build-time warnings or anything like that, alas.  But as
> docs for the person who learns this stuff via IDL, they're still better than
> nothing.

Our docs should reflect that deprecated methods are deprecated in big scary text.
Comment 19 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-20 16:46:12 PDT
I talked to fligtar a bit last night, and apparently (I missed the memo) we have no real process right now for making sufficiently-breaking changes that should happen, but might be rocky along the way.  Which is, um, somewhat less than awesome.  And I just get to be the lucky fool testing this process.  :-)  (Or :-\ depending how much I want to subscribe to the "laugh" approach in applying my usual philosophy that "when you have a choice between either laughing or crying, laugh".)

I also found out that apparently we notify extension developers about changes much later than I thought we notified them.  (I was under the impression that we were telling extension developers to use Aurora builds, so they'd find out about changes roughly every six weeks or so, giving that twelve-week delay to producing complete fixes.)  That being something like a couple few weeks after a merge to Aurora, or a couple weeks after the merge to beta (can't remember which, either is rather too long).

So, um.  I don't know where that leaves any of this.
Comment 20 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-20 16:50:18 PDT
My source for returning failure rather than NS_OK was unreliable.  So let me go look at this again.
Comment 21 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-20 16:53:09 PDT
So.  Looking at this again, I think the nsIJSON implementation changes are okay, and nothing needs to happen, contra my review comments.  The other changes (comments, etc.) still remain valid.
Comment 22 Nils Maier [:nmaier] 2011-07-20 16:57:03 PDT
Created attachment 547295 [details] [diff] [review]
patch v4, re-implement encode/decode again

- Fixed JSVAL_IS_OBJECT vs. JSVAL_IS_PRIMITIVE
- Fixed, amended and clarified some comments

Furthermore I did not [deprecate] decodeFromStream as I think was suggested, as it is actually used within the mozilla code base and is not a broken implementation (just encode*() is).
Comment 23 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-21 17:04:59 PDT
Comment on attachment 547295 [details] [diff] [review]
patch v4, re-implement encode/decode again

Review of attachment 547295 [details] [diff] [review]:
-----------------------------------------------------------------

Well, to be clear, I'd kind of like to go after decodeFromStream eventually, because it doesn't actually do anything interesting that nsIScriptableInputStream.read(wanted) + JSON.parse doesn't do.  And with all that removed I think that's enough to kill nsIJSON completely, and not make anyone confused by why there are two different ways to decode JSON.  But it's not all that important now, and if there are users to fix, then yeah, we should fix them first.

But I think this passes the gauntlet, if it ends up being decided that we should run with it.  Thanks for the effort put into it, I know it wasn't exactly the most fun thing for either of us.
Comment 24 Nils Maier [:nmaier] 2011-07-21 17:29:02 PDT
(In reply to comment #23)
> Thanks for the effort put into it, I know it wasn't
> exactly the most fun thing for either of us.

Yeah, I suppose it wasn't.
Thanks for the review!

Does this need sr, and if so, who would be appropriate and free to ask?
Comment 25 David Mandelin [:dmandelin] 2011-07-21 18:17:02 PDT
Comment on attachment 547295 [details] [diff] [review]
patch v4, re-implement encode/decode again

(In reply to comment #24)
> (In reply to comment #23)
> > Thanks for the effort put into it, I know it wasn't
> > exactly the most fun thing for either of us.
> 
> Yeah, I suppose it wasn't.
> Thanks for the review!
> 
> Does this need sr, and if so, who would be appropriate and free to ask?

Being an API change, probably should. Looks fine.
Comment 26 Nils Maier [:nmaier] 2011-07-21 19:49:37 PDT
Comment on attachment 547295 [details] [diff] [review]
patch v4, re-implement encode/decode again

First of all, central check-in needed.

Patch cleanly applies to aurora, so requesting approval.
Comment 27 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-21 20:06:46 PDT
Er, wait.  I reviewed this now so that *if* we decided to take it, it would be ready to land without delay.  Quoting comment 23, emphasis added:

> But I think this passes the gauntlet, *if* it ends up being decided that we
> should run with it.

As I understand it, we have not decided to take this yet.  If and when that's decided, only then should you request that it be checked in.
Comment 28 Jorge Villalobos [:jorgev] 2011-07-21 20:44:39 PDT
(In reply to comment #27)
> As I understand it, we have not decided to take this yet.  If and when
> that's decided, only then should you request that it be checked in.

Asa, Christian: what needs to happen here?
Comment 29 Nils Maier [:nmaier] 2011-07-21 21:29:23 PDT
(In reply to comment #27)
> As I understand it, we have not decided to take this yet.  If and when
> that's decided, only then should you request that it be checked in.

And as I understand it you're the only one opposed to reverting a breaking change and instead pushing it down add-on developers throats and by this Firefox users throats, because... well, why exactly?
Comment 30 Asa Dotzler [:asa] 2011-07-21 21:49:28 PDT
(In reply to comment #29)
> (In reply to comment #27)
> > As I understand it, we have not decided to take this yet.  If and when
> > that's decided, only then should you request that it be checked in.
> 
> And as I understand it you're the only one opposed to reverting a breaking
> change and instead pushing it down add-on developers throats and by this
> Firefox users throats, because... well, why exactly?

Nils, please don't turn this into a personal issue. We have a process and Jeff was just describing that process. Your personal comments are unhelpful and unwelcome (see item 1.3 https://bugzilla.mozilla.org/page.cgi?id=etiquette.html )

The release drivers will work with the engineering team to make a call on whether or not to back this out. If the decision is to back this out, then a patch for that backout will be created. If that patch is reviewed by the appropriate engineers and then approved for landing on a managed branch by the release drivers, then it will land and it will ship in a Firefox release.
Comment 31 Boris Zbarsky [:bz] 2011-07-21 21:54:31 PDT
Asa, the patch is already created (by Nils, note) and reviewed.  The only question is whether it should land, and if so where.
Comment 32 Asa Dotzler [:asa] 2011-07-21 22:00:12 PDT
bz, yes, I see that now. It was unclear to me from the description on the attachment. 

So Jeff's comment still stands. I just overstated the remaining issues (and Nils is still violating our Bugzilla guidelines by getting personal with Jeff like that.)

So what remains is for the release drivers to make the call as to whether and where this change lands. That will happen when someone requests approval for it to land on a release drivers managed repo.
Comment 33 Boris Zbarsky [:bz] 2011-07-21 22:07:01 PDT
Well, right now the discussion is about whether to land this on m-c, no?  Are you saying Nils should request aurora approval to get this landed on m-c?
Comment 34 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-21 22:17:56 PDT
Kind of what bz said.  I've assumed the discussion is about both, myself.  Although, I had thought the policy on getting something in aurora was that requests were only triaged after successful landing in m-c, so I guess maybe m-c is the first step, then if deemed appropriate aurora is the second.  (Which is arguably a little weird, since addon developers "don't care" about a landing that happens in m-c alone, only about aurora and what actually gets released.)

And yes, there is some level of frustration here on all sides.  I think we've pretty much kept things at a reasonable (if indisputably excited) level, and I'd request that people not react too strongly to anything that seems slightly incendiary, one way or the other.  In short, read generously:

http://prawfsblawg.blogs.com/prawfsblawg/2011/01/reading-generously.html

And with that I'll end my little homily to civility and empathy.  :-)
Comment 35 Nils Maier [:nmaier] 2011-07-21 22:23:24 PDT
(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #27)
> > > As I understand it, we have not decided to take this yet.  If and when
> > > that's decided, only then should you request that it be checked in.
> > 
> > And as I understand it you're the only one opposed to reverting a breaking
> > change and instead pushing it down add-on developers throats and by this
> > Firefox users throats, because... well, why exactly?
> 
> Nils, please don't turn this into a personal issue. We have a process and
> Jeff was just describing that process. Your personal comments are unhelpful
> and unwelcome (see item 1.3
> https://bugzilla.mozilla.org/page.cgi?id=etiquette.html )

I didn't try to attack Jeff as a person, I tried to express my growing frustration with what is happening here around this technical issue. If Jeff was personally offended, then I'm sorry.

So let me rephrase my question:
Jeff, what's the technical merit of keeping the breaking change?
Comment 36 Asa Dotzler [:asa] 2011-07-21 22:32:35 PDT
Great link, Jeff. I must remind myself to read like that regularly. 

OK. Sorry for injecting more noise than signal here. Let me try to add something more precise and useful.

Release drivers won't be making a call on whether or not this lands on m-c. That's up to the module owner/peers or otherwise responsible engineer or engineering team.

If and when someone thinks this should land on Aurora or Beta (and not all changes that land in those repos have to have landed on m-c first -- or at all) then please nominate the patch for a approval and the release drivers will look at it.
Comment 37 Brendan Eich [:brendan] 2011-07-21 22:34:42 PDT
I'm going to give an anti-etiquette ticket to anyone who over-cites marginal or potential but possibly unreal etiquette infractions. Srsly.

> https://mxr.mozilla.org/addons/search?string=nsIJSON shows more than 1000 for nsIJSON. Hence there a sensation of impending doom in the add-on land.

If this is anywhere near an accurate count, we should keep compatibility. Only by first evangelizing and even helping add-ons get off nsIJSON can we rightly yank it. This should not be controversial.

/be
Comment 38 Nicholas Nethercote [:njn] 2011-07-22 00:05:44 PDT
(In reply to comment #34)
> Although, I had thought the policy on getting something in aurora was that
> requests were only triaged after successful landing in m-c, so I guess maybe
> m-c is the first step, then if deemed appropriate aurora is the second. 
> (Which is arguably a little weird, since addon developers "don't care" about
> a landing that happens in m-c alone, only about aurora and what actually
> gets released.)

Bug 645922's patch removed these functions.  This bug's patch adds them back in.  Why don't we just back-out bug 645922's patch?  Isn't that the standard way of dealing with problems on Aurora?  I must be missing some details here.
Comment 39 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-22 00:14:35 PDT
(In reply to comment #38)
> (In reply to comment #34)
> > Although, I had thought the policy on getting something in aurora was that
> > requests were only triaged after successful landing in m-c, so I guess maybe
> > m-c is the first step, then if deemed appropriate aurora is the second. 
> > (Which is arguably a little weird, since addon developers "don't care" about
> > a landing that happens in m-c alone, only about aurora and what actually
> > gets released.)
> 
> Bug 645922's patch removed these functions.  This bug's patch adds them back
> in.  Why don't we just back-out bug 645922's patch?  Isn't that the standard
> way of dealing with problems on Aurora?  I must be missing some details here.

Especially given that bug 645922 was one of the things that snuck in at the very very end with the TM merge that originally missed the train.
Comment 40 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-22 00:32:26 PDT
Backing out isn't the only way to deal with problems: you could have a kill switch, or you could fix the problems if feasible, or you could use some other mitigation strategy.  Backing out's not the sole option.  Some parts of that patch everyone's fine with, other parts not so much, and here it was reasonably straightforward to fix the part that's arguably a problem.

For what it's worth, just in case anyone missed it, Nils changed four lines to update his extension to use JSON.parse/stringify rather than nsIJSON, from comment 13:

https://github.com/scriptish/scriptish/commit/12e6ac0f3c33a8ae06013a599f7982506ec22db5

This was similar to the magnitude of work needed to remove m-c nsIJSON.*code users.  It would be typical of almost all addons needing updating.

Regarding "snuck in": I'm willing to agree that late merge "snuck in" (and for what it's worth, I leaned toward thinking the exception wasn't worth the trouble, although not enough to argue it).  Yet I would not characterize the individual pushes that landed comfortably on time for an expected final merge that only happened late as "sneaking in".  Nothing I did there, nor that others did in sibling commits that were taken late, was intended to be "snuck in", as far as I know -- rather landed per standard procedure before the next departure.

bz suggests landing in m-c for testing, if it were to eventually go in aurora.  I continue to think few-line addon updates are reasonable, but the testing point makes sense.  Unless someone wants to do it faster, I'll push this in the morning.
Comment 41 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-22 00:34:55 PDT
(In reply to comment #40)
> Regarding "snuck in": I'm willing to agree that late merge "snuck in" (and
> for what it's worth, I leaned toward thinking the exception wasn't worth the
> trouble, although not enough to argue it).  Yet I would not characterize the
> individual pushes that landed comfortably on time for an expected final
> merge that only happened late as "sneaking in".  Nothing I did there, nor
> that others did in sibling commits that were taken late, was intended to be
> "snuck in", as far as I know -- rather landed per standard procedure before
> the next departure.

Sorry, "snuck in" was probably the wrong phrase to use here, given the bad faith that has already been implied here.  It was not my intention to add to that.
Comment 42 Jeff Walden [:Waldo] (remove +bmo to email) 2011-07-22 00:39:36 PDT
Okay, understood.  And arguably I could have read my own comment 34 for that paragraph of response, too.  :-)
Comment 43 Dão Gottwald [:dao] 2011-07-22 05:11:16 PDT
Nils did everything right: checkin-needed for central and approval request for aurora.
Comment 44 Jorge Villalobos [:jorgev] 2011-07-22 07:14:54 PDT
(In reply to comment #38)
> Bug 645922's patch removed these functions.  This bug's patch adds them back
> in.  Why don't we just back-out bug 645922's patch?  Isn't that the standard
> way of dealing with problems on Aurora?  I must be missing some details here.

I think we want more than just delay the change. What we want is a deprecation period were add-on developers get a visible warning that they need to switch to a different method of doing things. There is a non-trivial fraction of developers who we can't reach with our messaging, and there's a chance that having at least one release cycle where we do this will reduce the impact of this change when it is implemented.

On a somewhat related note, the Firefox 7 compatibility bump on AMO will happen sometime today or early next week, including a warning about nsIJSON which will prevent the automatic upgrade for affected add-ons.
Comment 45 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-22 07:30:23 PDT
(In reply to comment #44)
> On a somewhat related note, the Firefox 7 compatibility bump on AMO will
> happen sometime today or early next week, including a warning about nsIJSON
> which will prevent the automatic upgrade for affected add-ons.

Will there be an additional bump down the road after we land this patch for Aurora?
Comment 46 Jorge Villalobos [:jorgev] 2011-07-22 07:33:38 PDT
Yes, I think it would be wise to include this particular validation in all future bumps until the functions are finally removed.
Comment 47 Nils Maier [:nmaier] 2011-07-22 07:36:03 PDT
(In reply to comment #44)
> On a somewhat related note, the Firefox 7 compatibility bump on AMO will
> happen sometime today or early next week, including a warning about nsIJSON
> which will prevent the automatic upgrade for affected add-ons.

Seeing that nsIJSON is the major (or even only?) compat issue that will be looked at in amo-validator, can the compat testing be delayed until this bug is resolved?
From what I gather, this bug is ready to land on m-c, and chances aren't exactly bad this lands in aurora, too, so not bumping the affected add-ons right now (and by that creating yet another bunch of seemingly abandoned, not-fx7 add-ons) seems premature...
Comment 48 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-22 07:37:23 PDT
I don't understand.  If we're going to prevent automatic upgrade of addons using nsIJSON, what's the point of keeping it around?
Comment 49 Jorge Villalobos [:jorgev] 2011-07-22 07:42:32 PDT
(In reply to comment #47)
> Seeing that nsIJSON is the major (or even only?) compat issue that will be
> looked at in amo-validator, can the compat testing be delayed until this bug
> is resolved?

It is not the only one that we're checking for, but I'll consult with fligtar about this. Waiting for a new version of Aurora to pushed to be pushed to all users could delay the bump too much and our messaging would then come out too late in the process (note that we should ideally do these bumps right after the first Aurora push).

(In reply to comment #48)
> I don't understand.  If we're going to prevent automatic upgrade of addons
> using nsIJSON, what's the point of keeping it around?

The bump only affects add-ons with compatibility between 6.0 and 7.0a2. Add-ons with lower compatibility are not affected, and therefore the developers won't be messaged about this. Furthermore, there are a very large amount of developers that work outside of AMO and are also not involved in the bump. Having a deprecation period in a public version is the only way to potentially reach all of these developers without breaking their add-ons right away. There will always be a number of developers that won't heed any warnings, but that's their problem.
Comment 50 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-07-22 07:43:53 PDT
Aurora versions are pushed nightly, so we're talking about at most a one-weekend delay here.
Comment 51 :Ehsan Akhgari (busy, don't ask for review please) 2011-07-22 14:21:20 PDT
http://hg.mozilla.org/mozilla-central/rev/590784923cf7
Comment 52 Nicholas Nethercote [:njn] 2011-07-22 14:49:28 PDT
Please let's not announce publically that these functions will be removed and then announce that they're back.  Let's also not do the 7.0a2 auto-compatibility bump based on these functions being removed, because that's just wrong.

In other words, let's not confuse everyone, pretty please?
Comment 53 Jorge Villalobos [:jorgev] 2011-07-22 15:40:29 PDT
(In reply to comment #52)
> In other words, let's not confuse everyone, pretty please?

The blog announced the change, but it also mentioned the possibility of being reverted. If this is accepted for Aurora, then we will modify the compatibility validation so that the usage of nsIJSON is only a warning a not an error that prevents the bump.

Finally, can somebody explain to me what the timeline of this patch will be? I know it was pushed to m-c for testing. How long will that take until it is considered appropriate for Aurora? And after it is pushed to that branch, is there also a testing period, or will it be pushed to users on the same day?
Comment 54 christian 2011-07-26 13:52:23 PDT
Comment on attachment 547295 [details] [diff] [review]
patch v4, re-implement encode/decode again

Approved for releases/mozilla-aurora. Let's get this in so we can do the automated bump.
Comment 56 Daniel Veditz [:dveditz] 2011-07-26 14:10:40 PDT
(In reply to comment #53)
> Finally, can somebody explain to me what the timeline of this patch will be?
> I know it was pushed to m-c for testing. How long will that take until it is
> considered appropriate for Aurora? And after it is pushed to that branch, is
> there also a testing period, or will it be pushed to users on the same day?

Can't answer your other questions, but Aurora pushes almost like Nightlies except only on days when someone has actually checked something in. Early in Aurora that might be every day like Nightly, toward the end we hope it's less frequent.

tl;dr -- users will see updates with this fix the day after the fix lands.
Comment 57 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 16:10:51 PDT
qa- as no QA fix verification needed

Note You need to log in before you can comment on or make changes to this bug.