Bug 473845 - Break a potential cycle with XPConnect and mozStroageService
authorShawn Wilsher <sdwilsh@shawnwilsher.com>
Wed, 21 Jan 2009 16:52:16 -0500
changeset 24013 44fd2796cee1a02147845b92cc0d983feaf5bc72
parent 24012 427d147720e331b82820df79436ec0cc3944ebdd
child 24015 aa69e508e312db254755a5e79ac24c18122f43c7
push idunknown
push userunknown
push dateunknown
bugs473845
milestone1.9.2a1pre
Bug 473845 - Break a potential cycle with XPConnect and mozStroageService This changeset removes a cycle that gets created between XPConnect and the storage service. This accomplishes this by releasing the cached reference to XPConnect during xpcom-shutdown. This was the last remaining leak of the browser-chrome test harness, so this also sets the leak threshold to zero for that test. r=bent
storage/src/mozStorageService.cpp
storage/src/mozStorageService.h
storage/src/mozStorageStatementJSHelper.cpp
testing/mochitest/runtests.py.in
--- a/storage/src/mozStorageService.cpp
+++ b/storage/src/mozStorageService.cpp
@@ -44,22 +44,23 @@
 #include "nsCRT.h"
 #include "plstr.h"
 #include "prinit.h"
 #include "nsAutoLock.h"
 #include "nsAutoPtr.h"
 #include "nsEmbedCID.h"
 #include "mozStoragePrivateHelpers.h"
 #include "nsIXPConnect.h"
+#include "nsIObserverService.h"
 
 #include "sqlite3.h"
 
 #include "nsIPromptService.h"
 
-NS_IMPL_THREADSAFE_ISUPPORTS1(mozStorageService, mozIStorageService)
+NS_IMPL_THREADSAFE_ISUPPORTS2(mozStorageService, mozIStorageService, nsIObserver)
 
 mozStorageService *mozStorageService::gStorageService = nsnull;
 
 mozStorageService *
 mozStorageService::GetSingleton()
 {
     if (gStorageService) {
         NS_ADDREF(gStorageService);
@@ -88,79 +89,95 @@ mozStorageService::GetSingleton()
         NS_ADDREF(gStorageService);
         if (NS_FAILED(gStorageService->Init()))
             NS_RELEASE(gStorageService);
     }
 
     return gStorageService;
 }
 
-nsIXPConnect *mozStorageService::sXPConnect = nsnull;
-
-nsIXPConnect *
+already_AddRefed<nsIXPConnect>
 mozStorageService::XPConnect()
 {
-  NS_ASSERTION(gStorageService,
-               "Can not get XPConnect without an instance of our service!");
+    NS_ASSERTION(gStorageService,
+                 "Can not get XPConnect without an instance of our service!");
 
-  if (!sXPConnect) {
-    (void)CallGetService(nsIXPConnect::GetCID(), &sXPConnect);
-    NS_ASSERTION(sXPConnect, "Could not get XPConnect!");
-  }
-  return sXPConnect;
+    // If we've been shutdown, sXPConnect will be null.  To prevent leaks, we do
+    // not cache the service after this point.
+    nsCOMPtr<nsIXPConnect> xpc(sXPConnect);
+    if (!xpc)
+        xpc = do_GetService(nsIXPConnect::GetCID());
+    NS_ASSERTION(xpc, "Could not get XPConnect!");
+    return xpc.forget();
 }
 
 mozStorageService::~mozStorageService()
 {
     // Shutdown the sqlite3 API.  Warn if shutdown did not turn out okay, but
     // there is nothing actionable we can do in that case.
     int rc = sqlite3_shutdown();
     if (rc != SQLITE_OK)
         NS_WARNING("sqlite3 did not shutdown cleanly.");
-    
+
     gStorageService = nsnull;
     PR_DestroyLock(mLock);
+}
 
+void
+mozStorageService::Shutdown()
+{
     NS_IF_RELEASE(sXPConnect);
-    sXPConnect = nsnull;
 }
 
