Bug 1487287 - Set profiler env vars in child processes without side-effecting the parent process. r=mstange

We can directly set environment variables for the child process on
all platforms now, instead of changing the parent's environment and
inheriting the changes.  This simplifies memory management, but more
importantly it's necessary for thread safety to allow launching
processes from a thread pool.

Depends on D8944

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

--HG--
extra : moz-landing-system : lando

ipc/glue/EnvironmentMap.h

@@ -1,3 +1,5 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
@@ -18,7 +20,7 @@ typedef std::map<NativeEnvironmentString, NativeEnvironmentString>
     EnvironmentMap;
 
 #define ENVIRONMENT_LITERAL(x) L##x
-#define ENVIRONMENT_STRING(x) (std::wstring)(NS_ConvertUTF8toUTF16((x)).get())
+#define ENVIRONMENT_STRING(x) ((std::wstring)(NS_ConvertUTF8toUTF16((x)).get()))
 
 // Returns a modified environment vector constructed from the given environment
 // and the list of changes given in |changes|. Each key in the environment is
@@ -40,7 +42,7 @@ typedef std::map<NativeEnvironmentString, NativeEnvironmentString>
     EnvironmentMap;
 
 #define ENVIRONMENT_LITERAL(x) x
-#define ENVIRONMENT_STRING(x) ((x)).get()
+#define ENVIRONMENT_STRING(x) x
 
 // See general comments for the Windows version above.
 //

ipc/glue/GeckoChildProcessHost.cpp

@@ -548,7 +548,10 @@ AddAppDirToCommandLine(std::vector<std::string>& aCmdLine)
 bool GeckoChildProcessHost::PerformAsyncLaunch(
     std::vector<std::string> aExtraOpts) {
 #ifdef MOZ_GECKO_PROFILER
-  AutoSetProfilerEnvVarsForChildProcess profilerEnvironment;
+  GetProfilerEnvVarsForChildProcess([this](const char* key, const char* value) {
+    mLaunchOptions->env_map[ENVIRONMENT_STRING(key)] =
+        ENVIRONMENT_STRING(value);
+  });
 #endif
 
   const auto startTS = TimeStamp::Now();
@@ -564,20 +567,20 @@ bool GeckoChildProcessHost::PerformAsyncLaunch(
     nsAutoCString nsprLogName;
     GetChildLogName(origNSPRLogName, nsprLogName);
     mLaunchOptions->env_map[ENVIRONMENT_LITERAL("NSPR_LOG_FILE")] =
-        ENVIRONMENT_STRING(nsprLogName);
+        ENVIRONMENT_STRING(nsprLogName.get());
   }
   if (origMozLogName) {
     nsAutoCString mozLogName;
     GetChildLogName(origMozLogName, mozLogName);
     mLaunchOptions->env_map[ENVIRONMENT_LITERAL("MOZ_LOG_FILE")] =
-        ENVIRONMENT_STRING(mozLogName);
+        ENVIRONMENT_STRING(mozLogName.get());
   }
 
   // `RUST_LOG_CHILD` is meant for logging child processes only.
   nsAutoCString childRustLog(PR_GetEnv("RUST_LOG_CHILD"));
   if (!childRustLog.IsEmpty()) {
     mLaunchOptions->env_map[ENVIRONMENT_LITERAL("RUST_LOG")] =
-        ENVIRONMENT_STRING(childRustLog);
+        ENVIRONMENT_STRING(childRustLog.get());
   }
 
 #if defined(XP_LINUX) && defined(MOZ_CONTENT_SANDBOX)
@@ -585,10 +588,10 @@ bool GeckoChildProcessHost::PerformAsyncLaunch(
     // Point a bunch of things that might want to write from content to our
     // shiny new content-process specific tmpdir
     mLaunchOptions->env_map[ENVIRONMENT_LITERAL("TMPDIR")] =
-        ENVIRONMENT_STRING(mTmpDirName);
+        ENVIRONMENT_STRING(mTmpDirName.get());
     // Partial fix for bug 1380051 (not persistent - should be)
     mLaunchOptions->env_map[ENVIRONMENT_LITERAL("MESA_GLSL_CACHE_DIR")] =
-        ENVIRONMENT_STRING(mTmpDirName);
+        ENVIRONMENT_STRING(mTmpDirName.get());
   }
 #endif
 

tools/profiler/core/platform.cpp

@@ -36,6 +36,7 @@
 #include "PlatformMacros.h"
 #include "mozilla/ArrayUtils.h"
 #include "mozilla/Atomics.h"
+#include "mozilla/Printf.h"
 #include "mozilla/UniquePtr.h"
 #include "mozilla/Vector.h"
 #include "GeckoProfiler.h"
@@ -2907,40 +2908,33 @@ void profiler_get_start_params(int* aCapacity, Maybe<double>* aDuration,
   }
 }
 
