Bug 525299 - Make Library left pane creation more robust, r=dietrich

browser/components/places/content/utils.js

@@ -1138,64 +1138,146 @@ var PlacesUIUtils = {
 
   // Get the folder id for the organizer left-pane folder.
   get leftPaneFolderId() {
-    var leftPaneRoot = -1;
-    var allBookmarksId;
+    let leftPaneRoot = -1;
+    let allBookmarksId;
 
     // Shortcuts to services.
-    var bs = PlacesUtils.bookmarks;
-    var as = PlacesUtils.annotations;
+    let bs = PlacesUtils.bookmarks;
+    let as = PlacesUtils.annotations;
 
-    // Get all items marked as being the left pane folder.  We should only have
-    // one of them.
-    var items = as.getItemsWithAnnotation(ORGANIZER_FOLDER_ANNO);
+    // This is the list of the left pane queries.
+    let queries = {
+      "PlacesRoot": { title: "" },
+      "History": { title: this.getString("OrganizerQueryHistory") },
+      "Tags": { title: this.getString("OrganizerQueryTags") },
+      "AllBookmarks": { title: this.getString("OrganizerQueryAllBookmarks") },
+      "BookmarksToolbar":
+        { title: null,
+          concreteTitle: PlacesUtils.getString("BookmarksToolbarFolderTitle"),
+          concreteId: PlacesUtils.toolbarFolderId },
+      "BookmarksMenu":
+        { title: null,
+          concreteTitle: PlacesUtils.getString("BookmarksMenuFolderTitle"),
+          concreteId: PlacesUtils.bookmarksMenuFolderId },
+      "UnfiledBookmarks":
+        { title: null,
+          concreteTitle: PlacesUtils.getString("UnsortedBookmarksFolderTitle"),
+          concreteId: PlacesUtils.unfiledBookmarksFolderId },
+    };
+    // All queries but PlacesRoot.
+    const EXPECTED_QUERY_COUNT = 6;
+
+    // Removes an item and associated annotations, ignoring eventual errors.
+    function safeRemoveItem(aItemId) {
+      try {
+        if (as.itemHasAnnotation(aItemId, ORGANIZER_QUERY_ANNO) &&
+            !(as.getItemAnnotation(aItemId, ORGANIZER_QUERY_ANNO) in queries)) {
+          // Some extension annotated their roots with our query annotation,
+          // so we should not delete them.
+          return;
+        }
+        // removeItemAnnotation does not check if item exists, nor the anno,
+        // so this is safe to do.
+        as.removeItemAnnotation(aItemId, ORGANIZER_FOLDER_ANNO);
+        as.removeItemAnnotation(aItemId, ORGANIZER_QUERY_ANNO);
+        // This will throw if the annotation is an orphan.
+        bs.removeItem(aItemId);
+      }
+      catch(e) { /* orphan anno */ }
+    }
+
+    // Returns true if item really exists, false otherwise.
+    function itemExists(aItemId) {
+      try {
+        bs.getItemIndex(aItemId);
+        return true;
+      }
+      catch(e) {
+        return false;
+      }
+    }
+
+    // Get all items marked as being the left pane folder.
+    let items = as.getItemsWithAnnotation(ORGANIZER_FOLDER_ANNO, {});
     if (items.length > 1) {
       // Something went wrong, we cannot have more than one left pane folder,
       // remove all left pane folders and continue.  We will create a new one.
-      items.forEach(bs.removeItem);
+      items.forEach(safeRemoveItem);
     }
     else if (items.length == 1 && items[0] != -1) {
       leftPaneRoot = items[0];
-      // Check organizer left pane version.
-      var version = as.getItemAnnotation(leftPaneRoot, ORGANIZER_FOLDER_ANNO);
-      if (version != ORGANIZER_LEFTPANE_VERSION) {
-        // If version is not valid we must rebuild the left pane.
-        bs.removeItem(leftPaneRoot);
+
+      // Check that organizer left pane root is valid.
+      let version = as.getItemAnnotation(leftPaneRoot, ORGANIZER_FOLDER_ANNO);
+      if (version != ORGANIZER_LEFTPANE_VERSION || !itemExists(leftPaneRoot)) {
+        // Invalid root, we must rebuild the left pane.
+        safeRemoveItem(leftPaneRoot);
         leftPaneRoot = -1;
       }
     }
 
-    var queriesTitles = {
-      "PlacesRoot": "",
-      "History": this.getString("OrganizerQueryHistory"),
-      // TODO: Bug 489681, Tags needs its own string in places.properties
-      "Tags": bs.getItemTitle(PlacesUtils.tagsFolderId),
-      "AllBookmarks": this.getString("OrganizerQueryAllBookmarks"),
-      "Downloads": this.getString("OrganizerQueryDownloads"),
-      "BookmarksToolbar": null,
-      "BookmarksMenu": null,
-      "UnfiledBookmarks": null
-    };
-
     if (leftPaneRoot != -1) {
       // A valid left pane folder has been found.
-      // Build the leftPaneQueries Map.  This is used to quickly access them
+      // Build the leftPaneQueries Map.  This is used to quickly access them,
       // associating a mnemonic name to the real item ids.
       delete this.leftPaneQueries;
       this.leftPaneQueries = {};
-      var items = as.getItemsWithAnnotation(ORGANIZER_QUERY_ANNO);
-      // While looping through queries we will also check for titles validity.
-      for (var i = 0; i < items.length; i++) {
-        var queryName = as.getItemAnnotation(items[i], ORGANIZER_QUERY_ANNO);
-        this.leftPaneQueries[queryName] = items[i];
+
+      let items = as.getItemsWithAnnotation(ORGANIZER_QUERY_ANNO, {});
+      // While looping through queries we will also check for their validity.
+      let queriesCount = 0;
+      for(let i = 0; i < items.length; i++) {
+        let queryName = as.getItemAnnotation(items[i], ORGANIZER_QUERY_ANNO);
+        // Some extension did use our annotation to decorate their items
+        // with icons, so we should check only our elements, to avoid dataloss.
+        if (!(queryName in queries))
+          continue;
+
+        let query = queries[queryName];
+        query.itemId = items[i];
+
+        if (!itemExists(query.itemId)) {
+          // Orphan annotation, bail out and create a new left pane root.
+          break;
+        }
+
+        // Check that all queries have valid parents.
+        let parentId = bs.getFolderIdForItem(query.itemId);
+        if (items.indexOf(parentId) == -1 && parentId != leftPaneRoot) {
+          // The parent is not part of the left pane, bail out and create a new
+          // left pane root.
+          break;
+        }
+
         // Titles could have been corrupted or the user could have changed his
-        // locale.  Check title is correctly set and eventually fix it.
-        if (bs.getItemTitle(items[i]) != queriesTitles[queryName])
-          bs.setItemTitle(items[i], queriesTitles[queryName]);
+        // locale.  Check title and eventually fix it.
+        if (bs.getItemTitle(query.itemId) != query.title)
+          bs.setItemTitle(query.itemId, query.title);
+        if ("concreteId" in query) {
+          if (bs.getItemTitle(query.concreteId) != query.concreteTitle)
+            bs.setItemTitle(query.concreteId, query.concreteTitle);
+        }
+
+        // Add the query to our cache.
+        this.leftPaneQueries[queryName] = query.itemId;
+        queriesCount++;
+      }
+
+      if (queriesCount != EXPECTED_QUERY_COUNT) {
+        // Queries number is wrong, so the left pane must be corrupt.
+        // Note: we can't just remove the leftPaneRoot, because some query could
+        // have a bad parent, so we have to remove all items one by one.
+        items.forEach(safeRemoveItem);
+        safeRemoveItem(leftPaneRoot);
+      }
+      else {
+        // Everything is fine, return the current left pane folder.
+        delete this.leftPaneFolderId;
+        return this.leftPaneFolderId = leftPaneRoot;
       }
-      delete this.leftPaneFolderId;
-      return this.leftPaneFolderId = leftPaneRoot;
     }
 
+    // Create a new left pane folder.
     var self = this;
     var callback = {
       // Helper to create an organizer special query.
@@ -1203,7 +1285,7 @@ var PlacesUIUtils = {
         let itemId = bs.insertBookmark(aParentId,
                                        PlacesUtils._uri(aQueryUrl),
                                        bs.DEFAULT_INDEX,
-                                       queriesTitles[aQueryName]);
+                                       queries[aQueryName].title);
         // Mark as special organizer query.
         as.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO, aQueryName,
                              0, as.EXPIRE_NEVER);
@@ -1219,7 +1301,7 @@ var PlacesUIUtils = {
       create_folder: function CB_create_folder(aFolderName, aParentId, aIsRoot) {
               // Left Pane Root Folder.
         let folderId = bs.createFolder(aParentId,
-                                       queriesTitles[aFolderName],
+                                       queries[aFolderName].title,
                                        bs.DEFAULT_INDEX);
         // We should never backup this, since it changes between profiles.
         as.setItemAnnotation(folderId, EXCLUDE_FROM_BACKUP_ANNO, 1,

browser/components/places/tests/browser/browser_library_left_pane_fixnames.js

@@ -62,6 +62,10 @@ var windowObserver = {
             var query = leftPaneQueries[i];
             is(PlacesUtils.bookmarks.getItemTitle(query.itemId),
                query.correctTitle, "Title is correct for query " + query.name);
+            if ("concreteId" in query) {
+              is(PlacesUtils.bookmarks.getItemTitle(query.concreteId),
+               query.concreteTitle, "Concrete title is correct for query " + query.name);
+            }
           }
 
           // Close Library window.
@@ -105,12 +109,28 @@ function test() {
     var queryName = PlacesUtils.annotations
                                .getItemAnnotation(items[i],
                                                   ORGANIZER_QUERY_ANNO);
-    leftPaneQueries.push({ name: queryName,
-                           itemId: itemId,
-                           correctTitle: PlacesUtils.bookmarks
-                                                    .getItemTitle(itemId) });
+    var query = { name: queryName,
+                  itemId: itemId,
+                  correctTitle: PlacesUtils.bookmarks.getItemTitle(itemId) }
+    switch (queryName) {
+      case "BookmarksToolbar":
+        query.concreteId = PlacesUtils.toolbarFolderId;
+        query.concreteTitle = PlacesUtils.bookmarks.getItemTitle(query.concreteId);
+        break;
+      case "BookmarksMenu":
+        query.concreteId = PlacesUtils.bookmarksMenuFolderId;
+        query.concreteTitle = PlacesUtils.bookmarks.getItemTitle(query.concreteId);
+        break;
+      case "UnfiledBookmarks":
+        query.concreteId = PlacesUtils.unfiledBookmarksFolderId;
+        query.concreteTitle = PlacesUtils.bookmarks.getItemTitle(query.concreteId);
+        break;
+    }
+    leftPaneQueries.push(query);
     // Rename to a bad title.
-    PlacesUtils.bookmarks.setItemTitle(itemId, "badName");
+    PlacesUtils.bookmarks.setItemTitle(query.itemId, "badName");
+    if ("concreteId" in query)
+      PlacesUtils.bookmarks.setItemTitle(query.concreteId, "badName");
   }
 
   // Open Library, this will kick-off left pane code.

browser/components/places/tests/unit/test_leftpane_corruption_handling.js

@@ -0,0 +1,224 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Places Unit Test code.
+ *
+ * The Initial Developer of the Original Code is Mozilla Foundation.
+ * Portions created by the Initial Developer are Copyright (C) 2009
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *  Marco Bonardo <mak77@bonardo.net>
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+/**
+ * Tests that we build a working leftpane in various corruption situations.
+ */
+
+// Used to store the original leftPaneFolderId getter.
+let gLeftPaneFolderIdGetter;
+let gAllBookmarksFolderIdGetter;
+// Used to store the original left Pane status as a JSON string.
+let gReferenceJSON;
+let gLeftPaneFolderId;
+// Third party annotated folder.
+let gFolderId;
+
+// Corruption cases.
+let gTests = [
+
+  function test1() {
+    print("1. Do nothing, checks test calibration.");
+  },
+
+  function test2() {
+    print("2. Delete the left pane folder.");
+    PlacesUtils.bookmarks.removeItem(gLeftPaneFolderId);
+  },
+
+  function test3() {
+    print("3. Delete a child of the left pane folder.");
+    let id = PlacesUtils.bookmarks.getIdForItemAt(gLeftPaneFolderId, 0);
+    PlacesUtils.bookmarks.removeItem(id);
+  },
+
+  function test4() {
+    print("4. Delete AllBookmarks.");
+    PlacesUtils.bookmarks.removeItem(PlacesUIUtils.allBookmarksFolderId);
+  },
+
+  function test5() {
+    print("5. Create a duplicated left pane folder.");
+    let id = PlacesUtils.bookmarks.createFolder(PlacesUtils.unfiledBookmarksFolderId,
+                                                "PlacesRoot",
+                                                PlacesUtils.bookmarks.DEFAULT_INDEX);
+    PlacesUtils.annotations.setItemAnnotation(id, ORGANIZER_FOLDER_ANNO,
+                                              "PlacesRoot", 0,
+                                              PlacesUtils.annotations.EXPIRE_NEVER);
+  },
+
+  function test6() {
+    print("6. Create a duplicated left pane query.");
+    let id = PlacesUtils.bookmarks.createFolder(PlacesUtils.unfiledBookmarksFolderId,
+                                                "AllBookmarks",
+                                                PlacesUtils.bookmarks.DEFAULT_INDEX);
+    PlacesUtils.annotations.setItemAnnotation(id, ORGANIZER_QUERY_ANNO,
+                                              "AllBookmarks", 0,
+                                              PlacesUtils.annotations.EXPIRE_NEVER);
+  },
+
+  function test7() {
+    print("7. Remove the left pane folder annotation.");
+    PlacesUtils.annotations.removeItemAnnotation(gLeftPaneFolderId,
+                                                 ORGANIZER_FOLDER_ANNO);
+  },
+
+  function test8() {
+    print("8. Remove a left pane query annotation.");
+    PlacesUtils.annotations.removeItemAnnotation(PlacesUIUtils.allBookmarksFolderId,
+                                                 ORGANIZER_QUERY_ANNO);
+  },
+
+  function test9() {
+    print("9. Remove a child of AllBookmarks.");
+    let id = PlacesUtils.bookmarks.getIdForItemAt(PlacesUIUtils.allBookmarksFolderId, 0);
+    PlacesUtils.bookmarks.removeItem(id);
+  },
+
+];
+
+function run_test() {
+  // We want empty roots.
+  remove_all_bookmarks();
+
+  // Import PlacesUIUtils.
+  let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
+                     getService(Ci.mozIJSSubScriptLoader);
+  scriptLoader.loadSubScript("chrome://browser/content/places/utils.js", this);
+  do_check_true(!!PlacesUIUtils);
+
+  // Check getters.
+  gLeftPaneFolderIdGetter = PlacesUIUtils.__lookupGetter__("leftPaneFolderId");
+  do_check_eq(typeof(gLeftPaneFolderIdGetter), "function");
+  gAllBookmarksFolderIdGetter = PlacesUIUtils.__lookupGetter__("allBookmarksFolderId");
+  do_check_eq(typeof(gAllBookmarksFolderIdGetter), "function");
+
+  // Add a third party bogus annotated item.  Should not be removed.
+  gFolderId = PlacesUtils.bookmarks.createFolder(PlacesUtils.unfiledBookmarksFolderId,
+                                                 "test",
+                                                 PlacesUtils.bookmarks.DEFAULT_INDEX);
+  PlacesUtils.annotations.setItemAnnotation(gFolderId, ORGANIZER_QUERY_ANNO,
+                                            "test", 0,
+                                            PlacesUtils.annotations.EXPIRE_NEVER);
+
+  // Create the left pane, and store its current status, it will be used
+  // as reference value.
+  gLeftPaneFolderId = PlacesUIUtils.leftPaneFolderId;
+  gReferenceJSON = folderToJSON(gLeftPaneFolderId);
+
+  // Kick-off tests.
+  do_test_pending();
+  do_timeout(0, "run_next_test();");
+}
+
+function run_next_test() {
+  if (gTests.length) {
+    // Create corruption.
+    let test = gTests.shift();
+    test();
+    // Regenerate getters.
+    PlacesUIUtils.__defineGetter__("leftPaneFolderId", gLeftPaneFolderIdGetter);
+    gLeftPaneFolderId = PlacesUIUtils.leftPaneFolderId;
+    PlacesUIUtils.__defineGetter__("allBookmarksFolderId", gAllBookmarksFolderIdGetter);
+    // Check the new left pane folder.
+    let leftPaneJSON = folderToJSON(gLeftPaneFolderId);
+    do_check_true(compareJSON(gReferenceJSON, leftPaneJSON));
+    do_check_eq(PlacesUtils.bookmarks.getItemTitle(gFolderId), "test");
+    // Go to next test.
+    do_timeout(0, "run_next_test();");
+  }
+  else {
+    // All tests finished.
+    remove_all_bookmarks();
+    do_test_finished();
+  }
+}
+
+/**
+ * Convert a folder item id to a JSON representation of it and its contents.
+ */
+function folderToJSON(aItemId) {
+  let query = PlacesUtils.history.getNewQuery();
+  query.setFolders([aItemId], 1);
+  let options = PlacesUtils.history.getNewQueryOptions();
+  options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS;
+  let root = PlacesUtils.history.executeQuery(query, options).root;
+  let writer = {
+    value: "",
+    write: function PU_wrapNode__write(aStr, aLen) {
+      this.value += aStr;
+    }
+  };
+  PlacesUtils.serializeNodeAsJSONToOutputStream(root, writer, false, false);
+  do_check_true(writer.value.length > 0);
+  return writer.value;
+}
+
+/**
+ * Compare the JSON representation of 2 nodes, skipping everchanging properties
+ * like dates.
+ */
+function compareJSON(aNodeJSON_1, aNodeJSON_2) {
+  let JSON = Cc["@mozilla.org/dom/json;1"].createInstance(Ci.nsIJSON);
+  node1 = JSON.decode(aNodeJSON_1);
+  node2 = JSON.decode(aNodeJSON_2);
+
+  // List of properties we should not compare (expected to be different).
+  const SKIP_PROPS = ["dateAdded", "lastModified", "id"];
+
+  function compareObjects(obj1, obj2) {
+    do_check_eq(obj1.__count__, obj2.__count__);
+    for (let prop in obj1) {
+      // Skip everchanging values.
+      if (SKIP_PROPS.indexOf(prop) != -1)
+        continue;
+      // Skip undefined objects, otherwise we hang on them.
+      if (!obj1[prop])
+        continue;
+      if (typeof(obj1[prop]) == "object")
+        return compareObjects(obj1[prop], obj2[prop]);
+      if (obj1[prop] !== obj2[prop]) {
+        print(prop + ": " + obj1[prop] + "!=" + obj2[prop]);
+        return false;
+      }
+    }
+    return true;
+  }
+
+  return compareObjects(node1, node2);
+}

browser/locales/en-US/chrome/browser/places/places.properties

@@ -6,8 +6,6 @@ deleteDomainNoSelection=Delete domain
 load-js-data-url-error=For security reasons, javascript or data urls cannot be loaded from the history window or sidebar.
 noTitle=(no title)
 
-bookmarksMenuName=Bookmarks Menu
-bookmarksToolbarName=Bookmarks Toolbar
 bookmarksMenuEmptyFolder=(Empty)
 
 # LOCALIZATION NOTE (bookmarksBackupFilename) :
@@ -94,6 +92,7 @@ recentTagsTitle=Recent Tags
 OrganizerQueryHistory=History
 OrganizerQueryDownloads=Downloads
 OrganizerQueryAllBookmarks=All Bookmarks
+OrganizerQueryTags=Tags
 
 # LOCALIZATION NOTE (tagResultLabel) :
 # This is what we use to form the label (for screen readers)