Bug 1487287 - Move child process launch off the I/O thread. r=mccr8
☠☠ backed out by b0e69b136883 ☠ ☠
authorJed Davis <jld@mozilla.com>
Thu, 22 Nov 2018 00:06:22 +0000
changeset 504606 8b1f88d7bfeb1949060170ba51b6530db5e93c8e
parent 504605 8fa5e81ad8015f9bd5ba2b7497f840e4cc50b8bd
child 504607 40310bdbb6708f03fd171eb194abb09d7e02f72b
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs1487287
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1487287 - Move child process launch off the I/O thread. r=mccr8 Launching processes takes enough time that we should avoid blocking the parent process's IPC I/O thread for it; it's less bad for responsiveness than blocking the main thread, but it's not good. On Windows we need to use a dedicated thread, because the sandbox isn't thread-safe and it asserts that the same thread is used for every launch. Otherwise, a thread pool is used. Depends on D8945 Differential Revision: https://phabricator.services.mozilla.com/D8946
ipc/glue/GeckoChildProcessHost.cpp
ipc/glue/GeckoChildProcessHost.h
--- a/ipc/glue/GeckoChildProcessHost.cpp
+++ b/ipc/glue/GeckoChildProcessHost.cpp
@@ -28,21 +28,24 @@
 #include "nsAppDirectoryServiceDefs.h"
 #endif
 
 #include "nsExceptionHandler.h"
 
 #include "nsDirectoryServiceDefs.h"
 #include "nsIFile.h"
 #include "nsPrintfCString.h"
+#include "nsIObserverService.h"
 
-#include "mozilla/ClearOnShutdown.h"
 #include "mozilla/ipc/BrowserProcessSubThread.h"
 #include "mozilla/ipc/EnvironmentMap.h"
 #include "mozilla/Omnijar.h"
+#include "mozilla/Services.h"
+#include "mozilla/SharedThreadPool.h"
+#include "mozilla/StaticMutex.h"
 #include "mozilla/Telemetry.h"
 #include "ProtocolUtils.h"
 #include <sys/stat.h>
 
 #ifdef XP_WIN
 #include "nsIWinTaskbar.h"
 #include <stdlib.h>
 #define NS_TASKBAR_CONTRACTID "@mozilla.org/windows-taskbar;1"
@@ -61,16 +64,17 @@
 #include "nsTArray.h"
 #include "nsClassHashtable.h"
 #include "nsHashKeys.h"
 #include "nsNativeCharsetUtils.h"
 #include "nscore.h" // for NS_FREE_PERMANENT_DATA
 #include "private/pprio.h"
 
 using mozilla::MonitorAutoLock;
+using mozilla::StaticMutexAutoLock;
 using mozilla::ipc::GeckoChildProcessHost;
 
 #ifdef MOZ_WIDGET_ANDROID
 #include "AndroidBridge.h"
 #include "GeneratedJNIWrappers.h"
 #include "mozilla/jni/Refs.h"
 #include "mozilla/jni/Utils.h"
 #endif
@@ -481,35 +485,126 @@ GeckoChildProcessHost::GetChildLogName(c
     buffer.Append(origLogName);
   }
 
   // Append child-specific postfix to name
   buffer.AppendLiteral(".child-");
   buffer.AppendInt(mChildCounter);
 }
 
