Bug 1286798 - Part 6: Fix a dead lock in the single process case; r=asuth,janv

Expose the nested main event target, so it can be used in the single process case by the parent side to process runnables which need to run on the main thread.
After this change we don't have use hacks like getting profile directory path on the child side and sending it to the parent. The parent side can now do it freely even in the single process case.

This patch was enhanced by asuth to not tunnel the nested main event target through IPC.

dom/localstorage/ActorsParent.cpp

@@ -7,6 +7,7 @@
 #include "ActorsParent.h"
 
 #include "LocalStorageCommon.h"
+#include "LSObject.h"
 #include "mozIStorageConnection.h"
 #include "mozIStorageService.h"
 #include "mozStorageCID.h"
@@ -706,10 +707,7 @@ class PrepareDatastoreOp
     Initial,
 
     // Waiting to open/opening on the main thread. Next step is FinishOpen.
-    OpeningOnMainThread,
+    Opening,
-
-    // Waiting to open/opening on the owning thread. Next step is FinishOpen.
-    OpeningOnOwningThread,
 
     // Checking if a prepare datastore operation is already running for given
     // origin on the PBackground thread. Next step is PreparationPending.
@@ -753,6 +751,7 @@ class PrepareDatastoreOp
     Completed
   };
 
+  nsCOMPtr<nsIEventTarget> mMainEventTarget;
   RefPtr<PrepareDatastoreOp> mDelayedOp;
   RefPtr<DirectoryLock> mDirectoryLock;
   RefPtr<Datastore> mDatastore;
@@ -766,7 +765,8 @@ class PrepareDatastoreOp
   bool mInvalidated;
 
 public:
-  explicit PrepareDatastoreOp(const LSRequestParams& aParams);
+  PrepareDatastoreOp(nsIEventTarget* aMainEventTarget,
+                     const LSRequestParams& aParams);
 
   bool
   OriginIsKnown() const
@@ -808,10 +808,7 @@ private:
   ~PrepareDatastoreOp() override;
 
   nsresult
-  OpenOnMainThread();
+  Open();
-
-  nsresult
-  OpenOnOwningThread();
 
   nsresult
   FinishOpen();
@@ -1066,31 +1063,19 @@ AllocPBackgroundLSRequestParent(PBackgroundParent* aBackgroundActor,
     return nullptr;
   }
 
