Bug 327331 - nsNavHistory::Init is fragile and leaky in error conditions, r=brettw

browser/components/build/nsModule.cpp

@@ -81,7 +81,6 @@
 /////////////////////////////////////////////////////////////////////////////
 
 #ifdef MOZ_PLACES
-NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsNavHistory, Init)
 NS_GENERIC_FACTORY_CONSTRUCTOR(nsAnnoProtocolHandler)
 NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsAnnotationService, Init)
 NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsNavBookmarks, Init)
@@ -140,17 +139,17 @@ static const nsModuleComponentInfo components[] =
   { "Browser Navigation History",
     NS_NAVHISTORYSERVICE_CID,
     NS_NAVHISTORYSERVICE_CONTRACTID,
-    nsNavHistoryConstructor },
+    nsNavHistory::Create },
 
   { "Browser Navigation History",
     NS_NAVHISTORYSERVICE_CID,
     "@mozilla.org/browser/global-history;2",
-    nsNavHistoryConstructor },
+    nsNavHistory::Create },
 
   { "Browser Navigation History",
     NS_NAVHISTORYSERVICE_CID,
     "@mozilla.org/autocomplete/search;1?name=history",
-    nsNavHistoryConstructor },
+    nsNavHistory::Create },
 
   { "Page Annotation Service",
     NS_ANNOTATIONSERVICE_CID,

browser/components/places/src/nsNavHistory.cpp

@@ -322,6 +322,10 @@ nsNavHistory::Init()
   NS_ENSURE_TRUE(mRecentBookmark.Init(128), NS_ERROR_OUT_OF_MEMORY);
   NS_ENSURE_TRUE(mRecentRedirects.Init(128), NS_ERROR_OUT_OF_MEMORY);
 
+  rv = CreateLookupIndexes();
+  if (NS_FAILED(rv))
+    return rv;
+
   // The AddObserver calls must be the last lines in this function, because
   // this function may fail, and thus, this object would be not completely
   // initialized), but the observerservice would still keep a reference to us
@@ -351,7 +355,10 @@ nsNavHistory::Init()
     }
   }
 
-  return CreateLookupIndexes();
+  // Don't add code that can fail here! Do it up above, before we add our
+  // observers.
+
+  return NS_OK;
 }
 
 
@@ -3789,3 +3796,22 @@ nsresult BindStatementURI(mozIStorageStatement* statement, PRInt32 index,
   NS_ENSURE_SUCCESS(rv, rv);
   return NS_OK;
 }
+
+NS_METHOD
+nsNavHistory::Create(nsISupports *aOuter, REFNSIID aIID, void **aResult)
+{
+  NS_ENSURE_NO_AGGREGATION(aOuter);
+
+  if (gHistoryService)
+    return gHistoryService->QueryInterface(aIID, aResult);
+
+  nsRefPtr<nsNavHistory> serv(new nsNavHistory());
+  if (!serv)
+    return NS_ERROR_OUT_OF_MEMORY;
+
+  nsresult rv = serv->Init();
+  if (NS_FAILED(rv))
+    return rv;
+
+  return serv->QueryInterface(aIID, aResult);
+}

browser/components/places/src/nsNavHistory.h

@@ -116,6 +116,7 @@ public:
   NS_DECL_NSIOBSERVER
   NS_DECL_NSIAUTOCOMPLETESEARCH
 
+  static NS_METHOD Create(nsISupports *aOuter, REFNSIID aIID,  void **aResult);
   nsresult Init();
 
   /**
@@ -125,15 +126,12 @@ public:
    */
   static nsNavHistory* GetHistoryService()
   {
-    if (! gHistoryService) {
-      nsresult rv;
-      nsCOMPtr<nsINavHistoryService> serv(do_GetService("@mozilla.org/browser/nav-history-service;1", &rv));
-      NS_ENSURE_SUCCESS(rv, nsnull);
+    if (gHistoryService)
+      return gHistoryService;
+
+    nsCOMPtr<nsINavHistoryService> serv;
+    Create(nsnull, NS_GET_IID(nsINavHistoryService), getter_AddRefs(serv));
 
-      // our constructor should have set the static variable. If it didn't,
-      // something is wrong.
-      NS_ASSERTION(gHistoryService, "History service creation failed");
-    }
     return gHistoryService;
   }