Bug 1350637 - Part 6: Fix a deadlock when main process storage child actor triggers storage thread initialization. r=asuth, a=lizzard
authorJan Varga <jan.varga@gmail.com>
Tue, 08 Aug 2017 23:01:52 +0200
changeset 421299 e77772fae3c2
parent 421298 9c97e6d93100
child 421300 8267e1dbf9bc
push id7647
push userryanvm@gmail.com
push dateMon, 21 Aug 2017 19:15:39 +0000
treeherdermozilla-beta@6a2167880016 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersasuth, lizzard
bugs1350637
milestone56.0
Bug 1350637 - Part 6: Fix a deadlock when main process storage child actor triggers storage thread initialization. r=asuth, a=lizzard
dom/storage/StorageDBThread.cpp
dom/storage/StorageDBThread.h
dom/storage/StorageIPC.cpp
dom/storage/StorageIPC.h
ipc/glue/BackgroundChildImpl.cpp
ipc/glue/BackgroundChildImpl.h
ipc/glue/BackgroundParentImpl.cpp
ipc/glue/BackgroundParentImpl.h
ipc/glue/PBackground.ipdl
--- a/dom/storage/StorageDBThread.cpp
+++ b/dom/storage/StorageDBThread.cpp
@@ -192,50 +192,88 @@ StorageDBThread::Get()
 {
   AssertIsOnBackgroundThread();
 
   return sStorageThread;
 }
 
 // static
 StorageDBThread*
-StorageDBThread::GetOrCreate()
+StorageDBThread::GetOrCreate(const nsString& aProfilePath)
 {
   AssertIsOnBackgroundThread();
 
   if (sStorageThread || sStorageThreadDown) {
     // When sStorageThreadDown is at true, sStorageThread is null.
     // Checking sStorageThreadDown flag here prevents reinitialization of
     // the storage thread after shutdown.
     return sStorageThread;
   }
 
   nsAutoPtr<StorageDBThread> storageThread(new StorageDBThread());
 
-  nsresult rv = storageThread->Init();
+  nsresult rv = storageThread->Init(aProfilePath);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return nullptr;
   }
 
   sStorageThread = storageThread.forget();
 
   return sStorageThread;
 }
 
+// static
 nsresult
-StorageDBThread::Init()
+StorageDBThread::GetProfilePath(nsString& aProfilePath)
+{
+  MOZ_ASSERT(XRE_IsParentProcess());
+  MOZ_ASSERT(NS_IsMainThread());
+
+  // Need to determine location on the main thread, since
+  // NS_GetSpecialDirectory accesses the atom table that can
+  // only be accessed on the main thread.
+  nsCOMPtr<nsIFile> profileDir;
+  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
+                                       getter_AddRefs(profileDir));
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
+  }
+
+  rv = profileDir->GetPath(aProfilePath);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
+  }
+
+  // This service has to be started on the main thread currently.
+  nsCOMPtr<mozIStorageService> ss =
+    do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID, &rv);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
+  }
+
+  return NS_OK;
+}
+
+nsresult
+StorageDBThread::Init(const nsString& aProfilePath)
 {
   AssertIsOnBackgroundThread();
 
-  RefPtr<InitHelper> helper = new InitHelper();
+  nsresult rv;
 
   nsString profilePath;
-  nsresult rv = helper->SyncDispatchAndReturnProfilePath(profilePath);
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
+  if (aProfilePath.IsEmpty()) {
+    RefPtr<InitHelper> helper = new InitHelper();
+
+    rv = helper->SyncDispatchAndReturnProfilePath(profilePath);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return rv;
+    }
+  } else {
+    profilePath = aProfilePath;
   }
 
   mDatabaseFile = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
 
   rv = mDatabaseFile->InitWithPath(profilePath);
