424958 - bookmark folder name with quotation marks silently causes the backup file to be unloadable

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Bayle Shanks]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9b5pre) Gecko/2008032215 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9b5pre) Gecko/2008032215 Minefield/3.0b5pre

When backing up the bookmark file, the user receives no warning that anything is amiss. 

Upon attempting to restore the bookmark backup json, the console receives this message:

*** unwrapNodes(): JSON.decode(): [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIJSON.decode]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///usr/local/lib/firefox-3.0b5pre/modules/utils.js :: PU_unwrapNodes :: line 529"  data: no]

The GUI silently fails; no bookmarks are loaded from the file, but there is no error message in the GUI.

Reproducible: Always

Steps to Reproduce:
1. Create a bookmark folder with the quotation character (") in the folder name
2. Bookmarks -> Organize bookmarks -> Import and Backup -> Backup...
3. Try to load the .json backup file that you created into a clean firefox (Bookmarks -> Organize bookmarks -> Import and Backup -> Restore...)
Actual Results:  
(see "details", above)

Expected Results:  
The backup should be able to be used, of course.

It is also bad that there is no error message in the GUI. Also, it would have been nice if the error message had been more informative than "PU_unwrapNodes :: line 529".


This is a scary bug because the user thinks they have backups but the backups won't work.

Comment 1

[Bayle Shanks]

this bug also occurs in the latest nightly;

Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9b5pre) Gecko/2008032504 Minefield/3.0b5pre

Comment 2

[Bayle Shanks]

the problem is that the backup .json file has not backslash-escaped quote characters which appear in folder titles. For instance, I found the following fragment in a backup file of mine called "Bookmarks 2008-03-25.json":

{"index":1,"title":"restaurant delivery "san diego" - Google Search",

So it's not just a problem reading in the file; the backup files produced are actually corrupt.

I've attached a short Perl script that I wrote to repair the backup files.

usage: 

fix_bookmark_json.pl Bookmarks\ 2008-03-25.json >  Bookmarks\ 2008-03-25_fixed.json

Comment 3

[Bayle Shanks]

Attachment: fix_bookmark_json.pl

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

1. escape strings in a couple of places where we hand-sew our JSON to allow for streaming
2. allow exceptions to bubble up
3. fix error prompt to actually work

i'll ask around to see if it's worth a new string here at this point. the existing one would suffice if it's not worth localizer pain for this.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 311731 [details] [diff] [review]
fix v1

>? toolkit/components/places/src/.utils.js.swp
>? toolkit/components/places/tests/bookmarks/test_418379.js
>Index: toolkit/components/places/src/utils.js
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/src/utils.js,v
>retrieving revision 1.7
>diff -u -8 -p -r1.7 utils.js
>--- toolkit/components/places/src/utils.js	19 Mar 2008 23:14:45 -0000	1.7
>+++ toolkit/components/places/src/utils.js	26 Mar 2008 04:04:10 -0000
>@@ -523,22 +523,18 @@ var PlacesUtils = {
>    */
>   unwrapNodes: function PU_unwrapNodes(blob, type) {
>     // We split on "\n"  because the transferable system converts "\r\n" to "\n"
>     var nodes = [];
>     switch(type) {
>       case this.TYPE_X_MOZ_PLACE:
>       case this.TYPE_X_MOZ_PLACE_SEPARATOR:
>       case this.TYPE_X_MOZ_PLACE_CONTAINER:
>-        try {
>-          var JSON = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON);
>-          nodes = JSON.decode("[" + blob + "]");
>-        } catch(ex) {
>-          LOG("unwrapNodes(): JSON.decode(): " + ex);
>-        }
>+        var JSON = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON);
>+        nodes = JSON.decode("[" + blob + "]");
>         break;
>       case this.TYPE_X_MOZ_URL:
>         var parts = blob.split("\n");
>         // data in this type has 2 parts per entry, so if there are fewer
>         // than 2 parts left, the blob is malformed and we should stop
>         // but drag and drop of files from the shell has parts.length = 1
>         if (parts.length != 1 && parts.length % 2)
>           break;
>@@ -953,32 +949,45 @@ var PlacesUtils = {
>   },
> 
>   /**
>    * Restores bookmarks/tags from a JSON file.
>    * WARNING: This method *removes* any bookmarks in the collection before
>    * restoring from the file.
>    */
>   restoreBookmarksFromFile: function PU_restoreBookmarksFromFile(aFile) {
>+    var errorStr = null;
>+

It's declared again later.

>     var ioSvc = Cc["@mozilla.org/network/io-service;1"].
>                  getService(Ci.nsIIOService);
>     var fileURL = ioSvc.newFileURI(aFile).QueryInterface(Ci.nsIURL);
>     var fileExtension = fileURL.fileExtension.toLowerCase();
>-    if (fileExtension == "json")
>-      this.restoreBookmarksFromJSONFile(aFile);
>+
>+    if (fileExtension == "json") {
>+      try {
>+        this.restoreBookmarksFromJSONFile(aFile);
>+      } catch(ex) {
>+        // XXX need new string
>+        errorStr = this.getString("restoreFormatError");
>+      }
>+    }
>     else {
>+      errorStr = this.getString("restoreFormatError");
>+    }
>+
>+    if (errorStr) {
>       const BRANDING_BUNDLE_URI = "chrome://branding/locale/brand.properties";
>       var brandShortName = Cc["@mozilla.org/intl/stringbundle;1"].
>                            getService(Ci.nsIStringBundleService).
>                            createBundle(BRANDING_BUNDLE_URI).
>                            GetStringFromName("brandShortName");
>       var errorStr = this.getString("restoreFormatError");

we read it again here if it was already set... to the same string?

>       Cc["@mozilla.org/embedcomp/prompt-service;1"].
>         getService(Ci.nsIPromptService).
>-        alert(window, brandShortName, errorStr);
>+        alert(null, brandShortName, errorStr);

Hrm, should be the most recent organizer window, if any.

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

comments addressed, new string added

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 311917 [details] [diff] [review]
fix v2

requesting driver approval of a new error string for when a bookmark backup is not able to be processed.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 311917 [details] [diff] [review]
fix v2

I meant most recent *organizer* window, not any window.

r=mano either way though.

Comment 9

[Dietrich Ayala (:dietrich)]

Sorry, forgot to comment on that bit: I didn't do that as the organizer identifier is only in /browser.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311917 [details] [diff] [review]
fix v2

a=beltzner

Comment 11

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/utils.js;
/cvsroot/mozilla/toolkit/components/places/src/utils.js,v  <--  utils.js
new revision: 1.9; previous revision: 1.8
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_424958-json-quoted-folders.js,v
done
Checking in toolkit/components/places/tests/bookmarks/test_424958-json-quoted-folders.js;
/cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_424958-json-quoted-folders.js,v  <--  test_424958-json-quoted-folders.js
initial revision: 1.1
done
Checking in toolkit/locales/en-US/chrome/places/places.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/places/places.properties,v  <--  places.properties
new revision: 1.6; previous revision: 1.5
done

Comment 12

[Tracy Walker [:tracy]]

Verified with Win XP from 2008042407

Comment 13

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv