Add a legacy JSON parsing mode for use by bookmarks.json ("JSON") loading from pre-Firefox 4 profiles

RESOLVED FIXED in mozilla2.0b5

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla2.0b5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

9 years ago
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.
Assignee

Comment 2

9 years ago
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...
blocking2.0: --- → ?
Component: Places → JavaScript Engine
Product: Toolkit → Core
QA Contact: places → general
Target Milestone: mozilla2.0b3 → mozilla2.0b5
Assignee

Comment 3

9 years ago
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.
Attachment #469748 - Flags: review?(sayrer)

Comment 4

9 years ago
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.
Attachment #469748 - Flags: review?(sayrer) → review-
Assignee

Comment 5

9 years ago
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.
Attachment #469748 - Attachment is obsolete: true
Attachment #470154 - Flags: review?(sayrer)

Comment 6

9 years ago
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");
Attachment #470154 - Flags: review?(sayrer) → review+

Updated

9 years ago
blocking2.0: ? → beta6+
Assignee

Comment 7

9 years ago
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.
Summary: Migrate old bookmarks backup "JSON" files to real JSON (without trailing commas) → Add a legacy JSON parsing mode for use by bookmarks.json ("JSON") loading from pre-Firefox 4 profiles
Whiteboard: fixed-in-tracemonkey

Comment 8

9 years ago
http://hg.mozilla.org/mozilla-central/rev/77a30f0f6a17
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.