Bug 1133073 - Use PR_DuplicateEnvironment to avoid post-fork malloc on all Linux platforms. r=dhylands
authorJed Davis <jld@mozilla.com>
Mon, 11 Jan 2016 14:17:01 -0800
changeset 279723 f75b461c2c3c420f51597415721e71f2adef19eb
parent 279722 214b31421207ed37a113659dad2fb66a7a31ebea
child 279724 0d4f53b5744d74accdb9a858c489d4292483a70d
push id16994
push usercbook@mozilla.com
push dateWed, 13 Jan 2016 10:59:30 +0000
treeherderfx-team@cae1c805bf91 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdhylands
bugs1133073
milestone46.0a1
Bug 1133073 - Use PR_DuplicateEnvironment to avoid post-fork malloc on all Linux platforms. r=dhylands
CLOBBER
config/external/nss/Makefile.in
configure.in
ipc/chromium/src/base/process_util_linux.cc
mozglue/build/BionicGlue.cpp
--- a/CLOBBER
+++ b/CLOBBER
@@ -17,9 +17,9 @@
 #
 # Modifying this file will now automatically clobber the buildbot machines \o/
 #
 
 # Are you updating CLOBBER because you think it's needed for your WebIDL
 # changes to stick? As of bug 928195, this shouldn't be necessary! Please
 # don't change CLOBBER for WebIDL changes any more.
 
-Bug 1209344 - Remove debug button from about:addons. r=mossop
+Bug 1133073 - Use PR_DuplicateEnvironment from NSPR and remove interim mozglue wrappers.
--- a/config/external/nss/Makefile.in
+++ b/config/external/nss/Makefile.in
@@ -207,21 +207,24 @@ DEFAULT_GMAKE_FLAGS += \
 DEFAULT_GMAKE_FLAGS += ARCHFLAG='$(filter-out -W%,$(CFLAGS)) -DCHECK_FORK_GETPID $(addprefix -DANDROID_VERSION=,$(ANDROID_VERSION)) -include $(topsrcdir)/security/manager/android_stub.h'
 endif
 endif
 
 ifdef WRAP_LDFLAGS
 NSS_EXTRA_LDFLAGS += $(WRAP_LDFLAGS)
 endif
 
-ifdef MOZ_GLUE_WRAP_LDFLAGS
+# The SHARED_LIBS part is needed unconditionally on Android.  It's not
+# clear why this is the case, but see bug 1133073 (starting around
+# comment #8) for context.
+ifneq (,$(or $(MOZ_GLUE_WRAP_LDFLAGS), $(filter Android, $(OS_TARGET))))
 NSS_EXTRA_LDFLAGS += $(SHARED_LIBS:$(DEPTH)%=$(MOZ_BUILD_ROOT)%) $(MOZ_GLUE_WRAP_LDFLAGS)
 endif
 
-ifneq (,$(WRAP_LDFLAGS)$(MOZ_GLUE_WRAP_LDFLAGS))
+ifneq (,$(NSS_EXTRA_LDFLAGS))
 DEFAULT_GMAKE_FLAGS += \
 	LDFLAGS='$(LDFLAGS) $(NSS_EXTRA_LDFLAGS)' \
 	DSO_LDOPTS='$(DSO_LDOPTS) $(LDFLAGS) $(NSS_EXTRA_LDFLAGS)' \
 	$(NULL)
 endif
 
 DEFAULT_GMAKE_FLAGS += FREEBL_NO_DEPEND=0
 ifeq ($(OS_TARGET),Linux)
--- a/configure.in
+++ b/configure.in
@@ -7266,17 +7266,16 @@ if test -f "${srcdir}/${MOZ_BUILD_APP}/c
      "${srcdir}/${MOZ_BUILD_APP}/configure.in" > $tmpscript
   . $tmpscript
   rm -f $tmpscript
 fi
 
 dnl We need to wrap dlopen and related functions on Android because we use
 dnl our own linker.
 if test "$OS_TARGET" = Android; then
-    MOZ_GLUE_WRAP_LDFLAGS="${MOZ_GLUE_WRAP_LDFLAGS} -Wl,--wrap=PR_GetEnv,--wrap=PR_SetEnv"
     if test "$MOZ_WIDGET_TOOLKIT" = gonk -a -n "$MOZ_NUWA_PROCESS"; then
         MOZ_GLUE_WRAP_LDFLAGS="${MOZ_GLUE_WRAP_LDFLAGS} -Wl,--wrap=pthread_create,--wrap=epoll_wait,--wrap=poll,--wrap=pthread_cond_timedwait,--wrap=pthread_cond_wait,--wrap=epoll_create,--wrap=epoll_ctl,--wrap=close,--wrap=pthread_key_create,--wrap=pthread_key_delete,--wrap=socketpair,--wrap=pthread_self,--wrap=pthread_mutex_lock,--wrap=pthread_mutex_trylock,--wrap=pthread_join,--wrap=pipe,--wrap=pipe2"
     fi
     if test "$MOZ_WIDGET_TOOLKIT" = android -a "$MOZ_ANDROID_MIN_SDK_VERSION" -lt 11; then
         MOZ_GLUE_WRAP_LDFLAGS="${MOZ_GLUE_WRAP_LDFLAGS} -Wl,--wrap=getaddrinfo,--wrap=freeaddrinfo,--wrap=gai_strerror"
     fi
 fi
 
