bookmarkbackups/bookmarks-yyyy-mm-dd.json isn't real JSON, because it includes trailing commas. Since we're changing our JSON code to no longer accept trailing commas, we need some way to migrate such old non-JSON to be real JSON -- conditioned, I guess, on the build id being updated. Or something. My brief skim of the migration stuff I could see in places code suggests there's really nothing for migrating bookmarks right now, because so far it's only been the HTML and "JSON" formats, nothing else, without serious format-internal changes. I'm thinking a quick and easy hack to implement this would be to sneak in a null-change places schema update (schema bump only, no change to database structure), detect that that case occurred somehow in browser glue where we handle places initialization, and convert or "convert" all old backups from "JSON" or JSON (if the profile is post-bug 505656) to JSON if a places upgrade needed to happen: http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js?mark=718,773#715 Marco, does this technique seem at least semi-reasonable?

hm, you could probably check databaseStatus = DATABASE_STATUS_UPGRADED, but then there is no shortcut to check the current schema version, doing it in cpp would be easier from this point of view, but not for json. We could maybe expose a databaseVersion property, then checking status and version would allow us to do these kind of upgrades easily.

Megamorph! I'm going to add a "legacy" JSON decoding mode, which bookmarks can use until Firefox <4 profiles are no longer supported. Patch might be complete now, not entirely certain -- I *think* it's okay, but I still need to do a touch more testing to make sure it handles the old way correctly. Expect that later tonight...

I think this covers it all, finally. It's about time for bug 564621 to land! While this patch builds atop that one, for easier reviewing, I expect when push time comes I'll fold the two patches into one, so that anyone build-bisecting doesn't manage to hit the first changeset and somehow hork their profile in the process.

Comment on attachment 469748 [details] [diff] [review] Reimplement the "legacy" parsing quirks in a special mode, undo a test change to exercise the legacy mode and demonstrate it's functional JS_UNLIKELY is not allowed unless a profile or benchmark proves it's desirable. r- for that, otherwise looks reasonable. Prefer &= for making sure js_FinishJSONParse is called.

Re: the &= bit, it looks like we have an existing leak in DecodeToJSVal then, and the corresponding comment disagrees with the code it precedes. The only users are popState and indexed db, and I lack knowledge of either to quickly figure out a testcase to demonstrate the leak (and backtracking through MXR callers looked like a fairly slow process), so I haven't verified that 1) there was a leak (although it seems evident) or 2) that this fixed the leak if it existed.

