Bug 1487287 - Set profiler env vars in child processes without side-effecting the parent process. r=mstange
☠☠ backed out by b0e69b136883 ☠ ☠
authorJed Davis <jld@mozilla.com>
Thu, 22 Nov 2018 00:06:20 +0000
changeset 504605 8fa5e81ad8015f9bd5ba2b7497f840e4cc50b8bd
parent 504604 7a480161fa0fc82f11d2ed492f9f8938a1cdf1fc
child 504606 8b1f88d7bfeb1949060170ba51b6530db5e93c8e
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)
reviewersmstange
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 - 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
ipc/glue/EnvironmentMap.h
ipc/glue/GeckoChildProcessHost.cpp
tools/profiler/core/platform.cpp
tools/profiler/public/GeckoProfiler.h
--- a/ipc/glue/EnvironmentMap.h
+++ b/ipc/glue/EnvironmentMap.h
@@ -1,8 +1,10 @@
+/* -*- 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.
 
 #ifndef SANDBOXING_COMMON_ENVIRONMENTMAP_H_
 #define SANDBOXING_COMMON_ENVIRONMENTMAP_H_
 
 #include <map>
@@ -13,17 +15,17 @@ namespace base {
 
 #if defined(OS_WIN)
 
 typedef std::wstring NativeEnvironmentString;
 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
 // matched against the first element of the pairs. In the event of a match, the
 // value is replaced by the second of the pair, unless the second is empty, in
 // which case the key-value is removed.
 //
 // This Windows version takes and returns a Windows-style environment block
@@ -35,17 +37,17 @@ NativeEnvironmentString AlterEnvironment
 
 #elif defined(OS_POSIX)
 
 typedef std::string NativeEnvironmentString;
 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.
 //
 // This Posix version takes and returns a Posix-style environment block, which
 // is a null-terminated list of pointers to null-terminated strings. The
 // returned array will have appended to it the storage for the array itself so
 // there is only one pointer to manage, but this means that you can't copy the
 // array without keeping the original around.
--- a/ipc/glue/GeckoChildProcessHost.cpp
+++ b/ipc/glue/GeckoChildProcessHost.cpp
@@ -563,57 +563,60 @@ AddAppDirToCommandLine(std::vector<std::
     }
   }
 }
 
 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();
 
   // - Note: this code is not called re-entrantly, nor are restoreOrig*LogName
   //   or mChildCounter touched by any other thread, so this is safe.
   ++mChildCounter;
 
   const char* origNSPRLogName = PR_GetEnv("NSPR_LOG_FILE");
   const char* origMozLogName = PR_GetEnv("MOZ_LOG_FILE");
 
   if (origNSPRLogName) {
     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)
   if (!mTmpDirName.IsEmpty()) {
     // 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
 
   // We rely on the fact that InitializeChannel() has already been processed
   // on the IO thread before this point is reached.
   if (!GetChannel()) {
     return false;
   }
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -31,16 +31,17 @@
 #include <fstream>
 #include <sstream>
 #include <errno.h>
 
 #include "platform.h"
 #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"
 #include "VTuneProfiler.h"
 #include "GeckoProfilerReporter.h"
 #include "ProfilerIOInterposeObserver.h"
 #include "mozilla/AutoProfilerLabel.h"
 #include "mozilla/ExtensionPolicyService.h"
@@ -2966,80 +2967,58 @@ profiler_get_start_params(int* aCapacity
 
   const Vector<std::string>& filters = ActivePS::Filters(lock);
   MOZ_ALWAYS_TRUE(aFilters->resize(filters.length()));
   for (uint32_t i = 0; i < filters.length(); ++i) {
     (*aFilters)[i] = filters[i].c_str();
   }
 }
 
-AutoSetProfilerEnvVarsForChildProcess::AutoSetProfilerEnvVarsForChildProcess(
-  MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_IN_IMPL)
-  : mSetCapacity()
-  , mSetInterval()
-  , mSetFeaturesBitfield()
-  , mSetFilters()
+namespace mozilla {
+
+void
+GetProfilerEnvVarsForChildProcess(
+  std::function<void(const char* key, const char* value)>&& aSetEnv)
 {
-  MOZ_GUARD_OBJECT_NOTIFIER_INIT;
-
   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);
-
-  // Use AppendFloat instead of SprintfLiteral with %f because the decimal
+  aSetEnv("MOZ_PROFILER_STARTUP", "1");
+  auto capacityString = Smprintf("%d", ActivePS::Capacity(lock));
+  aSetEnv("MOZ_PROFILER_STARTUP_ENTRIES", capacityString.get());
+
+  // 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);
-
-  SprintfLiteral(mSetFeaturesBitfield,
-                 "MOZ_PROFILER_STARTUP_FEATURES_BITFIELD=%d",
-                 ActivePS::Features(lock));
-  PR_SetEnv(mSetFeaturesBitfield);
+  nsCString intervalString;
+  intervalString.AppendFloat(ActivePS::Interval(lock));
+  aSetEnv("MOZ_PROFILER_STARTUP_INTERVAL", intervalString.get());
+
+  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);
   for (uint32_t i = 0; i < filters.length(); ++i) {
     filtersString += filters[i];
     if (i != filters.length() - 1) {
       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,
                                      bool aIsShuttingDown = false)
 {
   LOG("locked_profiler_save_profile_to_file(%s)", aFilename);
 
   MOZ_RELEASE_ASSERT(CorePS::Exists() && ActivePS::Exists(aLock));
--- a/tools/profiler/public/GeckoProfiler.h
+++ b/tools/profiler/public/GeckoProfiler.h
@@ -924,30 +924,21 @@ public:
 protected:
   MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
   const char* mCategory;
   const char* mMarkerName;
   const mozilla::Maybe<nsID> mDocShellId;
   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
 
 #endif // !MOZ_GECKO_PROFILER
 
 #endif  // GeckoProfiler_h