@@ -1617,54 +1655,23 @@ InitHelper::SyncDispatchAndReturnProfile
   if (NS_WARN_IF(NS_FAILED(mMainThreadResultCode))) {
     return mMainThreadResultCode;
   }
 
   aProfilePath = mProfilePath;
   return NS_OK;
 }
 
-nsresult
-StorageDBThread::
-InitHelper::RunOnMainThread()
-{
-  MOZ_ASSERT(NS_IsMainThread());
-
-  // Need to determine location on the main thread, since
-  // NS_GetSpecialDirectory accesses the atom table that can
-  // only be accessed on the main thread.
-  nsCOMPtr<nsIFile> profileDir;
-  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
-                                       getter_AddRefs(profileDir));
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
-  }
-
-  rv = profileDir->GetPath(mProfilePath);
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
-  }
-
-  // This service has to be started on the main thread currently.
-  nsCOMPtr<mozIStorageService> ss =
-    do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID, &rv);
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
-  }
-
-  return NS_OK;
-}
-
 NS_IMETHODIMP
 StorageDBThread::
 InitHelper::Run()
 {
   MOZ_ASSERT(NS_IsMainThread());
 
-  nsresult rv = RunOnMainThread();
+  nsresult rv = GetProfilePath(mProfilePath);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     mMainThreadResultCode = rv;
   }
 
   mozilla::MutexAutoLock lock(mMutex);
   MOZ_ASSERT(mWaiting);
 
   mWaiting = false;
--- a/dom/storage/StorageDBThread.h
+++ b/dom/storage/StorageDBThread.h
@@ -321,19 +321,22 @@ public:
 public:
   StorageDBThread();
   virtual ~StorageDBThread() {}
 
   static StorageDBThread*
   Get();
 
   static StorageDBThread*
-  GetOrCreate();
+  GetOrCreate(const nsString& aProfilePath);
 
