Bug 721319 - PlacesUtils.removeLazyBookmarkObserver() doesn't always remove observers, causes browser window to leak.

r=dietrich

toolkit/components/places/PlacesCategoriesStarter.js

@@ -76,7 +76,8 @@ function PlacesCategoriesStarter()
       Cc["@mozilla.org/categorymanager;1"]
         .getService(Ci.nsICategoryManager)
         .deleteCategoryEntry("bookmarks-observer", this, false);
-      Services.obs.notifyObservers(null, "bookmarks-service-ready", null);
+      // Directly notify PlacesUtils, to ensure it catches the notification.
+      PlacesUtils.observe(null, "bookmarks-service-ready", null);
     }
   }).bind(this);
   [ "onItemAdded", "onItemRemoved", "onItemChanged", "onBeginUpdateBatch",

toolkit/components/places/PlacesUtils.jsm

@@ -320,12 +320,12 @@ var PlacesUtils = {
         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);
+          // Since we are shutting down, there's no reason to add the observers.
           this._bookmarksServiceObserversQueue.length = 0;
         }
         break;
       case "bookmarks-service-ready":
-        Services.obs.removeObserver(this, "bookmarks-service-ready", false);
+        this._bookmarksServiceReady = true;
         while (this._bookmarksServiceObserversQueue.length > 0) {
           let observer = this._bookmarksServiceObserversQueue.shift();
           this.bookmarks.addObserver(observer, false);
@@ -2120,16 +2120,6 @@ 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
@@ -2141,16 +2131,17 @@ var PlacesUtils = {
    *       notifies categories before real observers, and uses
    *       PlacesCategoriesStarter component to kick-off the registration.
    */
+  _bookmarksServiceReady: false,
   _bookmarksServiceObserversQueue: [],
   addLazyBookmarkObserver:
   function PU_addLazyBookmarkObserver(aObserver) {
-    if (this._isServiceInstantiated("@mozilla.org/browser/nav-bookmarks-service;1")) {
+    if (this._bookmarksServiceReady) {
       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.
    *
@@ -2159,7 +2150,7 @@ var PlacesUtils = {
    */
   removeLazyBookmarkObserver:
   function PU_removeLazyBookmarkObserver(aObserver) {
-    if (this._bookmarksServiceObserversQueue.length == 0) {
+    if (this._bookmarksServiceReady) {
       this.bookmarks.removeObserver(aObserver, false);
       return;
     }

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

@@ -2,6 +2,8 @@
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 function run_test() {
+  do_test_pending();
+  
   const TEST_URI = NetUtil.newURI("http://moz.org/")
   let observer = {
     QueryInterface: XPCOMUtils.generateQI([
@@ -26,7 +28,19 @@ function run_test() {
   PlacesUtils.addLazyBookmarkObserver(observer);
   PlacesUtils.removeLazyBookmarkObserver(observer);
 
+  // Add a proper lazy observer we will test.
   PlacesUtils.addLazyBookmarkObserver(observer);
+
+  // Check that we don't leak when adding and removing an observer while the
+  // bookmarks service is instantiated but no change happened (bug 721319).
+  PlacesUtils.bookmarks;
+  PlacesUtils.addLazyBookmarkObserver(observer);
+  PlacesUtils.removeLazyBookmarkObserver(observer);
+  try {
+    PlacesUtils.bookmarks.removeObserver(observer);
+    do_throw("Trying to remove a nonexisting observer should throw!");
+  } catch (ex) {}
+
   PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
                                        TEST_URI,
                                        PlacesUtils.bookmarks.DEFAULT_INDEX,