Bug 420505 - mozStorageService isn't as threadsafe as it claims to be. p=sdwilsh, r=brendan, r=bsmedberg, b1.9=sayrer
authoredward.lee@engineering.uiuc.edu
Wed, 19 Mar 2008 18:37:04 -0700
changeset 13350 42cda9062d59238d96334d0a4bb08617b46e2ae1
parent 13349 1603382f26788e72c475fa4e4dd92c7f02166e18
child 13351 b4a0dc4db376ca22f74c173c3ab5a8924a640540
push idunknown
push userunknown
push dateunknown
reviewersbrendan, bsmedberg
bugs420505
milestone1.9b5pre
Bug 420505 - mozStorageService isn't as threadsafe as it claims to be. p=sdwilsh, r=brendan, r=bsmedberg, b1.9=sayrer
storage/src/mozStorageService.cpp
storage/src/mozStorageService.h
--- a/storage/src/mozStorageService.cpp
+++ b/storage/src/mozStorageService.cpp
@@ -36,29 +36,66 @@
  * 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 ***** */
 
 #include "mozStorageService.h"
 #include "mozStorageConnection.h"
-#include "nsThreadUtils.h"
 #include "nsCRT.h"
 #include "plstr.h"
+#include "prinit.h"
+#include "nsAutoLock.h"
+#include "nsAutoPtr.h"
+#include "mozStorage.h"
 
 #include "sqlite3.h"
 
 NS_IMPL_THREADSAFE_ISUPPORTS1(mozStorageService, mozIStorageService)
 
+/**
+ * Lock used in mozStorageService::GetSingleton to ensure that we only create
+ * one instance of the storage service.  This lock is created in SingetonInit()
+ * and destroyed in mozStorageService::~mozStorageService().
+ */
+static PRLock *gSingletonLock;
+
+/**
+ * Creates the lock used in the singleton getter.  This lock is destroyed in
+ * the services destructor.
+ *
+ * @returns PR_SUCCESS if creating the lock was successful, PR_FAILURE otherwise
+ */
+static
+PRStatus
+SingletonInit()
+{
+    gSingletonLock = PR_NewLock();
+    NS_ENSURE_TRUE(gSingletonLock, PR_FAILURE);
+    return PR_SUCCESS;
+}
+
 mozStorageService *mozStorageService::gStorageService = nsnull;
 
 mozStorageService *
 mozStorageService::GetSingleton()
 {
+    // Since this service can be called by multiple threads, we have to use a
+    // a lock to test and possibly create gStorageService
+    static PRCallOnceType sInitOnce;
+    PRStatus rc = PR_CallOnce(&sInitOnce, SingletonInit);
+    if (rc != PR_SUCCESS)
+        return nsnull;
+
+    // If someone managed to start us twice, error out early.
+    if (!gSingletonLock)
+        return nsnull;
+
+    nsAutoLock lock(gSingletonLock);
     if (gStorageService) {
         NS_ADDREF(gStorageService);
         return gStorageService;
     }
     
     gStorageService = new mozStorageService();
     if (gStorageService) {
         NS_ADDREF(gStorageService);
@@ -67,24 +104,35 @@ mozStorageService::GetSingleton()
     }
     
     return gStorageService;
 }
 
 mozStorageService::~mozStorageService()
 {
     gStorageService = nsnull;
+    PR_DestroyLock(mLock);
+    PR_DestroyLock(gSingletonLock);
+    gSingletonLock = nsnull;
 }
 
 nsresult
 mozStorageService::Init()
 {
-    // this makes multiple connections to the same database share the same pager
-    // cache.
-    sqlite3_enable_shared_cache(1);
+    mLock = PR_NewLock();
+    if (!mLock)
+        return NS_ERROR_OUT_OF_MEMORY;
+
+    // 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.
+    int rc = sqlite3_enable_shared_cache(1);
+    if (rc != SQLITE_OK)
+        return ConvertResultCode(rc);
 
     return NS_OK;
 }
 
 #ifndef NS_APP_STORAGE_50_FILE
 #define NS_APP_STORAGE_50_FILE "UStor"
 #endif
 
@@ -124,47 +172,55 @@ mozStorageService::OpenSpecialDatabase(c
     NS_ADDREF(*_retval = msc);
     return NS_OK;
 }
 
 /* mozIStorageConnection openDatabase(in nsIFile aDatabaseFile); */
 NS_IMETHODIMP
 mozStorageService::OpenDatabase(nsIFile *aDatabaseFile, mozIStorageConnection **_retval)
 {
-    nsresult rv;
-
     mozStorageConnection *msc = new mozStorageConnection(this);
     if (!msc)
         return NS_ERROR_OUT_OF_MEMORY;
 
     // We want to return a valid connection regardless if it succeeded or not so
     // that consumers can backup the database if it failed.
-    (void)msc->Initialize(aDatabaseFile);
+    {
+        nsAutoLock lock(mLock);
+        (void)msc->Initialize(aDatabaseFile);
+    }
     NS_ADDREF(*_retval = msc);
 
     return NS_OK;
 }
 
 /* mozIStorageConnection openUnsharedDatabase(in nsIFile aDatabaseFile); */
 NS_IMETHODIMP
 mozStorageService::OpenUnsharedDatabase(nsIFile *aDatabaseFile, mozIStorageConnection **_retval)
 {
-    nsresult rv;
-
-    mozStorageConnection *msc = new mozStorageConnection(this);
+    nsRefPtr<mozStorageConnection> msc = new mozStorageConnection(this);
     if (!msc)
         return NS_ERROR_OUT_OF_MEMORY;
 
     // Initialize the connection, temporarily turning off shared caches so the
     // new connection gets its own cache.  Database connections are assigned
     // caches when they are opened, and they retain those caches for their
     // lifetimes, unaffected by changes to the shared caches setting, so we can
     // disable shared caches temporarily while we initialize the new connection
     // without affecting the caches currently in use by other connections.
     // We want to return a valid connection regardless if it succeeded or not so
     // that consumers can backup the database if it failed.
-    sqlite3_enable_shared_cache(0);
-    (void)msc->Initialize(aDatabaseFile);
-    sqlite3_enable_shared_cache(1);
+    {
+        nsAutoLock lock(mLock);
+        int rc = sqlite3_enable_shared_cache(0);
+        if (rc != SQLITE_OK)
+            return ConvertResultCode(rc);
+
+        (void)msc->Initialize(aDatabaseFile);
+
+        rc = sqlite3_enable_shared_cache(1);
+        if (rc != SQLITE_OK)
+            return ConvertResultCode(rc);
+    }
     NS_ADDREF(*_retval = msc);
 
     return NS_OK;
 }
--- a/storage/src/mozStorageService.h
+++ b/storage/src/mozStorageService.h
@@ -41,16 +41,17 @@
 
 #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 mozStorageService : public mozIStorageService
 {
     friend class mozStorageConnection;
@@ -64,15 +65,21 @@ public:
     // nsISupports
     NS_DECL_ISUPPORTS
 
     // mozIStorageService
     NS_DECL_MOZISTORAGESERVICE
 
 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;
 protected:
     nsCOMPtr<nsIFile> mProfileStorageFile;
 
     static mozStorageService *gStorageService;
 };
 
 #endif /* _MOZSTORAGESERVICE_H_ */