--- a/ipc/chromium/src/base/process_util_linux.cc
+++ b/ipc/chromium/src/base/process_util_linux.cc
@@ -17,16 +17,19 @@
 #include "base/eintr_wrapper.h"
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/string_tokenizer.h"
 #include "base/string_util.h"
 #include "nsLiteralString.h"
 #include "mozilla/UniquePtr.h"
 
+#include "prenv.h"
+#include "prmem.h"
+
 #ifdef MOZ_B2G_LOADER
 #include "ProcessUtils.h"
 
 using namespace mozilla::ipc;
 #endif	// MOZ_B2G_LOADER
 
 #ifdef MOZ_WIDGET_GONK
 /*
@@ -42,99 +45,61 @@ using namespace mozilla::ipc;
  * On platforms that are not gonk based, we fall back to an arbitrary
  * UID. This is generally the UID for user `nobody', albeit it is not
  * always the case.
  */
 # define CHILD_UNPRIVILEGED_UID 65534
 # define CHILD_UNPRIVILEGED_GID 65534
 #endif
 
-#ifdef ANDROID
-#include <pthread.h>
-/*
- * Currently, PR_DuplicateEnvironment is implemented in
- * mozglue/build/BionicGlue.cpp
- */
-#define HAVE_PR_DUPLICATE_ENVIRONMENT
-
-#include "plstr.h"
-#include "prenv.h"
-#include "prmem.h"
-/* Temporary until we have PR_DuplicateEnvironment in prenv.h */
-extern "C" { NSPR_API(pthread_mutex_t *)PR_GetEnvLock(void); }
-#endif
-
 namespace {
 
 enum ParsingState {
   KEY_NAME,
   KEY_VALUE
 };
 
 static mozilla::EnvironmentLog gProcessLog("MOZ_PROCESS_LOG");
 
 }  // namespace
 
 namespace base {
 
-#ifdef HAVE_PR_DUPLICATE_ENVIRONMENT
-/* 
- * I tried to put PR_DuplicateEnvironment down in mozglue, but on android 
- * this winds up failing because the strdup/free calls wind up mismatching. 
- */
-
-static char **
-PR_DuplicateEnvironment(void)
-{
-    char **result = NULL;
-    char **s;
-    char **d;
-    pthread_mutex_lock(PR_GetEnvLock());
-    for (s = environ; *s != NULL; s++)
-      ;
-    if ((result = (char **)PR_Malloc(sizeof(char *) * (s - environ + 1))) != NULL) {
-      for (s = environ, d = result; *s != NULL; s++, d++) {
-        *d = PL_strdup(*s);
-      }
-      *d = NULL;
-    }
-    pthread_mutex_unlock(PR_GetEnvLock());
-    return result;
-}
-
 class EnvironmentEnvp
 {
 public:
   EnvironmentEnvp()
     : mEnvp(PR_DuplicateEnvironment()) {}
 
-  EnvironmentEnvp(const environment_map &em)
+  explicit EnvironmentEnvp(const environment_map &em)
   {
     mEnvp = (char **)PR_Malloc(sizeof(char *) * (em.size() + 1));
     if (!mEnvp) {
       return;
     }
     char **e = mEnvp;
     for (environment_map::const_iterator it = em.begin();
          it != em.end(); ++it, ++e) {
       std::string str = it->first;
       str += "=";
       str += it->second;
-      *e = PL_strdup(str.c_str());
+      size_t len = str.length() + 1;
+      *e = static_cast<char*>(PR_Malloc(len));
+      memcpy(*e, str.c_str(), len);
     }
     *e = NULL;
   }
 
   ~EnvironmentEnvp()
   {
     if (!mEnvp) {
       return;
     }
     for (char **e = mEnvp; *e; ++e) {
-      PL_strfree(*e);
+      PR_Free(*e);
     }
     PR_Free(mEnvp);
   }
 
   char * const *AsEnvp() { return mEnvp; }
 
   void ToMap(environment_map &em)
   {
@@ -173,17 +138,16 @@ public:
   {
     for (const_iterator it = em.begin(); it != em.end(); ++it) {
       (*this)[it->first] = it->second;
     }
   }
 private:
   std::auto_ptr<EnvironmentEnvp> mEnvp;
 };
-#endif // HAVE_PR_DUPLICATE_ENVIRONMENT
 
 bool LaunchApp(const std::vector<std::string>& argv,
                const file_handle_mapping_vector& fds_to_remap,
                bool wait, ProcessHandle* process_handle) {
   return LaunchApp(argv, fds_to_remap, environment_map(),
                    wait, process_handle);
 }
 
@@ -263,25 +227,23 @@ bool LaunchApp(const std::vector<std::st
 #endif // MOZ_B2G_LOADER
 
   mozilla::UniquePtr<char*[]> argv_cstr(new char*[argv.size() + 1]);
   // Illegal to allocate memory after fork and before execvp
   InjectiveMultimap fd_shuffle1, fd_shuffle2;
   fd_shuffle1.reserve(fds_to_remap.size());
   fd_shuffle2.reserve(fds_to_remap.size());
 
-#ifdef HAVE_PR_DUPLICATE_ENVIRONMENT
   Environment env;
   env.Merge(env_vars_to_set);
   char * const *envp = env.AsEnvp();
   if (!envp) {
     DLOG(ERROR) << "FAILED to duplicate environment for: " << argv_cstr[0];
     return false;
   }
-#endif
 
   pid_t pid = fork();
   if (pid < 0)
     return false;
 
   if (pid == 0) {
     for (file_handle_mapping_vector::const_iterator
         it = fds_to_remap.begin(); it != fds_to_remap.end(); ++it) {
@@ -295,27 +257,20 @@ bool LaunchApp(const std::vector<std::st
     CloseSuperfluousFds(fd_shuffle2);
 
     for (size_t i = 0; i < argv.size(); i++)
       argv_cstr[i] = const_cast<char*>(argv[i].c_str());
     argv_cstr[argv.size()] = NULL;
 
     SetCurrentProcessPrivileges(privs);
 
-#ifdef HAVE_PR_DUPLICATE_ENVIRONMENT
     execve(argv_cstr[0], argv_cstr.get(), envp);
-#else
-    for (environment_map::const_iterator it = env_vars_to_set.begin();
-         it != env_vars_to_set.end(); ++it) {
-      if (setenv(it->first.c_str(), it->second.c_str(), 1/*overwrite*/))
-        _exit(127);
-    }
-    execv(argv_cstr[0], argv_cstr.get());
-#endif
     // if we get here, we're in serious trouble and should complain loudly
+    // NOTE: This is async signal unsafe; it could deadlock instead.  (But
+    // only on debug builds; otherwise it's a signal-safe no-op.)
     DLOG(ERROR) << "FAILED TO exec() CHILD PROCESS, path: " << argv_cstr[0];
     _exit(127);
   } else {
     gProcessLog.print("==> process %d launched child process %d\n",
                       GetCurrentProcId(), pid);
     if (wait)
       HANDLE_EINTR(waitpid(pid, 0, 0));
 
--- a/mozglue/build/BionicGlue.cpp
+++ b/mozglue/build/BionicGlue.cpp
@@ -134,57 +134,16 @@ raise(int sig)
   // Bug 943170: POSIX specifies pthread_kill(pthread_self(), sig) as
   // equivalent to raise(sig), but Bionic also has a bug with these
   // functions, where a forked child will kill its parent instead.
 
   extern pid_t gettid(void);
   return syscall(__NR_tgkill, getpid(), gettid(), sig);
 }
 
-/*
- * The following wrappers for PR_Xxx are needed until we can get
- * PR_DuplicateEnvironment landed in NSPR.
- * See see bug 772734 and bug 773414.
- *
- * We can't #include the pr headers here, and we can't call any of the
- * PR/PL functions either, so we just reimplemnt using native code.
- */
-
-static pthread_mutex_t  _pr_envLock = PTHREAD_MUTEX_INITIALIZER;
-
-extern "C" NS_EXPORT char*
-__wrap_PR_GetEnv(const char *var)
-{
-    char *ev;
-
-    pthread_mutex_lock(&_pr_envLock);
-    ev = getenv(var);
-    pthread_mutex_unlock(&_pr_envLock);
-    return ev;
-}
-
-extern "C" NS_EXPORT int
-__wrap_PR_SetEnv(const char *string)
-{
-    int result;
-
-    if ( !strchr(string, '=')) return(-1);
-
-    pthread_mutex_lock(&_pr_envLock);
-    result = putenv(const_cast<char*>(string));
-    pthread_mutex_unlock(&_pr_envLock);
-    return (result)? -1 : 0;
-}
-
-extern "C" NS_EXPORT pthread_mutex_t *
-PR_GetEnvLock(void)
-{
-  return &_pr_envLock;
-}
-
 /* Flash plugin uses symbols that are not present in Android >= 4.4 */
 #ifndef MOZ_WIDGET_GONK
 namespace android {
   namespace VectorImpl {
     NS_EXPORT void reservedVectorImpl1(void) { }
     NS_EXPORT void reservedVectorImpl2(void) { }
     NS_EXPORT void reservedVectorImpl3(void) { }
     NS_EXPORT void reservedVectorImpl4(void) { }