+  // If we're in the same process as the actor, we need to get the target event
+  // queue from the current RequestHelper.
+  nsCOMPtr<nsIEventTarget> mainEventTarget;
+  if (!BackgroundParent::IsOtherProcessActor(aBackgroundActor)) {
+    mainEventTarget = LSObject::GetSyncLoopEventTarget();
+  }
+
   RefPtr<LSRequestBase> actor;
 
   switch (aParams.type()) {
     case LSRequestParams::TLSRequestPrepareDatastoreParams: {
-      bool isOtherProcess =
-        BackgroundParent::IsOtherProcessActor(aBackgroundActor);
-
-      const LSRequestPrepareDatastoreParams& params =
-        aParams.get_LSRequestPrepareDatastoreParams();
-
-      const PrincipalOrQuotaInfo& info = params.info();
-
-      PrincipalOrQuotaInfo::Type infoType = info.type();
-
-      bool paramsOk =
-        (isOtherProcess && infoType == PrincipalOrQuotaInfo::TPrincipalInfo) ||
-        (!isOtherProcess && infoType == PrincipalOrQuotaInfo::TQuotaInfo);
-
-      if (NS_WARN_IF(!paramsOk)) {
-        ASSERT_UNLESS_FUZZING();
-        return nullptr;
-      }
-
       RefPtr<PrepareDatastoreOp> prepareDatastoreOp =
-        new PrepareDatastoreOp(aParams);
+        new PrepareDatastoreOp(mainEventTarget, aParams);
 
       if (!gPrepareDatastoreOps) {
         gPrepareDatastoreOps = new PrepareDatastoreOpArray();
@@ -1572,8 +1557,10 @@ LSRequestBase::RecvCancel()
  * PrepareDatastoreOp
  ******************************************************************************/
 
-PrepareDatastoreOp::PrepareDatastoreOp(const LSRequestParams& aParams)
+PrepareDatastoreOp::PrepareDatastoreOp(nsIEventTarget* aMainEventTarget,
-  : mParams(aParams.get_LSRequestPrepareDatastoreParams())
+                                       const LSRequestParams& aParams)
+  : mMainEventTarget(aMainEventTarget)
+  , mParams(aParams.get_LSRequestPrepareDatastoreParams())
   , mState(State::Initial)
   , mRequestedDirectoryLock(false)
   , mInvalidated(false)
@@ -1594,35 +1581,27 @@ PrepareDatastoreOp::Dispatch()
 {
   AssertIsOnOwningThread();
 
-  const PrincipalOrQuotaInfo& info = mParams.info();
+  mState = State::Opening;
 
-  if (info.type() == PrincipalOrQuotaInfo::TPrincipalInfo) {
+  if (mMainEventTarget) {
-    mState = State::OpeningOnMainThread;
+    MOZ_ALWAYS_SUCCEEDS(mMainEventTarget->Dispatch(this, NS_DISPATCH_NORMAL));
-    MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(this));
   } else {
-    MOZ_ASSERT(info.type() == PrincipalOrQuotaInfo::TQuotaInfo);
+    MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(this));
-
-    mState = State::OpeningOnOwningThread;
-    MOZ_ALWAYS_SUCCEEDS(NS_DispatchToCurrentThread(this));
   }
 }
 
 nsresult
-PrepareDatastoreOp::OpenOnMainThread()
+PrepareDatastoreOp::Open()
 {
   MOZ_ASSERT(NS_IsMainThread());
-  MOZ_ASSERT(mState == State::OpeningOnMainThread);
+  MOZ_ASSERT(mState == State::Opening);
 
   if (NS_WARN_IF(QuotaClient::IsShuttingDownOnNonBackgroundThread()) ||
       !MayProceedOnNonOwningThread()) {
     return NS_ERROR_FAILURE;
   }
 
-  const PrincipalOrQuotaInfo& info = mParams.info();
+  const PrincipalInfo& principalInfo = mParams.principalInfo();
-
-  MOZ_ASSERT(info.type() == PrincipalOrQuotaInfo::TPrincipalInfo);
-
-  const PrincipalInfo& principalInfo = info.get_PrincipalInfo();
 
   if (principalInfo.type() == PrincipalInfo::TSystemPrincipalInfo) {
     QuotaManager::GetInfoForChrome(&mSuffix, &mGroup, &mOrigin);
@@ -1657,33 +1636,6 @@ PrepareDatastoreOp::OpenOnMainThread()
   return NS_OK;
 }
 
-nsresult
-PrepareDatastoreOp::OpenOnOwningThread()
-{
-  AssertIsOnOwningThread();
-  MOZ_ASSERT(mState == State::OpeningOnOwningThread);
-
-  if (NS_WARN_IF(QuotaClient::IsShuttingDownOnBackgroundThread()) ||
-      !MayProceed()) {
-    return NS_ERROR_FAILURE;
-  }
-
-  const PrincipalOrQuotaInfo& info = mParams.info();
-
-  MOZ_ASSERT(info.type() == PrincipalOrQuotaInfo::TQuotaInfo);
-
-  const QuotaInfo& quotaInfo = info.get_QuotaInfo();
-
-  mSuffix = quotaInfo.suffix();
-  mGroup = quotaInfo.group();
-  mOrigin = quotaInfo.origin();
-
-  mState = State::FinishOpen;
-  MOZ_ALWAYS_SUCCEEDS(OwningEventTarget()->Dispatch(this, NS_DISPATCH_NORMAL));
-
-  return NS_OK;
-}
-
 nsresult
 PrepareDatastoreOp::FinishOpen()
 {
@@ -1702,9 +1654,7 @@ PrepareDatastoreOp::FinishOpen()
   // However, the methods OriginIsKnown and Origin can be called at any time.
   // So we have to make sure the member variable is set on the same thread as
   // those methods are called.
-  if (mParams.info().type() == PrincipalOrQuotaInfo::TPrincipalInfo) {
+  mOrigin = mMainThreadOrigin;
-    mOrigin = mMainThreadOrigin;
-  }
 
   MOZ_ASSERT(!mOrigin.IsEmpty());
 
@@ -1760,7 +1710,7 @@ PrepareDatastoreOp::BeginDatastorePreparation()
   }
 
   mState = State::QuotaManagerPending;
-  QuotaManager::GetOrCreate(this);
+  QuotaManager::GetOrCreate(this, mMainEventTarget);
 
   return NS_OK;
 }
@@ -2077,12 +2027,8 @@ PrepareDatastoreOp::Run()
   nsresult rv;
 
   switch (mState) {
-    case State::OpeningOnMainThread:
+    case State::Opening:
-      rv = OpenOnMainThread();
+      rv = Open();
-      break;
-
-    case State::OpeningOnOwningThread:
-      rv = OpenOnOwningThread();
       break;
 
     case State::FinishOpen:

dom/localstorage/LSObject.cpp

@@ -83,6 +83,17 @@ public:
     MOZ_ASSERT(IsOnOwningThread());
   }
 
+  // Used for requests from the parent process to the parent process; in that
+  // case we want ActorsParent to know our event-target and this is better than
+  // trying to tunnel the pointer through IPC.
+  const nsCOMPtr<nsIEventTarget>&
+  GetSyncLoopEventTarget() const
+  {
+    MOZ_ASSERT(XRE_IsParentProcess());
+
+    return mNestedEventTarget;
+  }
+
   nsresult
   StartAndReturnResponse(LSRequestResponse& aResponse);
 
@@ -146,52 +157,40 @@ LSObject::Create(nsPIDOMWindowInner* aWindow,
     return NS_ERROR_FAILURE;
   }
 
-  nsresult rv;
+  nsAutoPtr<PrincipalInfo> principalInfo(new PrincipalInfo());
-
+  nsresult rv = PrincipalToPrincipalInfo(principal, principalInfo);
-  nsAutoPtr<PrincipalOrQuotaInfo> info(new PrincipalOrQuotaInfo());
+  if (NS_WARN_IF(NS_FAILED(rv))) {
-  if (XRE_IsParentProcess()) {
+    return rv;
-    nsCString suffix;
-    nsCString group;
-    nsCString origin;
-    rv = QuotaManager::GetInfoFromPrincipal(principal,
-                                            &suffix,
-                                            &group,
-                                            &origin);
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      return rv;
-    }
-
-    QuotaInfo quotaInfo;
-    quotaInfo.suffix() = suffix;
-    quotaInfo.group() = group;
-    quotaInfo.origin() = origin;
-
-    *info = quotaInfo;
-
-    // This service has to be started on the main thread currently.
-    nsCOMPtr<mozIStorageService> ss;
-    if (NS_WARN_IF(!(ss = do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID)))) {
-      return NS_ERROR_FAILURE;
-    }
-  } else {
-    PrincipalInfo principalInfo;
-    rv = PrincipalToPrincipalInfo(principal, &principalInfo);
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      return rv;
-    }
-
-    MOZ_ASSERT(principalInfo.type() == PrincipalInfo::TContentPrincipalInfo);
-
-    *info = principalInfo;
   }
 
