From 488ccd8244ea01c511d507f2217323c9eacad529 Mon Sep 17 00:00:00 2001 From: Jean-Yves Avenard Date: Wed, 1 Jul 2020 06:46:59 +0000 Subject: [PATCH] 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 --- .../RemoteSandboxBrokerParent.cpp | 6 ++--- .../RemoteSandboxBrokerParent.h | 4 ++- .../remoteSandboxBroker.cpp | 25 +++++++++++-------- .../remotesandboxbroker/remoteSandboxBroker.h | 2 +- 4 files changed, 22 insertions(+), 15 deletions(-) diff --git a/security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.cpp b/security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.cpp index 0fe0ed63a2de..f59421d45473 100644 --- a/security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.cpp +++ b/security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.cpp @@ -12,7 +12,7 @@ namespace mozilla { RefPtr RemoteSandboxBrokerParent::Launch( - const nsTArray& aHandlesToShare) { + const nsTArray& aHandlesToShare, nsISerialEventTarget* aThread) { MOZ_ASSERT(!mProcess); if (mProcess) { // Don't re-init. @@ -45,8 +45,8 @@ RefPtr 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, diff --git a/security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.h b/security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.h index f78d74253636..c0ebd786d245 100644 --- a/security/sandbox/win/src/remotesandboxbroker/RemoteSandboxBrokerParent.h +++ b/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 Launch(const nsTArray& aHandlesToShare); + // aThread is the thread to use to resolve the promise on if needed. + RefPtr Launch(const nsTArray& aHandlesToShare, + nsISerialEventTarget* aThread); private: void ActorDestroy(ActorDestroyReason aWhy) override; diff --git a/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.cpp b/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.cpp index ec741d27bc6e..faf5036dd32e 100644 --- a/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.cpp +++ b/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 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; }); diff --git a/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.h b/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.h index 76d155139386..89456c3a3b7a 100644 --- a/security/sandbox/win/src/remotesandboxbroker/remoteSandboxBroker.h +++ b/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 mIPCLaunchThread; + nsCOMPtr mIPCLaunchThread; // True if we've been shutdown. bool mShutdown = false;