Last Comment Bug 424958 - bookmark folder name with quotation marks silently causes the backup file to be unloadable
: bookmark folder name with quotation marks silently causes the backup file to ...
Status: VERIFIED FIXED
[1 string]
: late-l10n, relnote
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
P2 major (vote)
: Firefox 3
Assigned To: Dietrich Ayala (:dietrich)
:
: Marco Bonardo [::mak]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-25 03:31 PDT by Bayle Shanks
Modified: 2009-11-26 05:40 PST (History)
5 users (show)
mbeltzner: blocking‑firefox3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix_bookmark_json.pl (467 bytes, text/x-perl)
2008-03-25 14:10 PDT, Bayle Shanks
no flags Details
fix v1 (9.87 KB, patch)
2008-03-25 21:20 PDT, Dietrich Ayala (:dietrich)
asaf: review-
Details | Diff | Splinter Review
fix v2 (10.97 KB, patch)
2008-03-26 16:59 PDT, Dietrich Ayala (:dietrich)
asaf: review+
mbeltzner: approval1.9+
Details | Diff | Splinter Review

Description User image Bayle Shanks 2008-03-25 03:31:39 PDT
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 User image Bayle Shanks 2008-03-25 11:41:34 PDT
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 User image Bayle Shanks 2008-03-25 14:07:36 PDT
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 User image Bayle Shanks 2008-03-25 14:10:55 PDT
Created attachment 311650 [details]
fix_bookmark_json.pl
Comment 4 User image Dietrich Ayala (:dietrich) 2008-03-25 21:20:45 PDT
Created attachment 311731 [details] [diff] [review]
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 User image Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-26 13:09:17 PDT
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 User image Dietrich Ayala (:dietrich) 2008-03-26 16:59:49 PDT
Created attachment 311917 [details] [diff] [review]
fix v2

comments addressed, new string added
Comment 7 User image Dietrich Ayala (:dietrich) 2008-03-26 17:00:38 PDT
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 User image Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-03-26 17:18:20 PDT
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 User image Dietrich Ayala (:dietrich) 2008-03-26 17:35:54 PDT
Sorry, forgot to comment on that bit: I didn't do that as the organizer identifier is only in /browser.
Comment 10 User image Mike Beltzner [:beltzner, not reading bugmail] 2008-03-29 13:58:11 PDT
Comment on attachment 311917 [details] [diff] [review]
fix v2

a=beltzner
Comment 11 User image Dietrich Ayala (:dietrich) 2008-03-31 09:58:33 PDT
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 User image Tracy Walker [:tracy] 2008-04-24 12:23:25 PDT
Verified with Win XP from 2008042407
Comment 13 User image Gervase Markham [:gerv] 2009-11-26 05:40:45 PST
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

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