-AutoSetProfilerEnvVarsForChildProcess::AutoSetProfilerEnvVarsForChildProcess(
-    MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_IN_IMPL)
-    : mSetCapacity(), mSetInterval(), mSetFeaturesBitfield(), mSetFilters() {
-  MOZ_GUARD_OBJECT_NOTIFIER_INIT;
+namespace mozilla {
 
+void GetProfilerEnvVarsForChildProcess(
+    std::function<void(const char* key, const char* value)>&& aSetEnv) {
   MOZ_RELEASE_ASSERT(CorePS::Exists());
 
   PSAutoLock lock(gPSMutex);
 
   if (!ActivePS::Exists(lock)) {
-    PR_SetEnv("MOZ_PROFILER_STARTUP=");
+    aSetEnv("MOZ_PROFILER_STARTUP", "");
     return;
   }
 
-  PR_SetEnv("MOZ_PROFILER_STARTUP=1");
-  SprintfLiteral(mSetCapacity, "MOZ_PROFILER_STARTUP_ENTRIES=%d",
-                 ActivePS::Capacity(lock));
-  PR_SetEnv(mSetCapacity);
+  aSetEnv("MOZ_PROFILER_STARTUP", "1");
+  auto capacityString = Smprintf("%d", ActivePS::Capacity(lock));
+  aSetEnv("MOZ_PROFILER_STARTUP_ENTRIES", capacityString.get());
 
-  // Use AppendFloat instead of SprintfLiteral with %f because the decimal
+  // Use AppendFloat instead of Smprintf with %f because the decimal
   // separator used by %f is locale-dependent. But the string we produce needs
   // to be parseable by strtod, which only accepts the period character as a
   // decimal separator. AppendFloat always uses the period character.
-  nsCString setInterval;
-  setInterval.AppendLiteral("MOZ_PROFILER_STARTUP_INTERVAL=");
-  setInterval.AppendFloat(ActivePS::Interval(lock));
-  strncpy(mSetInterval, setInterval.get(), MOZ_ARRAY_LENGTH(mSetInterval));
-  mSetInterval[MOZ_ARRAY_LENGTH(mSetInterval) - 1] = '\0';
-  PR_SetEnv(mSetInterval);
+  nsCString intervalString;
+  intervalString.AppendFloat(ActivePS::Interval(lock));
+  aSetEnv("MOZ_PROFILER_STARTUP_INTERVAL", intervalString.get());
 
-  SprintfLiteral(mSetFeaturesBitfield,
-                 "MOZ_PROFILER_STARTUP_FEATURES_BITFIELD=%d",
-                 ActivePS::Features(lock));
-  PR_SetEnv(mSetFeaturesBitfield);
+  auto featuresString = Smprintf("%d", ActivePS::Features(lock));
+  aSetEnv("MOZ_PROFILER_STARTUP_FEATURES_BITFIELD", featuresString.get());
 
   std::string filtersString;
   const Vector<std::string>& filters = ActivePS::Filters(lock);
@@ -2950,23 +2944,10 @@ AutoSetProfilerEnvVarsForChildProcess::AutoSetProfilerEnvVarsForChildProcess(
       filtersString += ",";
     }
   }
-  SprintfLiteral(mSetFilters, "MOZ_PROFILER_STARTUP_FILTERS=%s",
-                 filtersString.c_str());
-  PR_SetEnv(mSetFilters);
+  aSetEnv("MOZ_PROFILER_STARTUP_FILTERS", filtersString.c_str());
 }
 
-AutoSetProfilerEnvVarsForChildProcess::
-    ~AutoSetProfilerEnvVarsForChildProcess() {
-  // Our current process doesn't look at these variables after startup, so we
-  // can just unset all the variables. This allows us to use literal strings,
-  // which will be valid for the whole life time of the program and can be
-  // passed to PR_SetEnv without problems.
-  PR_SetEnv("MOZ_PROFILER_STARTUP=");
-  PR_SetEnv("MOZ_PROFILER_STARTUP_ENTRIES=");
-  PR_SetEnv("MOZ_PROFILER_STARTUP_INTERVAL=");
-  PR_SetEnv("MOZ_PROFILER_STARTUP_FEATURES_BITFIELD=");
-  PR_SetEnv("MOZ_PROFILER_STARTUP_FILTERS=");
-}
+}  // namespace mozilla
 
 static void locked_profiler_save_profile_to_file(PSLockRef aLock,
                                                  const char* aFilename,

tools/profiler/public/GeckoProfiler.h

@@ -885,22 +885,13 @@ class MOZ_RAII AutoProfilerTracing {
   const mozilla::Maybe<uint32_t> mDocShellHistoryId;
 };
 
-// Set MOZ_PROFILER_STARTUP* environment variables that will be inherited into
-// a child process that is about to be launched, in order to make that child
-// process start with the same profiler settings as in the current process.
-class MOZ_RAII AutoSetProfilerEnvVarsForChildProcess {
- public:
-  explicit AutoSetProfilerEnvVarsForChildProcess(
-      MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM);
-  ~AutoSetProfilerEnvVarsForChildProcess();
-
- private:
-  MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
-  char mSetCapacity[64];
-  char mSetInterval[64];
-  char mSetFeaturesBitfield[64];
-  char mSetFilters[1024];
-};
+// Get the MOZ_PROFILER_STARTUP* environment variables that should be
+// supplied to a child process that is about to be launched, in order
+// to make that child process start with the same profiler settings as
+// in the current process.  The given function is invoked once for
+// each variable to be set.
+void GetProfilerEnvVarsForChildProcess(
+    std::function<void(const char* key, const char* value)>&& aSetEnv);
 
 }  // namespace mozilla