-  virtual nsresult Init();
+  static nsresult
+  GetProfilePath(nsString& aProfilePath);
+
+  virtual nsresult Init(const nsString& aProfilePath);
 
   // Flushes all uncommited data and stops the I/O thread.
   virtual nsresult Shutdown();
 
   virtual void AsyncPreload(LocalStorageCacheBridge* aCache,
                             bool aPriority = false)
   {
     InsertDBOp(new DBOperation(aPriority
--- a/dom/storage/StorageIPC.cpp
+++ b/dom/storage/StorageIPC.cpp
@@ -151,19 +151,27 @@ StorageDBChild::Init()
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   PBackgroundChild* actor = BackgroundChild::GetOrCreateForCurrentThread();
   if (NS_WARN_IF(!actor)) {
     return NS_ERROR_FAILURE;
   }
 
+  nsString profilePath;
+  if (XRE_IsParentProcess()) {
+    nsresult rv = StorageDBThread::GetProfilePath(profilePath);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return rv;
+    }
+  }
+
   AddIPDLReference();
 
-  actor->SendPBackgroundStorageConstructor(this);
+  actor->SendPBackgroundStorageConstructor(this, profilePath);
 
   nsCOMPtr<nsIObserverService> observerService = services::GetObserverService();
   MOZ_ASSERT(observerService);
 
   nsCOMPtr<nsIObserver> observer = new ShutdownObserver();
 
   MOZ_ALWAYS_SUCCEEDS(
     observerService->AddObserver(observer,
@@ -521,18 +529,19 @@ private:
     return NS_OK;
   }
 
   RefPtr<StorageDBParent> mParent;
 };
 
 } // namespace
 
-StorageDBParent::StorageDBParent()
-: mIPCOpen(false)
+StorageDBParent::StorageDBParent(const nsString& aProfilePath)
+  : mProfilePath(aProfilePath)
+  , mIPCOpen(false)
 {
   AssertIsOnBackgroundThread();
 
   // We are always open by IPC only
   AddIPDLReference();
 
   // Cannot send directly from here since the channel
   // is not completely built at this moment.
@@ -578,31 +587,31 @@ StorageDBParent::ActorDestroy(ActorDestr
   // Implement me! Bug 1005169
 }
 
 mozilla::ipc::IPCResult
 StorageDBParent::RecvAsyncPreload(const nsCString& aOriginSuffix,
                                   const nsCString& aOriginNoSuffix,
                                   const bool& aPriority)
 {
-  StorageDBThread* storageThread = StorageDBThread::GetOrCreate();
+  StorageDBThread* storageThread = StorageDBThread::GetOrCreate(mProfilePath);
   if (!storageThread) {
     return IPC_FAIL_NO_REASON(this);
   }
 
   storageThread->AsyncPreload(NewCache(aOriginSuffix, aOriginNoSuffix),
                               aPriority);
 
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult
 StorageDBParent::RecvAsyncGetUsage(const nsCString& aOriginNoSuffix)
 {
-  StorageDBThread* storageThread = StorageDBThread::GetOrCreate();
+  StorageDBThread* storageThread = StorageDBThread::GetOrCreate(mProfilePath);
   if (!storageThread) {
     return IPC_FAIL_NO_REASON(this);
   }
 
   // The object releases it self in LoadUsage method
   RefPtr<UsageParentBridge> usage =
     new UsageParentBridge(this, aOriginNoSuffix);
 
@@ -693,17 +702,17 @@ private:
 mozilla::ipc::IPCResult
 StorageDBParent::RecvPreload(const nsCString& aOriginSuffix,
                              const nsCString& aOriginNoSuffix,
                              const uint32_t& aAlreadyLoadedCount,
                              InfallibleTArray<nsString>* aKeys,
                              InfallibleTArray<nsString>* aValues,
                              nsresult* aRv)
 {
-  StorageDBThread* storageThread = StorageDBThread::GetOrCreate();
+  StorageDBThread* storageThread = StorageDBThread::GetOrCreate(mProfilePath);
   if (!storageThread) {
     return IPC_FAIL_NO_REASON(this);
   }
 
   RefPtr<SyncLoadCacheHelper> cache(
     new SyncLoadCacheHelper(aOriginSuffix, aOriginNoSuffix, aAlreadyLoadedCount,
                             aKeys, aValues, aRv));
 
@@ -713,17 +722,17 @@ StorageDBParent::RecvPreload(const nsCSt
 }
 
 mozilla::ipc::IPCResult
 StorageDBParent::RecvAsyncAddItem(const nsCString& aOriginSuffix,
                                   const nsCString& aOriginNoSuffix,
                                   const nsString& aKey,
                                   const nsString& aValue)
 {
-  StorageDBThread* storageThread = StorageDBThread::GetOrCreate();
+  StorageDBThread* storageThread = StorageDBThread::GetOrCreate(mProfilePath);
   if (!storageThread) {
     return IPC_FAIL_NO_REASON(this);
   }
 
   nsresult rv =
     storageThread->AsyncAddItem(NewCache(aOriginSuffix, aOriginNoSuffix),
                                 aKey,
                                 aValue);
@@ -735,17 +744,17 @@ StorageDBParent::RecvAsyncAddItem(const 
 }
 
 mozilla::ipc::IPCResult
 StorageDBParent::RecvAsyncUpdateItem(const nsCString& aOriginSuffix,
                                      const nsCString& aOriginNoSuffix,
                                      const nsString& aKey,
                                      const nsString& aValue)
 {
-  StorageDBThread* storageThread = StorageDBThread::GetOrCreate();
+  StorageDBThread* storageThread = StorageDBThread::GetOrCreate(mProfilePath);
   if (!storageThread) {
     return IPC_FAIL_NO_REASON(this);
   }
 
   nsresult rv =
     storageThread->AsyncUpdateItem(NewCache(aOriginSuffix, aOriginNoSuffix),
                                    aKey,
                                    aValue);
@@ -756,17 +765,17 @@ StorageDBParent::RecvAsyncUpdateItem(con
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult
 StorageDBParent::RecvAsyncRemoveItem(const nsCString& aOriginSuffix,
                                      const nsCString& aOriginNoSuffix,
                                      const nsString& aKey)
 {
-  StorageDBThread* storageThread = StorageDBThread::GetOrCreate();
+  StorageDBThread* storageThread = StorageDBThread::GetOrCreate(mProfilePath);
   if (!storageThread) {
     return IPC_FAIL_NO_REASON(this);
   }
 
   nsresult rv =
     storageThread->AsyncRemoveItem(NewCache(aOriginSuffix, aOriginNoSuffix),
                                    aKey);
   if (NS_FAILED(rv) && mIPCOpen) {
@@ -775,17 +784,17 @@ StorageDBParent::RecvAsyncRemoveItem(con
 
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult
 StorageDBParent::RecvAsyncClear(const nsCString& aOriginSuffix,
                                 const nsCString& aOriginNoSuffix)
 {
-  StorageDBThread* storageThread = StorageDBThread::GetOrCreate();
+  StorageDBThread* storageThread = StorageDBThread::GetOrCreate(mProfilePath);
   if (!storageThread) {
     return IPC_FAIL_NO_REASON(this);
   }
 
   nsresult rv =
     storageThread->AsyncClear(NewCache(aOriginSuffix, aOriginNoSuffix));
   if (NS_FAILED(rv) && mIPCOpen) {
     mozilla::Unused << SendError(rv);
@@ -805,55 +814,55 @@ StorageDBParent::RecvAsyncFlush()
   storageThread->AsyncFlush();
 
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult
 StorageDBParent::RecvStartup()
 {
-  StorageDBThread* storageThread = StorageDBThread::GetOrCreate();
+  StorageDBThread* storageThread = StorageDBThread::GetOrCreate(mProfilePath);
   if (!storageThread) {
     return IPC_FAIL_NO_REASON(this);
   }
 
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult
 StorageDBParent::RecvClearAll()
 {
-  StorageDBThread* storageThread = StorageDBThread::GetOrCreate();
+  StorageDBThread* storageThread = StorageDBThread::GetOrCreate(mProfilePath);
   if (!storageThread) {
     return IPC_FAIL_NO_REASON(this);
   }
 
   storageThread->AsyncClearAll();
 
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult
 StorageDBParent::RecvClearMatchingOrigin(const nsCString& aOriginNoSuffix)
 {
-  StorageDBThread* storageThread = StorageDBThread::GetOrCreate();
+  StorageDBThread* storageThread = StorageDBThread::GetOrCreate(mProfilePath);
   if (!storageThread) {
     return IPC_FAIL_NO_REASON(this);
   }
 
   storageThread->AsyncClearMatchingOrigin(aOriginNoSuffix);
 
   return IPC_OK();
 }
 
 mozilla::ipc::IPCResult
 StorageDBParent::RecvClearMatchingOriginAttributes(
                                         const OriginAttributesPattern& aPattern)
 {
-  StorageDBThread* storageThread = StorageDBThread::GetOrCreate();
+  StorageDBThread* storageThread = StorageDBThread::GetOrCreate(mProfilePath);
   if (!storageThread) {
     return IPC_FAIL_NO_REASON(this);
   }
 
   storageThread->AsyncClearMatchingOriginAttributes(aPattern);
 
   return IPC_OK();
 }
@@ -1199,25 +1208,26 @@ ObserverSink::Observe(const char* aTopic
   return NS_OK;
 }
 
 /*******************************************************************************
  * Exported functions
  ******************************************************************************/
 
 PBackgroundStorageParent*
-AllocPBackgroundStorageParent()
+AllocPBackgroundStorageParent(const nsString& aProfilePath)
 {
   AssertIsOnBackgroundThread();
 
-  return new StorageDBParent();
+  return new StorageDBParent(aProfilePath);
 }
 
 mozilla::ipc::IPCResult
-RecvPBackgroundStorageConstructor(PBackgroundStorageParent* aActor)
+RecvPBackgroundStorageConstructor(PBackgroundStorageParent* aActor,
+                                  const nsString& aProfilePath)
 {
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(aActor);
 
   return IPC_OK();
 }
 
 bool
--- a/dom/storage/StorageIPC.h
+++ b/dom/storage/StorageIPC.h
@@ -138,17 +138,17 @@ private:
 // such as cookie cleaning etc to the child process.
 class StorageDBParent final : public PBackgroundStorageParent
 {
   class ObserverSink;
 
   virtual ~StorageDBParent();
 
 public:
-  StorageDBParent();
+  explicit StorageDBParent(const nsString& aProfilePath);
 
   void
   Init();
 
   NS_IMETHOD_(MozExternalRefCountType) AddRef(void);
   NS_IMETHOD_(MozExternalRefCountType) Release(void);
 
   void AddIPDLReference();
@@ -266,28 +266,36 @@ private:
                const nsCString& aOriginScope);
 
 private:
   CacheParentBridge* NewCache(const nsACString& aOriginSuffix,
                               const nsACString& aOriginNoSuffix);
 
   RefPtr<ObserverSink> mObserverSink;
 
+  // A hack to deal with deadlock between the parent process main thread and
+  // background thread when invoking StorageDBThread::GetOrCreate because it
+  // cannot safely perform a synchronous dispatch back to the main thread
+  // (because we are already synchronously doing things on the stack).
+  // Populated for the same process actors, empty for other process actors.
+  nsString mProfilePath;
+
   ThreadSafeAutoRefCnt mRefCnt;
   NS_DECL_OWNINGTHREAD
 
   // True when IPC channel is open and Send*() methods are OK to use.
   bool mIPCOpen;
 };
 
 PBackgroundStorageParent*
-AllocPBackgroundStorageParent();
+AllocPBackgroundStorageParent(const nsString& aProfilePath);
 
 mozilla::ipc::IPCResult
-RecvPBackgroundStorageConstructor(PBackgroundStorageParent* aActor);
+RecvPBackgroundStorageConstructor(PBackgroundStorageParent* aActor,
+                                  const nsString& aProfilePath);
 
 bool
 DeallocPBackgroundStorageParent(PBackgroundStorageParent* aActor);
 
 } // namespace dom
 } // namespace mozilla
 
 #endif // mozilla_dom_StorageIPC_h
--- a/ipc/glue/BackgroundChildImpl.cpp
+++ b/ipc/glue/BackgroundChildImpl.cpp
@@ -200,17 +200,17 @@ BackgroundChildImpl::DeallocPBackgroundI
 {
   MOZ_ASSERT(aActor);
 
   delete aActor;
   return true;
 }
 
 BackgroundChildImpl::PBackgroundStorageChild*
-BackgroundChildImpl::AllocPBackgroundStorageChild()
+BackgroundChildImpl::AllocPBackgroundStorageChild(const nsString& aProfilePath)
 {
   MOZ_CRASH("PBackgroundStorageChild actors should be manually constructed!");
 }
 
 bool
 BackgroundChildImpl::DeallocPBackgroundStorageChild(
                                                 PBackgroundStorageChild* aActor)
 {
--- a/ipc/glue/BackgroundChildImpl.h
+++ b/ipc/glue/BackgroundChildImpl.h
@@ -66,17 +66,17 @@ protected:
   virtual PBackgroundIndexedDBUtilsChild*
   AllocPBackgroundIndexedDBUtilsChild() override;
 
   virtual bool
   DeallocPBackgroundIndexedDBUtilsChild(PBackgroundIndexedDBUtilsChild* aActor)
                                         override;
 
   virtual PBackgroundStorageChild*
-  AllocPBackgroundStorageChild() override;
+  AllocPBackgroundStorageChild(const nsString& aProfilePath) override;
 
   virtual bool
   DeallocPBackgroundStorageChild(PBackgroundStorageChild* aActor) override;
 
   virtual PPendingIPCBlobChild*
   AllocPPendingIPCBlobChild(const IPCBlob& aBlob) override;
 
   virtual bool
--- a/ipc/glue/BackgroundParentImpl.cpp
+++ b/ipc/glue/BackgroundParentImpl.cpp
@@ -239,34 +239,35 @@ BackgroundParentImpl::RecvFlushPendingFi
 
   if (!mozilla::dom::indexedDB::RecvFlushPendingFileDeletions()) {
     return IPC_FAIL_NO_REASON(this);
   }
   return IPC_OK();
 }
 
 auto
-BackgroundParentImpl::AllocPBackgroundStorageParent()
+BackgroundParentImpl::AllocPBackgroundStorageParent(const nsString& aProfilePath)
   -> PBackgroundStorageParent*
 {
   AssertIsInMainProcess();
   AssertIsOnBackgroundThread();
 
-  return mozilla::dom::AllocPBackgroundStorageParent();
+  return mozilla::dom::AllocPBackgroundStorageParent(aProfilePath);
 }
 
 mozilla::ipc::IPCResult
 BackgroundParentImpl::RecvPBackgroundStorageConstructor(
-                                               PBackgroundStorageParent* aActor)
+                                               PBackgroundStorageParent* aActor,
+                                               const nsString& aProfilePath)
 {
   AssertIsInMainProcess();
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(aActor);
 
-  return mozilla::dom::RecvPBackgroundStorageConstructor(aActor);
+  return mozilla::dom::RecvPBackgroundStorageConstructor(aActor, aProfilePath);
 }
 
 bool
 BackgroundParentImpl::DeallocPBackgroundStorageParent(
                                                PBackgroundStorageParent* aActor)
 {
   AssertIsInMainProcess();
   AssertIsOnBackgroundThread();
--- a/ipc/glue/BackgroundParentImpl.h
+++ b/ipc/glue/BackgroundParentImpl.h
@@ -59,20 +59,21 @@ protected:
   DeallocPBackgroundIndexedDBUtilsParent(
                                         PBackgroundIndexedDBUtilsParent* aActor)
                                         override;
 
   virtual mozilla::ipc::IPCResult
   RecvFlushPendingFileDeletions() override;
 
   virtual PBackgroundStorageParent*
-  AllocPBackgroundStorageParent() override;
+  AllocPBackgroundStorageParent(const nsString& aProfilePath) override;
 
   virtual mozilla::ipc::IPCResult
-  RecvPBackgroundStorageConstructor(PBackgroundStorageParent* aActor) override;
+  RecvPBackgroundStorageConstructor(PBackgroundStorageParent* aActor,
+                                    const nsString& aProfilePath) override;
 
   virtual bool
   DeallocPBackgroundStorageParent(PBackgroundStorageParent* aActor) override;
 
   virtual PPendingIPCBlobParent*
   AllocPPendingIPCBlobParent(const IPCBlob& aBlob) override;
 
   virtual bool
--- a/ipc/glue/PBackground.ipdl
+++ b/ipc/glue/PBackground.ipdl
@@ -83,17 +83,17 @@ parent:
 
   async PBackgroundIDBFactory(LoggingInfo loggingInfo);
 
   async PBackgroundIndexedDBUtils();
 
   // Use only for testing!
   async FlushPendingFileDeletions();
 
-  async PBackgroundStorage();
+  async PBackgroundStorage(nsString profilePath);
 
   async PVsync();
 
   async PCameras();
 
   async PUDPSocket(OptionalPrincipalInfo pInfo, nsCString filter);
   async PBroadcastChannel(PrincipalInfo pInfo, nsCString origin, nsString channel);