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

VERIFIED FIXED in Firefox 3

Status

()

Firefox
Bookmarks & History
P2
major
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: Bayle Shanks, Assigned: dietrich)

Tracking

({late-l10n, relnote})

Trunk
Firefox 3
late-l10n, relnote
Points:
---
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [1 string])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
(Reporter)

Updated

10 years ago
Summary: e with quotation marks silently causes the backup file to be unloadable → bookmark folder name with quotation marks silently causes the backup file to be unloadable
(Reporter)

Updated

10 years ago
Version: unspecified → Trunk
(Reporter)

Updated

10 years ago
Severity: normal → major
(Reporter)

Comment 1

10 years ago
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
Component: Bookmarks → Places
QA Contact: bookmarks → places
(Reporter)

Comment 2

10 years ago
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
(Reporter)

Comment 3

10 years ago
Created attachment 311650 [details]
fix_bookmark_json.pl
(Assignee)

Updated

10 years ago
Assignee: nobody → dietrich
Severity: major → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3?
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → Firefox 3
(Assignee)

Updated

10 years ago
Severity: normal → major
(Assignee)

Comment 4

10 years ago
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.
Attachment #311731 - Flags: review?(mano)
Flags: blocking-firefox3? → blocking-firefox3+
Keywords: relnote
Priority: P1 → P2
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review mano]
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.
Attachment #311731 - Flags: review?(mano) → review-
(Assignee)

Comment 6

10 years ago
Created attachment 311917 [details] [diff] [review]
fix v2

comments addressed, new string added
Attachment #311731 - Attachment is obsolete: true
Attachment #311917 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Keywords: late-l10n
(Assignee)

Comment 7

10 years ago
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.
Attachment #311917 - Flags: approval1.9?
Comment on attachment 311917 [details] [diff] [review]
fix v2

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

r=mano either way though.
Attachment #311917 - Flags: review?(mano) → review+
(Assignee)

Comment 9

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

Updated

10 years ago
Whiteboard: [has patch][needs review mano]
(Assignee)

Updated

10 years ago
Whiteboard: [needs approval]

Updated

10 years ago
Whiteboard: [needs approval] → [needs approval][1 string]
Comment on attachment 311917 [details] [diff] [review]
fix v2

a=beltzner
Attachment #311917 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

9 years ago
Whiteboard: [needs approval][1 string] → [1 string]
(Assignee)

Comment 11

9 years ago
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Verified with Win XP from 2008042407
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.