+nsIXPConnect *mozStorageService::sXPConnect = nsnull;
+
 nsresult
 mozStorageService::Init()
 {
     mLock = PR_NewLock();
     if (!mLock)
         return NS_ERROR_OUT_OF_MEMORY;
-    
+
     // Disable memory allocation statistic collection, improving performance.
     // This must be done prior to a call to sqlite3_initialize to have any
     // effect.
     int rc = sqlite3_config(SQLITE_CONFIG_MEMSTATUS, 0);
     if (rc != SQLITE_OK)
         return ConvertResultCode(rc);
-    
+
     // Explicitly initialize sqlite3.  Although this is implicitly called by
     // various sqlite3 functions (and the sqlite3_open calls in our case),
     // the documentation suggests calling this directly.  So we do.
     rc = sqlite3_initialize();
     if (rc != SQLITE_OK)
         return ConvertResultCode(rc);
 
     // This makes multiple connections to the same database share the same pager
     // cache.  We do not need to lock here with mLock because this function is
     // only ever called from mozStorageService::GetSingleton, which will only
     // call this function once, and will not return until this function returns.
     // (It does not matter where this is called relative to sqlite3_initialize.)
     rc = sqlite3_enable_shared_cache(1);
     if (rc != SQLITE_OK)
         return ConvertResultCode(rc);
 
-    return NS_OK;
+    nsCOMPtr<nsIObserverService> os =
+        do_GetService("@mozilla.org/observer-service;1");
+    NS_ENSURE_TRUE(os, NS_ERROR_FAILURE);
+
+    nsresult rv = os->AddObserver(this, "xpcom-shutdown", PR_FALSE);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // We cache XPConnect for our language helpers.
+    (void)CallGetService(nsIXPConnect::GetCID(), &sXPConnect);
 }
 
