Bug 692120 - The star button doesn't need to observe bookmarks changes till bookmarks service is alive.

r=dietrich

browser/base/content/browser-places.js

@@ -971,7 +971,7 @@ var PlacesStarButton = {
   uninit: function PSB_uninit()
   {
     if (this._hasBookmarksObserver) {
-      PlacesUtils.bookmarks.removeObserver(this);
+      PlacesUtils.removeLazyBookmarkObserver(this);
     }
     if (this._pendingStmt) {
       this._pendingStmt.cancel();
@@ -1035,7 +1035,7 @@ var PlacesStarButton = {
       // Start observing bookmarks if needed.
       if (!this._hasBookmarksObserver) {
         try {
-          PlacesUtils.bookmarks.addObserver(this, false);
+          PlacesUtils.addLazyBookmarkObserver(this);
           this._hasBookmarksObserver = true;
         } catch(ex) {
           Components.utils.reportError("PlacesStarButton failed adding a bookmarks observer: " + ex);

toolkit/components/places/PlacesCategoriesStarter.js

@@ -66,6 +66,23 @@ function PlacesCategoriesStarter()
 {
   Services.obs.addObserver(this, TOPIC_GATHER_TELEMETRY, false);
   Services.obs.addObserver(this, PlacesUtils.TOPIC_SHUTDOWN, false);
+
+  // nsINavBookmarkObserver implementation.
+  let notify = (function () {
+    if (!this._notifiedBookmarksSvcReady) {
+      // For perf reasons unregister from the category, since no further
+      // notifications are needed.
+      Cc["@mozilla.org/categorymanager;1"]
+        .getService(Ci.nsICategoryManager)
+        .deleteCategoryEntry("bookmarks-observer", this, false);
+      Services.obs.notifyObservers(null, "bookmarks-service-ready", null);
+    }
+  }).bind(this);
+  [ "onItemAdded", "onItemRemoved", "onItemChanged", "onBeginUpdateBatch",
+    "onEndUpdateBatch", "onBeforeItemRemoved", "onItemVisited",
+    "onItemMoved" ].forEach(function(aMethod) {
+      this[aMethod] = notify;
+    }, this);
 }
 
 PlacesCategoriesStarter.prototype = {
@@ -106,6 +123,7 @@ PlacesCategoriesStarter.prototype = {
 
   QueryInterface: XPCOMUtils.generateQI([
     Ci.nsIObserver
+  , Ci.nsINavBookmarkObserver
   ])
 };
 

toolkit/components/places/PlacesUtils.jsm

@@ -315,9 +315,22 @@ var PlacesUtils = {
   //// nsIObserver
   observe: function PU_observe(aSubject, aTopic, aData)
   {
-    if (aTopic == this.TOPIC_SHUTDOWN) {
+    switch (aTopic) {
-      Services.obs.removeObserver(this, this.TOPIC_SHUTDOWN);
+      case this.TOPIC_SHUTDOWN:
-      this._shutdownFunctions.forEach(function (aFunc) aFunc.apply(this), this);
+        Services.obs.removeObserver(this, this.TOPIC_SHUTDOWN);
+        this._shutdownFunctions.forEach(function (aFunc) aFunc.apply(this), this);
+        if (this._bookmarksServiceObserversQueue.length > 0) {
+          Services.obs.removeObserver(this, "bookmarks-service-ready", false);
+          this._bookmarksServiceObserversQueue.length = 0;
+        }
+        break;
+      case "bookmarks-service-ready":
+        Services.obs.removeObserver(this, "bookmarks-service-ready", false);
+        while (this._bookmarksServiceObserversQueue.length > 0) {
+          let observer = this._bookmarksServiceObserversQueue.shift();
+          this.bookmarks.addObserver(observer, false);
+        }
+        break;
     }
   },
 
@@ -2116,7 +2129,56 @@ var PlacesUtils = {
         }
       }
     });
-  }
+  },
+
+  _isServiceInstantiated: function PU__isServiceInstantiated(aContractID) {
+    try {
+      return Components.manager
+                       .QueryInterface(Ci.nsIServiceManager)
+                       .isServiceInstantiatedByContractID(aContractID,
+                                                          Ci.nsISupports);
+    } catch (ex) {}
+    return false;
+  },
+
+  /**
+   * Lazily adds a bookmarks observer, waiting for the bookmarks service to be
+   * alive before registering the observer.  This is especially useful in the
+   * startup path, to avoid initializing the service just to add an observer.
+   *
+   * @param aObserver
+   *        Object implementing nsINavBookmarkObserver
+   * @note Correct functionality of lazy observers relies on the fact Places
+   *       notifies categories before real observers, and uses
+   *       PlacesCategoriesStarter component to kick-off the registration.
+   */
+  _bookmarksServiceObserversQueue: [],
+  addLazyBookmarkObserver:
+  function PU_addLazyBookmarkObserver(aObserver) {
+    if (this._isServiceInstantiated("@mozilla.org/browser/nav-bookmarks-service;1")) {
+      this.bookmarks.addObserver(aObserver, false);
+      return;
+    }
+    Services.obs.addObserver(this, "bookmarks-service-ready", false);
+    this._bookmarksServiceObserversQueue.push(aObserver);
+  },
+  /**
+   * Removes a bookmarks observer added through addLazyBookmarkObserver.
+   *
+   * @param aObserver
+   *        Object implementing nsINavBookmarkObserver
+   */
+  removeLazyBookmarkObserver:
+  function PU_removeLazyBookmarkObserver(aObserver) {
+    if (this._bookmarksServiceObserversQueue.length == 0) {
+      this.bookmarks.removeObserver(aObserver, false);
+      return;
+    }
+    let index = this._bookmarksServiceObserversQueue.indexOf(aObserver);
+    if (index != -1) {
+      this._bookmarksServiceObserversQueue.splice(index, 1);
+    }
+  },
 };
 
 /**

toolkit/components/places/nsLivemarkService.js

@@ -139,7 +139,7 @@ function LivemarkService() {
   Services.obs.addObserver(this, PlacesUtils.TOPIC_SHUTDOWN, false);
 
   // Observe bookmarks changes.
-  PlacesUtils.bookmarks.addObserver(this, false);
+  PlacesUtils.addLazyBookmarkObserver(this);
 }
 
 LivemarkService.prototype = {
@@ -194,7 +194,7 @@ LivemarkService.prototype = {
     if (aTopic == PlacesUtils.TOPIC_SHUTDOWN) {
       Services.obs.removeObserver(this, aTopic);
       // Remove bookmarks observer.
-      PlacesUtils.bookmarks.removeObserver(this);
+      PlacesUtils.removeLazyBookmarkObserver(this);
       // Stop updating livemarks.
       this.stopUpdateLivemarks();
     }

toolkit/components/places/nsNavBookmarks.cpp

@@ -301,14 +301,8 @@ nsNavBookmarks::Init()
 nsresult
 nsNavBookmarks::InitRoots(bool aForceCreate)
 {
-  // Get a read-only cloned connection to increase concurrency.
-  // It will be closed on destruction.
-  nsCOMPtr<mozIStorageConnection> DBConn;
-  mDB->MainConn()->Clone(true, getter_AddRefs(DBConn));
-  NS_ENSURE_STATE(DBConn);
-
   nsCOMPtr<mozIStorageStatement> stmt;
-  nsresult rv = DBConn->CreateStatement(NS_LITERAL_CSTRING(
+  nsresult rv = mDB->MainConn()->CreateStatement(NS_LITERAL_CSTRING(
     "SELECT root_name, folder_id FROM moz_bookmarks_roots"
   ), getter_AddRefs(stmt));
   NS_ENSURE_SUCCESS(rv, rv);

toolkit/components/places/tests/unit/test_PlacesUtils_lazyobservers.js

@@ -0,0 +1,34 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/ */
+
+function run_test() {
+  const TEST_URI = NetUtil.newURI("http://moz.org/")
+  let observer = {
+    QueryInterface: XPCOMUtils.generateQI([
+      Ci.nsINavBookmarkObserver,
+    ]),
+
+    onBeginUpdateBatch: function () {},
+    onEndUpdateBatch: function () {},
+    onItemAdded: function (aItemId, aParentId, aIndex, aItemType, aURI) {
+      do_check_true(aURI.equals(TEST_URI));
+      PlacesUtils.removeLazyBookmarkObserver(this);
+      do_test_finished();
+    },
+    onBeforeItemRemoved: function () {},
+    onItemRemoved: function () {},
+    onItemChanged: function () {},
+    onItemVisited: function () {},
+    onItemMoved: function () {},
+  };
+
+  // Check registration and removal with uninitialized bookmarks service.
+  PlacesUtils.addLazyBookmarkObserver(observer);
+  PlacesUtils.removeLazyBookmarkObserver(observer);
+
+  PlacesUtils.addLazyBookmarkObserver(observer);
+  PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
+                                       TEST_URI,
+                                       PlacesUtils.bookmarks.DEFAULT_INDEX,
+                                       "Bookmark title");
+}

toolkit/components/places/tests/unit/xpcshell.ini

@@ -125,4 +125,5 @@ skip-if = os == "android"
 [test_utils_getURLsForContainerNode.js]
 [test_utils_setAnnotationsFor.js]
 [test_PlacesUtils_asyncGetBookmarkIds.js]
+[test_PlacesUtils_lazyobservers.js]
 [test_telemetry.js]

toolkit/components/places/toolkitplaces.manifest

@@ -17,3 +17,4 @@ category history-observers nsPlacesExpiration @mozilla.org/places/expiration;1
 component {803938d5-e26d-4453-bf46-ad4b26e41114} PlacesCategoriesStarter.js
 contract @mozilla.org/places/categoriesStarter;1 {803938d5-e26d-4453-bf46-ad4b26e41114}
 category idle-daily PlacesCategoriesStarter @mozilla.org/places/categoriesStarter;1
+category bookmark-observers PlacesCategoriesStarter @mozilla.org/places/categoriesStarter;1