Comment on attachment 470154 [details] [diff] [review] Slightly rearrange to eliminate unlikely-check, &= (probably fix an existing leak, too) ># HG changeset patch ># User Jeff Walden <jwalden@mit.edu> ># Date 1282878865 14400 ># Node ID 96b14b9e624ab15f5fda0ff180c0fd9afc8faf7b ># Parent f7163eefc861d305123a8a4f8446818417b2d249 >Bug 582077 - Readd support for "legacy" decoding of non-JSON strings to the JSON encoder, to allow old Firefox profiles to continue to be read (bookmarks.json used to be encoded using a not-quite-JSON format). NOT REVIEWED YET > >diff --git a/browser/components/places/tests/unit/bookmarks.glue.json b/browser/components/places/tests/unit/bookmarks.glue.json >--- a/browser/components/places/tests/unit/bookmarks.glue.json >+++ b/browser/components/places/tests/unit/bookmarks.glue.json >@@ -1,1 +1,1 @@ >-{"title":"","id":1,"dateAdded":1233157910552624,"lastModified":1233157955206833,"type":"text/x-moz-place-container","root":"placesRoot","children":[{"title":"Bookmarks Menu","id":2,"parent":1,"dateAdded":1233157910552624,"lastModified":1233157993171424,"type":"text/x-moz-place-container","root":"bookmarksMenuFolder","children":[{"title":"examplejson","id":27,"parent":2,"dateAdded":1233157972101126,"lastModified":1233157984999673,"type":"text/x-moz-place","uri":"http://example.com/"}]},{"index":1,"title":"Bookmarks Toolbar","id":3,"parent":1,"dateAdded":1233157910552624,"lastModified":1233157972101126,"annos":[{"name":"bookmarkProperties/description","flags":0,"expires":4,"mimeType":null,"type":3,"value":"Add bookmarks to this folder to see them displayed on the Bookmarks Toolbar"}],"type":"text/x-moz-place-container","root":"toolbarFolder","children":[{"title":"examplejson","id":26,"parent":3,"dateAdded":1233157972101126,"lastModified":1233157984999673,"type":"text/x-moz-place","uri":"http://example.com/"}]},{"index":2,"title":"Tags","id":4,"parent":1,"dateAdded":1233157910552624,"lastModified":1233157910582667,"type":"text/x-moz-place-container","root":"tagsFolder","children":[]},{"index":3,"title":"Unsorted Bookmarks","id":5,"parent":1,"dateAdded":1233157910552624,"lastModified":1233157911033315,"type":"text/x-moz-place-container","root":"unfiledBookmarksFolder","children":[]}]} >\ No newline at end of file >+{"title":"","id":1,"dateAdded":1233157910552624,"lastModified":1233157955206833,"type":"text/x-moz-place-container","root":"placesRoot","children":[{"title":"Bookmarks Menu","id":2,"parent":1,"dateAdded":1233157910552624,"lastModified":1233157993171424,"type":"text/x-moz-place-container","root":"bookmarksMenuFolder","children":[{"title":"examplejson","id":27,"parent":2,"dateAdded":1233157972101126,"lastModified":1233157984999673,"type":"text/x-moz-place","uri":"http://example.com/"}]},{"index":1,"title":"Bookmarks Toolbar","id":3,"parent":1,"dateAdded":1233157910552624,"lastModified":1233157972101126,"annos":[{"name":"bookmarkProperties/description","flags":0,"expires":4,"mimeType":null,"type":3,"value":"Add bookmarks to this folder to see them displayed on the Bookmarks Toolbar"}],"type":"text/x-moz-place-container","root":"toolbarFolder","children":[{"title":"examplejson","id":26,"parent":3,"dateAdded":1233157972101126,"lastModified":1233157984999673,"type":"text/x-moz-place","uri":"http://example.com/"}]},{"index":2,"title":"Tags","id":4,"parent":1,"dateAdded":1233157910552624,"lastModified":1233157910582667,"type":"text/x-moz-place-container","root":"tagsFolder","children":[]},{"index":3,"title":"Unsorted Bookmarks","id":5,"parent":1,"dateAdded":1233157910552624,"lastModified":1233157911033315,"type":"text/x-moz-place-container","root":"unfiledBookmarksFolder","children":[]},]} >diff --git a/dom/interfaces/json/nsIJSON.idl b/dom/interfaces/json/nsIJSON.idl >--- a/dom/interfaces/json/nsIJSON.idl >+++ b/dom/interfaces/json/nsIJSON.idl >@@ -52,7 +52,7 @@ interface nsIScriptGlobalObject; > /** > * Encode and decode JSON text. > */ >-[scriptable, uuid(6fcf09ee-87d0-42ec-a72a-8d60114e974f)] >+[scriptable, uuid(a4d68b4e-0c0b-4c7c-b540-ef2f9834171f)] > interface nsIJSON : nsISupports > { > AString encode(/* in JSObject value */); >@@ -71,4 +71,25 @@ interface nsIJSON : nsISupports > > // Make sure you GCroot the result of this function before using it. > [noscript] jsval decodeToJSVal(in AString str, in JSContext cx); >+ >+ >+ /* >+ * Decode a JSON string, but also accept some strings in non-JSON format, as >+ * the decoding methods here did previously before tightening. >+ * >+ * This method is provided only as a temporary transition path for users of >+ * the old code who depended on the ability to decode leniently; new users >+ * should use the non-legacy decoding methods. >+ * >+ * @param str the string to parse >+ */ >+ void /* JSObject */ legacyDecode(in AString str); >+ >+ /* Identical to legacyDecode, but decode the contents of stream. */ >+ void /* JSObject */ legacyDecodeFromStream(in nsIInputStream stream, >+ in long contentLength); >+ >+ /* Identical to legacyDecode, but decode into a jsval. */ >+ // Make sure you GCroot the result of this function before using it. >+ [noscript] jsval legacyDecodeToJSVal(in AString str, in JSContext cx); > }; >diff --git a/dom/src/json/nsJSON.cpp b/dom/src/json/nsJSON.cpp >--- a/dom/src/json/nsJSON.cpp >+++ b/dom/src/json/nsJSON.cpp >@@ -417,7 +417,7 @@ nsJSON::DecodeToJSVal(const nsAString &s > // Since we've called JS_BeginJSONParse, we have to call JS_FinishJSONParse, > // even if JS_ConsumeJSONText fails. But if either fails, we'll report an > // error. >- ok = ok && JS_FinishJSONParse(cx, parser, JSVAL_NULL); >+ ok &= JS_FinishJSONParse(cx, parser, JSVAL_NULL); > > if (!ok) { > return NS_ERROR_UNEXPECTED; >@@ -429,7 +429,8 @@ nsJSON::DecodeToJSVal(const nsAString &s > nsresult > nsJSON::DecodeInternal(nsIInputStream *aStream, > PRInt32 aContentLength, >- PRBool aNeedsConverter) >+ PRBool aNeedsConverter, >+ DecodingMode mode /* = STRICT */) > { > nsresult rv; > nsIXPConnect *xpc = nsContentUtils::XPConnect(); >@@ -464,7 +465,7 @@ nsJSON::DecodeInternal(nsIInputStream *a > return NS_ERROR_FAILURE; > > nsRefPtr<nsJSONListener> >- jsonListener(new nsJSONListener(cx, retvalPtr, aNeedsConverter)); >+ jsonListener(new nsJSONListener(cx, retvalPtr, aNeedsConverter, mode)); > > if (!jsonListener) > return NS_ERROR_OUT_OF_MEMORY; >@@ -514,6 +515,52 @@ nsJSON::DecodeInternal(nsIInputStream *a > return NS_OK; > } > >+ >+NS_IMETHODIMP >+nsJSON::LegacyDecode(const nsAString& json) >+{ >+ const PRUnichar *data; >+ PRUint32 len = NS_StringGetData(json, &data); >+ nsCOMPtr<nsIInputStream> stream; >+ nsresult rv = NS_NewByteInputStream(getter_AddRefs(stream), >+ (const char*) data, >+ len * sizeof(PRUnichar), >+ NS_ASSIGNMENT_DEPEND); >+ NS_ENSURE_SUCCESS(rv, rv); >+ return DecodeInternal(stream, len, PR_FALSE, LEGACY); >+} >+ >+NS_IMETHODIMP >+nsJSON::LegacyDecodeFromStream(nsIInputStream *aStream, PRInt32 aContentLength) >+{ >+ return DecodeInternal(aStream, aContentLength, PR_TRUE, LEGACY); >+} >+ >+NS_IMETHODIMP >+nsJSON::LegacyDecodeToJSVal(const nsAString &str, JSContext *cx, jsval *result) >+{ >+ JSAutoRequest ar(cx); >+ >+ JSONParser *parser = JS_BeginJSONParse(cx, result); >+ NS_ENSURE_TRUE(parser, NS_ERROR_UNEXPECTED); >+ >+ JSBool ok = js_ConsumeJSONText(cx, parser, >+ (jschar*)PromiseFlatString(str).get(), >+ (uint32)str.Length(), >+ LEGACY); >+ >+ // Since we've called JS_BeginJSONParse, we have to call JS_FinishJSONParse, >+ // even if js_ConsumeJSONText fails. But if either fails, we'll report an >+ // error. >+ ok &= JS_FinishJSONParse(cx, parser, JSVAL_NULL); >+ >+ if (!ok) { >+ return NS_ERROR_UNEXPECTED; >+ } >+ >+ return NS_OK; >+} >+ > nsresult > NS_NewJSON(nsISupports* aOuter, REFNSIID aIID, void** aResult) > { >@@ -528,11 +575,13 @@ NS_NewJSON(nsISupports* aOuter, REFNSIID > } > > nsJSONListener::nsJSONListener(JSContext *cx, jsval *rootVal, >- PRBool needsConverter) >+ PRBool needsConverter, >+ DecodingMode mode /* = STRICT */) > : mNeedsConverter(needsConverter), > mJSONParser(nsnull), > mCx(cx), >- mRootVal(rootVal) >+ mRootVal(rootVal), >+ mDecodingMode(mode) > { > } > >@@ -706,7 +755,8 @@ nsJSONListener::Consume(const PRUnichar* > if (!mJSONParser) > return NS_ERROR_FAILURE; > >- if (!JS_ConsumeJSONText(mCx, mJSONParser, (jschar*) aBuffer, aByteLength)) { >+ if (!js_ConsumeJSONText(mCx, mJSONParser, (jschar*) aBuffer, aByteLength, >+ mDecodingMode)) { > Cleanup(); > return NS_ERROR_FAILURE; > } >diff --git a/dom/src/json/nsJSON.h b/dom/src/json/nsJSON.h >--- a/dom/src/json/nsJSON.h >+++ b/dom/src/json/nsJSON.h >@@ -40,6 +40,7 @@ > #define nsJSON_h__ > > #include "jsapi.h" >+#include "json.h" > #include "nsIJSON.h" > #include "nsString.h" > #include "nsCOMPtr.h" >@@ -86,9 +87,11 @@ public: > > protected: > nsresult EncodeInternal(nsJSONWriter *writer); >+ > nsresult DecodeInternal(nsIInputStream *aStream, > PRInt32 aContentLength, >- PRBool aNeedsConverter); >+ PRBool aNeedsConverter, >+ DecodingMode mode = STRICT); > nsCOMPtr<nsIURI> mURI; > }; > >@@ -98,7 +101,8 @@ NS_NewJSON(nsISupports* aOuter, REFNSIID > class nsJSONListener : public nsIStreamListener > { > public: >- nsJSONListener(JSContext *cx, jsval *rootVal, PRBool needsConverter); >+ nsJSONListener(JSContext *cx, jsval *rootVal, PRBool needsConverter, >+ DecodingMode mode); > virtual ~nsJSONListener(); > > NS_DECL_ISUPPORTS >@@ -112,6 +116,7 @@ protected: > jsval *mRootVal; > nsCOMPtr<nsIUnicodeDecoder> mDecoder; > nsCString mSniffBuffer; >+ DecodingMode mDecodingMode; > nsresult ProcessBytes(const char* aBuffer, PRUint32 aByteLength); > nsresult ConsumeConverted(const char* aBuffer, PRUint32 aByteLength); > nsresult Consume(const PRUnichar *data, PRUint32 len); >diff --git a/js/src/json.cpp b/js/src/json.cpp >--- a/js/src/json.cpp >+++ b/js/src/json.cpp >@@ -988,15 +988,16 @@ HandleData(JSContext *cx, JSONParser *jp > } > > JSBool >-js_ConsumeJSONText(JSContext *cx, JSONParser *jp, const jschar *data, uint32 len) >+js_ConsumeJSONText(JSContext *cx, JSONParser *jp, const jschar *data, uint32 len, >+ DecodingMode decodingMode) > { >- uint32 i; >+ CHECK_REQUEST(cx); > > if (*jp->statep == JSON_PARSE_STATE_INIT) { > PushState(cx, jp, JSON_PARSE_STATE_VALUE); > } > >- for (i = 0; i < len; i++) { >+ for (uint32 i = 0; i < len; i++) { > jschar c = data[i]; > switch (*jp->statep) { > case JSON_PARSE_STATE_ARRAY_INITIAL_VALUE: >@@ -1038,7 +1039,15 @@ js_ConsumeJSONText(JSContext *cx, JSONPa > *jp->statep = JSON_PARSE_STATE_ARRAY_AFTER_ELEMENT; > if (!OpenArray(cx, jp) || !PushState(cx, jp, JSON_PARSE_STATE_ARRAY_INITIAL_VALUE)) > return JS_FALSE; >- } else if (!JS_ISXMLSPACE(c)) { >+ } else if (JS_ISXMLSPACE(c)) { >+ // nothing to do >+ } else if (decodingMode == LEGACY && c == ']') { >+ if (!PopState(cx, jp)) >+ return JS_FALSE; >+ JS_ASSERT(*jp->statep == JSON_PARSE_STATE_ARRAY_AFTER_ELEMENT); >+ if (!CloseArray(cx, jp) || !PopState(cx, jp)) >+ return JS_FALSE; >+ } else { > return JSONParseError(jp, cx); > } > break; >@@ -1084,7 +1093,15 @@ js_ConsumeJSONText(JSContext *cx, JSONPa > *jp->statep = JSON_PARSE_STATE_OBJECT_IN_PAIR; > if (!PushState(cx, jp, JSON_PARSE_STATE_STRING)) > return JS_FALSE; >- } else if (!JS_ISXMLSPACE(c)) { >+ } else if (JS_ISXMLSPACE(c)) { >+ // nothing to do >+ } else if (decodingMode == LEGACY && c == '}') { >+ if (!PopState(cx, jp)) >+ return JS_FALSE; >+ JS_ASSERT(*jp->statep == JSON_PARSE_STATE_OBJECT_AFTER_PAIR); >+ if (!CloseObject(cx, jp) || !PopState(cx, jp)) >+ return JS_FALSE; >+ } else { > return JSONParseError(jp, cx); > } > break; >diff --git a/js/src/json.h b/js/src/json.h >--- a/js/src/json.h >+++ b/js/src/json.h >@@ -40,7 +40,10 @@ > /* > * JS JSON functions. > */ >-#include "jsscan.h" >+#include "jsprvtd.h" >+#include "jspubtd.h" >+#include "jsvalue.h" >+#include "jsvector.h" > > #define JSON_MAX_DEPTH 2048 > #define JSON_PARSER_BUFSIZE 1024 >@@ -116,8 +119,18 @@ struct JSONParser; > extern JSONParser * > js_BeginJSONParse(JSContext *cx, js::Value *rootVal, bool suppressErrors = false); > >-extern JSBool >-js_ConsumeJSONText(JSContext *cx, JSONParser *jp, const jschar *data, uint32 len); >+/* >+ * The type of JSON decoding to perform. Strict decoding is to-the-spec; >+ * legacy decoding accepts a few non-JSON syntaxes historically accepted by the >+ * implementation. (Full description of these deviations is deliberately >+ * omitted.) New users should use strict decoding rather than legacy decoding, >+ * as legacy decoding might be removed at a future time. >+ */ >+enum DecodingMode { STRICT, LEGACY }; >+ >+extern JS_FRIEND_API(JSBool) >+js_ConsumeJSONText(JSContext *cx, JSONParser *jp, const jschar *data, uint32 len, >+ DecodingMode decodingMode = STRICT); > > extern bool > js_FinishJSONParse(JSContext *cx, JSONParser *jp, const js::Value &reviver); >diff --git a/toolkit/components/places/src/PlacesUtils.jsm b/toolkit/components/places/src/PlacesUtils.jsm >--- a/toolkit/components/places/src/PlacesUtils.jsm >+++ b/toolkit/components/places/src/PlacesUtils.jsm >@@ -722,8 +722,13 @@ var PlacesUtils = { > case this.TYPE_X_MOZ_PLACE: > case this.TYPE_X_MOZ_PLACE_SEPARATOR: > case this.TYPE_X_MOZ_PLACE_CONTAINER: >- var JSON = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON); >- nodes = JSON.decode("[" + blob + "]"); >+ var json = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON); >+ // Old profiles (pre-Firefox 4) may contain bookmarks.json files with >+ // trailing commas, which we once accepted but no longer do -- except >+ // when decoded using the legacy decoder. This can be reverted to >+ // json.decode (better yet, to the ECMA-standard JSON.parse) when we no >+ // longer support upgrades from pre-Firefox 4 profiles. >+ nodes = json.legacyDecode("[" + blob + "]"); > break; > case this.TYPE_X_MOZ_URL: > var parts = blob.split("\n");

http://hg.mozilla.org/tracemonkey/rev/77a30f0f6a17 The revision also includes the patch from bug 564621, so one revision for that bug's patch and this bug's patch -- would be clearer to separate them, but I was wary of someone somehow hitting the intermediate state with strict-JSON-but-bad-bookmarks, so I folded them into one.