+namespace {
+// Windows needs a single dedicated thread for process launching,
+// because of thread-safety restrictions/assertions in the sandbox
+// code.  (This implementation isn't itself Windows-specific, so
+// the ifdef can be changed to test on other platforms.)
+#ifdef XP_WIN
+
+static mozilla::StaticMutex gIPCLaunchThreadMutex;
+static mozilla::StaticRefPtr<nsIThread> gIPCLaunchThread;
+
+class IPCLaunchThreadObserver final : public nsIObserver
+{
+public:
+  NS_DECL_ISUPPORTS
+  NS_DECL_NSIOBSERVER
+protected:
+  virtual ~IPCLaunchThreadObserver() = default;
+};
+
+NS_IMPL_ISUPPORTS(IPCLaunchThreadObserver, nsIObserver, nsISupports)
+
+NS_IMETHODIMP
+IPCLaunchThreadObserver::Observe(nsISupports* aSubject,
+                                 const char* aTopic,
+                                 const char16_t* aData)
+{
+  MOZ_RELEASE_ASSERT(strcmp(aTopic, "xpcom-shutdown-threads") == 0);
+  StaticMutexAutoLock lock(gIPCLaunchThreadMutex);
+
+  nsresult rv = NS_OK;
+  if (gIPCLaunchThread) {
+    rv = gIPCLaunchThread->Shutdown();
+    gIPCLaunchThread = nullptr;
+  }
+  mozilla::Unused << NS_WARN_IF(NS_FAILED(rv));
+  return rv;
+}
+
+static nsCOMPtr<nsIEventTarget>
+GetIPCLauncher()
+{
+  StaticMutexAutoLock lock(gIPCLaunchThreadMutex);
+  if (!gIPCLaunchThread) {
+    nsCOMPtr<nsIThread> thread;
+    nsresult rv =
+      NS_NewNamedThread(NS_LITERAL_CSTRING("IPC Launch"),
+                        getter_AddRefs(thread));
+    if (!NS_WARN_IF(NS_FAILED(rv))) {
+      NS_DispatchToMainThread(NS_NewRunnableFunction(
+        "GeckoChildProcessHost::GetIPCLauncher",
+        [] {
+          nsCOMPtr<nsIObserverService> obsService =
+            mozilla::services::GetObserverService();
+          nsCOMPtr<nsIObserver> obs = new IPCLaunchThreadObserver();
+          obsService->AddObserver(obs, "xpcom-shutdown-threads", false);
+        }));
+      gIPCLaunchThread = thread.forget();
+    }
+  }
+
+  nsCOMPtr<nsIEventTarget> thread = gIPCLaunchThread.get();
+  return thread;
+}
+
+#else // XP_WIN
+
+// Non-Windows platforms can use an on-demand thread pool.
+
+static nsCOMPtr<nsIEventTarget>
+GetIPCLauncher()
+{
+  nsCOMPtr<nsIEventTarget> pool =
+    mozilla::SharedThreadPool::Get(NS_LITERAL_CSTRING("IPC Launch"));
+  return pool;
+}
+
+#endif // XP_WIN
+} // anonymous namespace
+
 bool
 GeckoChildProcessHost::RunPerformAsyncLaunch(std::vector<std::string> aExtraOpts)
 {
+  // This (probably?) needs to happen on the I/O thread.
   InitializeChannel();
 
-  bool ok = PerformAsyncLaunch(aExtraOpts);
-  if (!ok) {
-    // WaitUntilConnected might be waiting for us to signal.
-    // If something failed let's set the error state and notify.
-    MonitorAutoLock lock(mMonitor);
-    mProcessState = PROCESS_ERROR;
-    mHandlePromise->Reject(LaunchError{}, __func__);
-    lock.Notify();
-    CHROMIUM_LOG(ERROR) << "Failed to launch " <<
-      XRE_ChildProcessTypeToString(mProcessType) << " subprocess";
-    Telemetry::Accumulate(Telemetry::SUBPROCESS_LAUNCH_FAILURE,
-      nsDependentCString(XRE_ChildProcessTypeToString(mProcessType)));
+  auto launcher = GetIPCLauncher();
+  if (NS_WARN_IF(!launcher)) {
+    return false;
   }
-  return ok;
+
+  // But the rest of this doesn't, and shouldn't block IPC messages:
+  nsresult rv = launcher->Dispatch(NS_NewRunnableFunction(
+    "ipc::GeckoChildProcessHost::PerformAsyncLaunch",
+    [this, aExtraOpts = std::move(aExtraOpts)]() {
+      bool ok = PerformAsyncLaunch(aExtraOpts);
+
+      if (!ok) {
+        // WaitUntilConnected might be waiting for us to signal.
+        // If something failed let's set the error state and notify.
+        MonitorAutoLock lock(mMonitor);
+        mProcessState = PROCESS_ERROR;
+        mHandlePromise->Reject(LaunchError{}, __func__);
+        lock.Notify();
+        CHROMIUM_LOG(ERROR) << "Failed to launch " <<
+          XRE_ChildProcessTypeToString(mProcessType) << " subprocess";
+        Telemetry::Accumulate(Telemetry::SUBPROCESS_LAUNCH_FAILURE,
+          nsDependentCString(XRE_ChildProcessTypeToString(mProcessType)));
+       }
+    }), NS_DISPATCH_NORMAL);
+  return !NS_WARN_IF(NS_FAILED(rv));
 }
 
 void
 #if defined(XP_WIN)
 AddAppDirToCommandLine(CommandLine& aCmdLine)
 #else
 AddAppDirToCommandLine(std::vector<std::string>& aCmdLine)
 #endif
--- a/ipc/glue/GeckoChildProcessHost.h
+++ b/ipc/glue/GeckoChildProcessHost.h
@@ -183,22 +183,22 @@ protected:
 #endif
   RefPtr<HandlePromise::Private> mHandlePromise;
 
   bool OpenPrivilegedHandle(base::ProcessId aPid);
 
 private:
   DISALLOW_EVIL_CONSTRUCTORS(GeckoChildProcessHost);
 
-  // Does the actual work for AsyncLaunch, on the IO thread.
-  // (TODO, bug 1487287: move this to its own thread(s).)
+  // Does the actual work for AsyncLaunch; run in a thread pool
+  // (or, on Windows, a dedicated thread).
   bool PerformAsyncLaunch(StringVector aExtraOpts);
 
-  // Also called on the I/O thread; creates channel, launches, and
-  // consolidates error handling.
+  // Called on the I/O thread; creates channel, dispatches
+  // PerformAsyncLaunch, and consolidates error handling.
   bool RunPerformAsyncLaunch(StringVector aExtraOpts);
 
   enum class BinaryPathType {
     Self,
     PluginContainer
   };
 
   static BinaryPathType GetPathToBinary(FilePath& exePath, GeckoProcessType processType);