+  MOZ_ASSERT(principalInfo->type() == PrincipalInfo::TContentPrincipalInfo);
+
   RefPtr<LSObject> object = new LSObject(aWindow, principal);
-  object->mInfo = std::move(info);
+  object->mPrincipalInfo = std::move(principalInfo);
 
   object.forget(aStorage);
   return NS_OK;
 }
 
+// static
+already_AddRefed<nsIEventTarget>
+LSObject::GetSyncLoopEventTarget()
+{
+  RefPtr<RequestHelper> helper;
+
+  {
+    StaticMutexAutoLock lock(gRequestHelperMutex);
+    helper = gRequestHelper;
+  }
+
+  nsCOMPtr<nsIEventTarget> target;
+  if (helper) {
+    target = helper->GetSyncLoopEventTarget();
+  }
+
+  return target.forget();
+}
+
 // static
 void
 LSObject::CancelSyncLoop()
@@ -436,7 +435,7 @@ LSObject::EnsureDatabase()
   }
 
   LSRequestPrepareDatastoreParams params;
-  params.info() = *mInfo;
+  params.principalInfo() = *mPrincipalInfo;
 
   RefPtr<RequestHelper> helper = new RequestHelper(this, params);
 

dom/localstorage/LSObject.h

@@ -16,18 +16,25 @@ namespace mozilla {
 
 class ErrorResult;
 
+namespace ipc {
+
+class PrincipalInfo;
+
+} // namespace ipc
+
 namespace dom {
 
 class LSDatabase;
 class LSRequestChild;
 class LSRequestChildCallback;
 class LSRequestParams;
-class PrincipalOrQuotaInfo;
 
 class LSObject final
   : public Storage
 {
-  nsAutoPtr<PrincipalOrQuotaInfo> mInfo;
+  typedef mozilla::ipc::PrincipalInfo PrincipalInfo;
+
+  nsAutoPtr<PrincipalInfo> mPrincipalInfo;
 
   RefPtr<LSDatabase> mDatabase;
 
@@ -36,6 +43,14 @@ public:
   Create(nsPIDOMWindowInner* aWindow,
          Storage** aStorage);
 
+  /**
+   * Used for requests from the parent process to the parent process; in that
+   * case we want ActorsParent to know our event-target and this is better than
+   * trying to tunnel the pointer through IPC.
+   */
+  static already_AddRefed<nsIEventTarget>
+  GetSyncLoopEventTarget();
+
   static void
   CancelSyncLoop();
 

dom/localstorage/PBackgroundLSSharedTypes.ipdlh

@@ -7,20 +7,9 @@ include PBackgroundSharedTypes;
 namespace mozilla {
 namespace dom {
 
-struct QuotaInfo {
-  nsCString suffix;
-  nsCString group;
-  nsCString origin;
-};
-
-union PrincipalOrQuotaInfo {
-  PrincipalInfo;
-  QuotaInfo;
-};
-
 struct LSRequestPrepareDatastoreParams
 {
-  PrincipalOrQuotaInfo info;
+  PrincipalInfo principalInfo;
 };
 
 union LSRequestParams

dom/quota/ActorsParent.cpp

@@ -405,6 +405,7 @@ class QuotaManager::CreateRunnable final
   : public BackgroundThreadObject
   , public Runnable
 {
+  nsCOMPtr<nsIEventTarget> mMainEventTarget;
   nsTArray<nsCOMPtr<nsIRunnable>> mCallbacks;
   nsString mBaseDirPath;
   RefPtr<QuotaManager> mManager;
@@ -422,8 +423,9 @@ class QuotaManager::CreateRunnable final
   State mState;
 
 public:
-  CreateRunnable()
+  explicit CreateRunnable(nsIEventTarget* aMainEventTarget)
     : Runnable("dom::quota::QuotaManager::CreateRunnable")
+    , mMainEventTarget(aMainEventTarget)
     , mResultCode(NS_OK)
     , mState(State::Initial)
   {
@@ -2780,7 +2782,11 @@ CreateRunnable::GetNextState(nsCOMPtr<nsIEventTarget>& aThread) -> State
       aThread = mOwningThread;
       return State::CreatingManager;
     case State::CreatingManager:
-      aThread = GetMainThreadEventTarget();
+      if (mMainEventTarget) {
+        aThread = mMainEventTarget;
+      } else {
+        aThread = GetMainThreadEventTarget();
+      }
       return State::RegisteringObserver;
     case State::RegisteringObserver:
       aThread = mOwningThread;
@@ -3245,7 +3251,8 @@ QuotaManager::~QuotaManager()
 }
 
 void
-QuotaManager::GetOrCreate(nsIRunnable* aCallback)
+QuotaManager::GetOrCreate(nsIRunnable* aCallback,
+                          nsIEventTarget* aMainEventTarget)
 {
   AssertIsOnBackgroundThread();
 
@@ -3261,8 +3268,14 @@ QuotaManager::GetOrCreate(nsIRunnable* aCallback)
     MOZ_ALWAYS_SUCCEEDS(NS_DispatchToCurrentThread(aCallback));
   } else {
     if (!gCreateRunnable) {
-      gCreateRunnable = new CreateRunnable();
+      gCreateRunnable = new CreateRunnable(aMainEventTarget);
-      MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(gCreateRunnable));
+      if (aMainEventTarget) {
+        MOZ_ALWAYS_SUCCEEDS(aMainEventTarget->Dispatch(gCreateRunnable,
+                                                       NS_DISPATCH_NORMAL));
+      } else {
+        MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(gCreateRunnable));
+      }
+
     }
 
     gCreateRunnable->AddCallback(aCallback);

dom/quota/QuotaManager.h

@@ -117,7 +117,8 @@ public:
   static const char kReplaceChars[];
 
   static void
-  GetOrCreate(nsIRunnable* aCallback);
+  GetOrCreate(nsIRunnable* aCallback,
+              nsIEventTarget* aMainEventTarget = nullptr);
 
   // Returns a non-owning reference.
   static QuotaManager*

toolkit/components/extensions/test/xpcshell/xpcshell-common.ini

@@ -60,7 +60,7 @@ skip-if = os == "android" # checking for telemetry needs to be updated: 1384923
 [test_ext_extension_startup_telemetry.js]
 [test_ext_geturl.js]
 [test_ext_idle.js]
-#[test_ext_localStorage.js]
+[test_ext_localStorage.js]
 [test_ext_management.js]
 skip-if = (os == "win" && !debug) #Bug 1419183 disable on Windows
 [test_ext_management_uninstall_self.js]