+////////////////////////////////////////////////////////////////////////////////
+//// mozIStorageService
+
 #ifndef NS_APP_STORAGE_50_FILE
 #define NS_APP_STORAGE_50_FILE "UStor"
 #endif
 
 /* mozIStorageConnection openSpecialDatabase(in string aStorageKey); */
 NS_IMETHODIMP
 mozStorageService::OpenSpecialDatabase(const char *aStorageKey, mozIStorageConnection **_retval)
 {
@@ -284,8 +301,18 @@ mozStorageService::BackupDatabaseFile(ns
     rv = backupDB->Remove(PR_FALSE);
     NS_ENSURE_SUCCESS(rv, rv);
 
     backupDB.swap(*backup);
 
     return aDBFile->CopyTo(parentDir, fileName);
 }
 
+////////////////////////////////////////////////////////////////////////////////
+//// nsIObserver
+
+NS_IMETHODIMP
+mozStorageService::Observe(nsISupports *, const char *aTopic, const PRUnichar *)
+{
+    if (strcmp(aTopic, "xpcom-shutdown") == 0)
+        Shutdown();
+    return NS_OK;
+}
--- a/storage/src/mozStorageService.h
+++ b/storage/src/mozStorageService.h
@@ -40,52 +40,56 @@
  * ***** END LICENSE BLOCK ***** */
 
 #ifndef _MOZSTORAGESERVICE_H_
 #define _MOZSTORAGESERVICE_H_
 
 #include "nsCOMPtr.h"
 #include "nsIFile.h"
 #include "nsIObserver.h"
-#include "nsIObserverService.h"
 #include "prlock.h"
 
 #include "mozIStorageService.h"
 
 class mozStorageConnection;
 class nsIXPConnect;
 
 class mozStorageService : public mozIStorageService
+                        , public nsIObserver
 {
     friend class mozStorageConnection;
 
 public:
     // two-phase init, must call before using service
     nsresult Init();
 
     static mozStorageService *GetSingleton();
 
-    // nsISupports
     NS_DECL_ISUPPORTS
-
-    // mozIStorageService
     NS_DECL_MOZISTORAGESERVICE
+    NS_DECL_NSIOBSERVER
 
     /**
-     * Obtains a pointer to XPConnect.  This is used by language helpers.
+     * Obtains an already AddRefed pointer to XPConnect.  This is used by
+     * language helpers.
      */
-    static nsIXPConnect *XPConnect();
+    static already_AddRefed<nsIXPConnect> XPConnect();
 private:
     virtual ~mozStorageService();
 
     /**
      * Used for locking around calls when initializing connections so that we
      * can ensure that the state of sqlite3_enable_shared_cache is sane.
      */
     PRLock *mLock;
+
+    /**
+     * Shuts down the storage service, freeing all of the acquired resources.
+     */
+    void Shutdown();
 protected:
     nsCOMPtr<nsIFile> mProfileStorageFile;
 
     static mozStorageService *gStorageService;
 
     static nsIXPConnect *sXPConnect;
 };
 
--- a/storage/src/mozStorageStatementJSHelper.cpp
+++ b/storage/src/mozStorageStatementJSHelper.cpp
@@ -51,18 +51,19 @@
 #include "mozStorageStatementParams.h"
 
 #include "jsapi.h"
 
 static
 JSBool
 stepFunc(JSContext *aCtx, PRUint32, jsval *_vp)
 {
+  nsCOMPtr<nsIXPConnect> xpc(mozStorageService::XPConnect());
   nsCOMPtr<nsIXPConnectWrappedNative> wrapper;
-  nsresult rv = mozStorageService::XPConnect()->GetWrappedNativeOfJSObject(
+  nsresult rv = xpc->GetWrappedNativeOfJSObject(
     aCtx, JS_THIS_OBJECT(aCtx, _vp), getter_AddRefs(wrapper)
   );
   if (NS_FAILED(rv)) {
     JS_ReportError(aCtx, "mozIStorageStatement::step() could not obtain native statement");
     return JS_FALSE;
   }
 
   mozStorageStatement *stmt =
@@ -104,17 +105,18 @@ mozStorageStatementJSHelper::getRow(mozS
   if (state != mozIStorageStatement::MOZ_STORAGE_STATEMENT_EXECUTING)
     return NS_ERROR_UNEXPECTED;
 
   if (!aStatement->mStatementRowHolder) {
     nsCOMPtr<mozIStorageStatementRow> row =
       new mozStorageStatementRow(aStatement);
     NS_ENSURE_TRUE(row, NS_ERROR_OUT_OF_MEMORY);
 
-    rv = mozStorageService::XPConnect()->WrapNative(
+    nsCOMPtr<nsIXPConnect> xpc(mozStorageService::XPConnect());
+    rv = xpc->WrapNative(
       aCtx,
       ::JS_GetGlobalForObject(aCtx, aScopeObj),
       row,
       NS_GET_IID(mozIStorageStatementRow),
       getter_AddRefs(aStatement->mStatementRowHolder)
     );
     NS_ENSURE_SUCCESS(rv, rv);
   }
@@ -139,17 +141,18 @@ mozStorageStatementJSHelper::getParams(m
   if (state != mozIStorageStatement::MOZ_STORAGE_STATEMENT_READY)
     return NS_ERROR_UNEXPECTED;
 
   if (!aStatement->mStatementParamsHolder) {
     nsCOMPtr<mozIStorageStatementParams> params =
       new mozStorageStatementParams(aStatement);
     NS_ENSURE_TRUE(params, NS_ERROR_OUT_OF_MEMORY);
 
-    rv = mozStorageService::XPConnect()->WrapNative(
+    nsCOMPtr<nsIXPConnect> xpc(mozStorageService::XPConnect());
+    rv = xpc->WrapNative(
       aCtx,
       ::JS_GetGlobalForObject(aCtx, aScopeObj),
       params,
       NS_GET_IID(mozIStorageStatementParams),
       getter_AddRefs(aStatement->mStatementParamsHolder)
     );
     NS_ENSURE_SUCCESS(rv, rv);
   }
--- a/testing/mochitest/runtests.py.in
+++ b/testing/mochitest/runtests.py.in
@@ -460,19 +460,18 @@ def maybeForceLeakThreshold(options):
   threshold can be successfully forced for the particular Mochitest type
   and platform in use.
   """
   if options.leakThreshold == INFINITY:
     if options.chrome:
       # We don't leak running the --chrome tests.
       options.leakThreshold = 0
     elif options.browserChrome:
-      # We still leak a nondeterministic amount running browser-chrome tests.
-      # But we are close to 0 (bug), so start to prevent/detect regressions. (Bug 460548)
-      options.leakThreshold = 76500
+      # We don't leak running the --browser-chrome tests.
+      options.leakThreshold = 0
     elif options.a11y:
       # We don't leak running the --a11y tests.
       options.leakThreshold = 0
     else:
       # Normal Mochitests: no leaks.
       options.leakThreshold = 0
 
 def makeTestConfig(options):