Bug 1648326 - Don't dispatch runnable on the running taskqueue. r=bobowen,jld

The current taskqueue is blocked until the current function has finished; Running the event loop would only process events on the running thread.

Additionally, we make mIPCLaunchThread an nsISerialEventTarget to guarantee that at shutdown the tasks are run in order regardless of the IPC Launch Thread type.

Differential Revision: https://phabricator.services.mozilla.com/D81511

security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.cpp

@@ -12,7 +12,7 @@
 namespace mozilla {
 
 RefPtr<GenericPromise> RemoteSandboxBrokerParent::Launch(
-    const nsTArray<uint64_t>& aHandlesToShare) {
+    const nsTArray<uint64_t>& aHandlesToShare, nsISerialEventTarget* aThread) {
   MOZ_ASSERT(!mProcess);
   if (mProcess) {
     // Don't re-init.
@@ -45,8 +45,8 @@ RefPtr<GenericPromise> RemoteSandboxBrokerParent::Launch(
     return GenericPromise::CreateAndReject(NS_ERROR_FAILURE, __func__);
   };
 
-  return mProcess->AsyncLaunch()->Then(GetCurrentSerialEventTarget(), __func__,
-                                       std::move(resolve), std::move(reject));
+  return mProcess->AsyncLaunch()->Then(aThread, __func__, std::move(resolve),
+                                       std::move(reject));
 }
 
 bool RemoteSandboxBrokerParent::DuplicateFromLauncher(HANDLE aLauncherHandle,

security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.h

@@ -26,7 +26,9 @@ class RemoteSandboxBrokerParent
   // Asynchronously launches the launcher process.
   // Note: we rely on the caller to keep this instance alive
   // until this promise resolves.
-  RefPtr<GenericPromise> Launch(const nsTArray<uint64_t>& aHandlesToShare);
+  // aThread is the thread to use to resolve the promise on if needed.
+  RefPtr<GenericPromise> Launch(const nsTArray<uint64_t>& aHandlesToShare,
+                                nsISerialEventTarget* aThread);
 
  private:
   void ActorDestroy(ActorDestroyReason aWhy) override;

security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.cpp

@@ -39,17 +39,17 @@ bool RemoteSandboxBroker::LaunchApp(
     base::EnvironmentMap& aEnvironment, GeckoProcessType aProcessType,
     const bool aEnableLogging, const IMAGE_THUNK_DATA*, void** aProcessHandle) {
   // Note: we expect to be called on the IPC launch thread from
-  // GeckoChildProcesHost while it's launching a child process. We can't
-  // run a synchronous launch here as that blocks the calling thread while
-  // it dispatches a task to the IPC launch thread to spawn the process.
-  // Since our calling thread is the IPC launch thread, we'd then be
-  // deadlocked. So instead we do an async launch, and spin the event
-  // loop until the process launch succeeds.
+  // GeckoChildProcesHost while it's launching a child process. The IPC launch
+  // thread is a TaskQueue. We can't run a synchronous launch here as that
+  // blocks the calling thread while it dispatches a task to the IPC launch
+  // thread to spawn the process. Since our calling thread is the IPC launch
+  // thread, we'd then be deadlocked. So instead we do an async launch, and spin
+  // the event loop until the process launch succeeds.
 
   // We should be on the IPC launch thread. We're shutdown on the IO thread,
   // so save a ref to the IPC launch thread here, so we can close the channel
   // on the IPC launch thread on shutdown.
-  mIPCLaunchThread = GetCurrentEventTarget();
+  mIPCLaunchThread = GetCurrentSerialEventTarget();
 
   mParameters.path() = nsDependentString(aPath);
   mParameters.args() = nsDependentString(aArguments);
@@ -77,9 +77,14 @@ bool RemoteSandboxBroker::LaunchApp(
     return GenericPromise::CreateAndReject(NS_ERROR_FAILURE, __func__);
   };
 
-  mParent.Launch(mParameters.shareHandles())
-      ->Then(GetCurrentSerialEventTarget(), __func__, std::move(resolve),
-             std::move(reject));
+  // We need to wait on the current thread for the process to launch which will
+  // block the running IPC Launch taskqueue. We cannot use
+  // GetCurrentSerialEventTarget() (as this returns the currently running
+  // TaskQueue) to resolve our promise as it will be blocked until we return from
+  // this function.
+  nsCOMPtr<nsISerialEventTarget> target = NS_GetCurrentThread();
+  mParent.Launch(mParameters.shareHandles(), target)
+      ->Then(target, __func__, std::move(resolve), std::move(reject));
 
   // Spin the event loop while the sandbox launcher process launches.
   SpinEventLoopUntil([&]() { return res != Pending; });

security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.h

@@ -56,7 +56,7 @@ class RemoteSandboxBroker : public AbstractSandboxBroker {
   // We bind the RemoteSandboxBrokerParent to the IPC launch thread.
   // As such, we must close its channel on the same thread. So we save
   // a reference to the IPC launch thread here.
-  nsCOMPtr<nsIEventTarget> mIPCLaunchThread;
+  nsCOMPtr<nsISerialEventTarget> mIPCLaunchThread;
 
   // True if we've been shutdown.
   bool mShutdown = false;