Bug 1648326 - Don't dispatch runnable on the running taskqueue. r=bobowen,jld a=RyanVM
authorJean-Yves Avenard <jyavenard@mozilla.com>
Wed, 01 Jul 2020 06:46:59 +0000
changeset 601887 9db3d5cb8008c0507e2507da8ff3409fc2e52e91
parent 601886 5882e2762533b6448ea12551b19561ade4d96657
child 601888 5304973b102a9f914cf31cc313a40d179dc25d12
push id13341
push userryanvm@gmail.com
push dateThu, 02 Jul 2020 15:55:10 +0000
treeherdermozilla-beta@a0161364227e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbobowen, jld, RyanVM
bugs1648326
milestone79.0
Bug 1648326 - Don't dispatch runnable on the running taskqueue. r=bobowen,jld a=RyanVM 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
security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.h
security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.cpp
security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.h
--- a/security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.cpp
+++ b/security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.cpp
@@ -7,17 +7,17 @@
 #include "RemoteSandboxBrokerParent.h"
 #include "RemoteSandboxBrokerProcessParent.h"
 #include "mozilla/Telemetry.h"
 #include <windows.h>
 
 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.
     return GenericPromise::CreateAndReject(NS_ERROR_FAILURE, __func__);
   }
 
   mProcess = new RemoteSandboxBrokerProcessParent();
   for (uint64_t handle : aHandlesToShare) {
@@ -40,18 +40,18 @@ RefPtr<GenericPromise> RemoteSandboxBrok
     NS_ERROR("failed to launch child in the parent");
     if (mProcess) {
       mProcess->Destroy();
       mProcess = nullptr;
     }
     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,
                                                       LPHANDLE aOurHandle) {
   return ::DuplicateHandle(mProcess->GetChildProcessHandle(), aLauncherHandle,
                            ::GetCurrentProcess(), aOurHandle, 0, false,
                            DUPLICATE_SAME_ACCESS | DUPLICATE_CLOSE_SOURCE);
 }
--- a/security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.h
+++ b/security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.h
@@ -21,17 +21,19 @@ class RemoteSandboxBrokerParent
  public:
   bool DuplicateFromLauncher(HANDLE aLauncherHandle, LPHANDLE aOurHandle);
 
   void Shutdown();
 
   // 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;
 
   RemoteSandboxBrokerProcessParent* mProcess = nullptr;
 
   bool mOpened = false;
 };
--- a/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.cpp
+++ b/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.cpp
@@ -34,27 +34,27 @@ void RemoteSandboxBroker::Shutdown() {
       }));
 }
 
 bool RemoteSandboxBroker::LaunchApp(
     const wchar_t* aPath, const wchar_t* aArguments,
     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);
 
   auto toNsString = [](const std::wstring& s) {
     return nsDependentString(s.c_str());
   };
   for (auto itr : aEnvironment) {
@@ -72,19 +72,24 @@ bool RemoteSandboxBroker::LaunchApp(
     return GenericPromise::CreateAndResolve(ok, __func__);
   };
 
   auto reject = [&](nsresult) {
     res = Failed;
     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; });
 
   if (res == Failed) {
     return false;
   }
 
--- a/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.h
+++ b/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.h
@@ -51,17 +51,17 @@ class RemoteSandboxBroker : public Abstr
   // Parameters that we use to launch the child process.
   LaunchParameters mParameters;
 
   RemoteSandboxBrokerParent mParent;
 
   // 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;
 };
 
 }  // namespace mozilla
 
 #endif  // __REMOTE_SANDBOXBROKER_H__