Bug 1466856 - Handle old style folder=<id> queries correctly when importing bookmarks from JSON. r=mak
authorMark Banner <standard8@mozilla.com>
Tue, 05 Jun 2018 20:42:21 +0100
changeset 421597 c0202601892863efc8d0e76b36508b435cb80de1
parent 421596 6379a0ea5323964aa9e421340d4cf9775045dca0
child 421598 3af85b19ae18340c6ac5a352cbe1bb919bbfab57
push id34099
push userncsoregi@mozilla.com
push dateWed, 06 Jun 2018 22:00:08 +0000
treeherdermozilla-central@1ab062fd31db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1466856
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1466856 - Handle old style folder=<id> queries correctly when importing bookmarks from JSON. r=mak MozReview-Commit-ID: 3axzYIpHcsE
toolkit/components/places/BookmarkJSONUtils.jsm
toolkit/components/places/tests/unit/bookmarks.json
toolkit/components/places/tests/unit/test_bookmarks_json.js
toolkit/components/places/tests/unit/test_import_mobile_bookmarks.js
--- a/toolkit/components/places/BookmarkJSONUtils.jsm
+++ b/toolkit/components/places/BookmarkJSONUtils.jsm
@@ -9,16 +9,27 @@ ChromeUtils.import("resource://gre/modul
 ChromeUtils.import("resource://gre/modules/osfile.jsm");
 ChromeUtils.import("resource://gre/modules/PlacesUtils.jsm");
 
 Cu.importGlobalProperties(["fetch"]);
 
 ChromeUtils.defineModuleGetter(this, "PlacesBackups",
   "resource://gre/modules/PlacesBackups.jsm");
 
+// This is used to translate old folder pseudonyms in queries with their newer
+// guids.
+const OLD_BOOKMARK_QUERY_TRANSLATIONS = {
+  "PLACES_ROOT": PlacesUtils.bookmarks.rootGuid,
+  "BOOKMARKS_MENU": PlacesUtils.bookmarks.menuGuid,
+  "TAGS": PlacesUtils.bookmarks.tagsGuid,
+  "UNFILED_BOOKMARKS": PlacesUtils.bookmarks.unfiledGuid,
+  "TOOLBAR": PlacesUtils.bookmarks.toolbarGuid,
+  "MOBILE_BOOKMARKS": PlacesUtils.bookmarks.mobileGuid,
+};
+
 /**
  * Generates an hash for the given string.
  *
  * @note The generated hash is returned in base64 form.  Mind the fact base64
  * is case-sensitive if you are going to reuse this code.
  */
 function generateHash(aString) {
   let cryptoHash = Cc["@mozilla.org/security/hash;1"]
@@ -224,109 +235,117 @@ BookmarkImporter.prototype = {
     let nodes = rootNode.children.filter(node => node.root !== "tagsFolder");
 
     // If we're replacing, then erase existing bookmarks first.
     if (this._replace) {
       await PlacesUtils.bookmarks.eraseEverything({ source: this._source });
     }
 
     let folderIdToGuidMap = {};
-    let searchGuids = [];
 
     // Now do some cleanup on the imported nodes so that the various guids
     // match what we need for insertTree, and we also have mappings of folders
     // so we can repair any searches after inserting the bookmarks (see bug 824502).
     for (let node of nodes) {
       if (!node.children || node.children.length == 0)
         continue; // Nothing to restore for this root
 
       // Ensure we set the source correctly.
       node.source = this._source;
 
       // Translate the node for insertTree.
-      let [folders, searches] = translateTreeTypes(node);
+      let folders = translateTreeTypes(node);
 
       folderIdToGuidMap = Object.assign(folderIdToGuidMap, folders);
-      searchGuids = searchGuids.concat(searches);
     }
 
     // Now we can add the actual nodes to the database.
     for (let node of nodes) {
       // Drop any nodes without children, we can't insert them.
       if (!node.children || node.children.length == 0) {
         continue;
       }
 
       // Drop any roots whose guid we don't recognise - we don't support anything
       // apart from the built-in roots.
       if (!PlacesUtils.bookmarks.userContentRoots.includes(node.guid)) {
         continue;
       }
 
+      fixupSearchQueries(node, folderIdToGuidMap);
+
       await PlacesUtils.bookmarks.insertTree(node, { fixupOrSkipInvalidEntries: true });
 
       // Now add any favicons.
       try {
         insertFaviconsForTree(node);
       } catch (ex) {
         Cu.reportError(`Failed to insert favicons: ${ex}`);
       }
     }
-
-    // Now update any bookmarks with a place: search that contain an index to
-    // a folder id.
-    for (let guid of searchGuids) {
-      let searchBookmark = await PlacesUtils.bookmarks.fetch(guid);
-      let url = await fixupQuery(searchBookmark.url, folderIdToGuidMap);
-      if (url != searchBookmark.url) {
-        await PlacesUtils.bookmarks.update({ guid, url, source: this._source });
-      }
-    }
   },
 };
 
 function notifyObservers(topic, replace) {
   Services.obs.notifyObservers(null, topic, replace ? "json" : "json-append");
 }
 
 /**
+ * Iterates through a node, fixing up any place: URL queries that are found. This
+ * replaces any old (pre Firefox 62) queries that contain "folder=<id>" parts with
+ * "parent=<guid>".
+ *
+ * @param {Object} aNode The node to search.
+ * @param {Array} aFolderIdMap An array mapping of old folder IDs to new folder GUIDs.
+ */
+function fixupSearchQueries(aNode, aFolderIdMap) {
+  if (aNode.url && aNode.url.startsWith("place:")) {
+    aNode.url = fixupQuery(aNode.url, aFolderIdMap);
+  }
+  if (aNode.children) {
+    for (let child of aNode.children) {
+      fixupSearchQueries(child, aFolderIdMap);
+    }
+  }
+}
+
+/**
  * Replaces imported folder ids with their local counterparts in a place: URI.
  *
- * @param   {nsIURI} aQueryURI
+ * @param   {String} aQueryURL
  *          A place: URI with folder ids.
  * @param   {Object} aFolderIdMap
  *          An array mapping of old folder IDs to new folder GUIDs.
  * @return {String} the fixed up URI if all matched. If some matched, it returns
  *         the URI with only the matching folders included. If none matched
  *         it returns the input URI unchanged.
  */
-async function fixupQuery(aQueryURI, aFolderIdMap) {
-  const reGlobal = /folder=([0-9]+)/g;
-  const re = /([0-9]+)/;
+function fixupQuery(aQueryURL, aFolderIdMap) {
+  let invalid = false;
+  let convert = function(str, existingFolderId) {
+    let guid;
+    if (Object.keys(OLD_BOOKMARK_QUERY_TRANSLATIONS).includes(existingFolderId)) {
+      guid = OLD_BOOKMARK_QUERY_TRANSLATIONS[existingFolderId];
+    } else {
+      guid = aFolderIdMap[existingFolderId];
+      if (!guid) {
+        invalid = true;
+        return `invalidOldParentId=${existingFolderId}`;
+      }
+    }
+    return `parent=${guid}`;
+  };
 
-  // Unfortunately .replace can't handle async functions. Therefore,
-  // we find the folder guids we need to know the ids for first, then
-  // do the async request, and finally replace everything in one go.
-  let uri = aQueryURI.href;
-  let found = uri.match(reGlobal);
-  if (!found) {
-    return uri;
+  let url = aQueryURL.replace(/folder=([A-Za-z0-9_]+)/g, convert);
+  if (invalid) {
+    // One or more of the folders don't exist, cause an empty query so that
+    // we don't try to display the whole database.
+    url += "&excludeItems=1";
   }
-
-  let queryFolderGuids = [];
-  for (let folderString of found) {
-    let existingFolderId = folderString.match(re)[0];
-    queryFolderGuids.push(aFolderIdMap[existingFolderId]);
-  }
-
-  let newFolderIds = await PlacesUtils.promiseManyItemIds(queryFolderGuids);
-  let convert = function(str, p1) {
-    return "folder=" + newFolderIds.get(aFolderIdMap[p1]);
-  };
-  return uri.replace(reGlobal, convert);
+  return url;
 }
 
 /**
  * A mapping of root folder names to Guids. To help fixupRootFolderGuid.
  */
 const rootToFolderGuidMap = {
   "placesRoot": PlacesUtils.bookmarks.rootGuid,
   "bookmarksMenuFolder": PlacesUtils.bookmarks.menuGuid,
@@ -355,17 +374,16 @@ function fixupRootFolderGuid(node) {
  * @param {Object} node A node to be updated. If it contains children, they will
  *                      be updated as well.
  * @return {Array} An array containing two items:
  *       - {Object} A map of current folder ids to GUIDS
  *       - {Array} An array of GUIDs for nodes that contain query URIs
  */
 function translateTreeTypes(node) {
   let folderIdToGuidMap = {};
-  let searchGuids = [];
 
   // Do the uri fixup first, so we can be consistent in this function.
   if (node.uri) {
     node.url = node.uri;
     delete node.uri;
   }
 
   switch (node.type) {
@@ -384,21 +402,16 @@ function translateTreeTypes(node) {
       }
 
       // Record the current id and the guid so that we can update any search
       // queries later.
       folderIdToGuidMap[node.id] = node.guid;
       break;
     case PlacesUtils.TYPE_X_MOZ_PLACE:
       node.type = PlacesUtils.bookmarks.TYPE_BOOKMARK;
-
-      if (node.url && node.url.substr(0, 6) == "place:") {
-        searchGuids.push(node.guid);
-      }
-
       break;
     case PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR:
       node.type = PlacesUtils.bookmarks.TYPE_SEPARATOR;
       if ("title" in node) {
         delete node.title;
       }
       break;
     default:
@@ -435,32 +448,31 @@ function translateTreeTypes(node) {
 
   // Sometimes postData can be null, so delete it to make the validators happy.
   if (node.postData == null) {
     delete node.postData;
   }
 
   // Now handle any children.
   if (!node.children) {
-    return [folderIdToGuidMap, searchGuids];
+    return folderIdToGuidMap;
   }
 
   // First sort the children by index.
   node.children = node.children.sort((a, b) => {
     return a.index - b.index;
   });
 
   // Now do any adjustments required for the children.
   for (let child of node.children) {
-    let [folders, searches] = translateTreeTypes(child);
+    let folders = translateTreeTypes(child);
     folderIdToGuidMap = Object.assign(folderIdToGuidMap, folders);
-    searchGuids = searchGuids.concat(searches);
   }
 
-  return [folderIdToGuidMap, searchGuids];
+  return folderIdToGuidMap;
 }
 
 /**
  * Handles inserting favicons into the database for a bookmark node.
  * It is assumed the node has already been inserted into the bookmarks
  * database.
  *
  * @param {Object} node The bookmark node for icons to be inserted.
--- a/toolkit/components/places/tests/unit/bookmarks.json
+++ b/toolkit/components/places/tests/unit/bookmarks.json
@@ -259,13 +259,49 @@
           "title": "test tagged bookmark",
           "id": 15,
           "parent": 5,
           "dateAdded": 1507025843703345,
           "lastModified": 1507025844703124,
           "type": "text/x-moz-place",
           "uri": "http://example.tld/tagged",
           "tags": "foo"
+        },
+        {
+          "guid": "lOZGoFR1eXbl",
+          "title": "Bookmarks Toolbar Shortcut",
+          "dateAdded": 1507025843703345,
+          "lastModified": 1507025844703124,
+          "id": 16,
+          "type": "text/x-moz-place",
+          "uri": "place:folder=TOOLBAR"
+        },
+        {
+          "guid": "7yJWnBVhjRtP",
+          "title": "Folder Shortcut",
+          "dateAdded": 1507025843703345,
+          "lastModified": 1507025844703124,
+          "id": 17,
+          "type": "text/x-moz-place",
+          "uri": "place:folder=6"
+        },
+        {
+          "guid": "vm5QXWuWc12l",
+          "title": "Folder Shortcut 2",
+          "dateAdded": 1507025843703345,
+          "lastModified": 1507025844703124,
+          "id": 18,
+          "type": "text/x-moz-place",
+          "uri": "place:folder=6123443"
+        },
+        {
+          "guid": "Icg1XlIozA1D",
+          "title": "Folder Shortcut 3",
+          "dateAdded": 1507025843703345,
+          "lastModified": 1507025844703124,
+          "id": 18,
+          "type": "text/x-moz-place",
+          "uri": "place:folder=6&folder=BOOKMARKS_MENU"
         }
       ]
     }
   ]
 }
--- a/toolkit/components/places/tests/unit/test_bookmarks_json.js
+++ b/toolkit/components/places/tests/unit/test_bookmarks_json.js
@@ -85,17 +85,41 @@ var test_bookmarks = {
       url: "http://example.tld/"
     },
     { guid: "Cfkety492Afk",
       title: "test tagged bookmark",
       dateAdded: 1507025843703000,
       lastModified: 1507025844703000,
       url: "http://example.tld/tagged",
       tags: ["foo"],
-    }
+    },
+    { guid: "lOZGoFR1eXbl",
+      title: "Bookmarks Toolbar Shortcut",
+      dateAdded: 1507025843703000,
+      lastModified: 1507025844703000,
+      url: `place:parent=${PlacesUtils.bookmarks.toolbarGuid}`,
+    },
+    { guid: "7yJWnBVhjRtP",
+      title: "Folder Shortcut",
+      dateAdded: 1507025843703000,
+      lastModified: 1507025844703000,
+      url: `place:parent=OCyeUO5uu9FF`,
+    },
+    { guid: "vm5QXWuWc12l",
+      title: "Folder Shortcut 2",
+      dateAdded: 1507025843703000,
+      lastModified: 1507025844703000,
+      url: "place:invalidOldParentId=6123443&excludeItems=1",
+    },
+    { guid: "Icg1XlIozA1D",
+      title: "Folder Shortcut 3",
+      dateAdded: 1507025843703000,
+      lastModified: 1507025844703000,
+      url: `place:parent=OCyeUO5uu9FF&parent=${PlacesUtils.bookmarks.menuGuid}`,
+    },
   ]
 };
 
 // Exported bookmarks file pointer.
 var bookmarksExportedFile;
 
 add_task(async function test_import_bookmarks() {
   let bookmarksFile = OS.Path.join(do_get_cwd().path, "bookmarks.json");
--- a/toolkit/components/places/tests/unit/test_import_mobile_bookmarks.js
+++ b/toolkit/components/places/tests/unit/test_import_mobile_bookmarks.js
@@ -126,23 +126,19 @@ add_task(async function test_restore_mob
       index: 4,
       children: [
         { guid: "_o8e1_zxTJFg", index: 0 },
         { guid: "QCtSqkVYUbXB", index: 1 },
       ],
     }],
   }, "Should restore mobile bookmark folder contents into mobile root");
 
-  // We rewrite queries to point to the root ID instead of the name
-  // ("MOBILE_BOOKMARKS") so that we don't break them if the user downgrades
-  // to an earlier release channel. This can be removed along with the anno in
-  // bug 1306445.
   let queryById = await PlacesUtils.bookmarks.fetch("XF4yRP6bTuil");
-  equal(queryById.url.href, "place:folder=" + PlacesUtils.mobileFolderId,
-    "Should rewrite mobile query to point to root ID");
+  equal(queryById.url.href, `place:parent=${PlacesUtils.bookmarks.mobileGuid}`,
+    "Should rewrite mobile query to point to root GUID");
 
   await PlacesUtils.bookmarks.eraseEverything();
 });
 
 add_task(async function test_import_mobile_bookmarks_folder() {
   await importFromFixture("mobile_bookmarks_folder_import.json",
                            /* replace */ false);
   await importFromFixture("mobile_bookmarks_folder_merge.json",