Bug 1348374 - Remove paf_child(). r=jseward.
authorNicholas Nethercote <nnethercote@mozilla.com>
Tue, 21 Mar 2017 09:52:15 +1100
changeset 501805 3026456dca8ccbb45d1527e21dcff4ed5149630c
parent 501804 c9812b88b9ed45fa80e0c45a8e80cf89c66d4c8f
child 501806 705a77b42fc16df0baf5a770560fb980354a9487
push id50127
push userna-g@nostrum.com
push dateTue, 21 Mar 2017 00:09:45 +0000
reviewersjseward
bugs1348374
milestone55.0a1
Bug 1348374 - Remove paf_child(). r=jseward. It's not necessary and causes hangs. The patch also inlines setup_atfork() and moves the Linux-only code closer to the Linux-only PlatformInit(), and tweaks the comments a bit.
tools/profiler/core/platform-linux-android.cpp
--- a/tools/profiler/core/platform-linux-android.cpp
+++ b/tools/profiler/core/platform-linux-android.cpp
@@ -74,80 +74,16 @@
 using namespace mozilla;
 
 /* static */ Thread::tid_t
 Thread::GetCurrentId()
 {
   return gettid();
 }
 
-#if !defined(GP_OS_android)
-
-// Keep track of when any of our threads calls fork(), so we can
-// temporarily disable signal delivery during the fork() call.  Not
-// doing so appears to cause a kind of race, in which signals keep
-// getting delivered to the thread doing fork(), which keeps causing
-// it to fail and be restarted; hence forward progress is delayed a
-// great deal.  A side effect of this is to permanently disable
-// sampling in the child process.  See bug 837390.
-
-// Unfortunately this is only doable on non-Android, since Bionic
-// doesn't have pthread_atfork.
-
-// In the parent, before the fork, record the pausedness state, and then pause.
-static void
-paf_prepare()
-{
-  // This function can run off the main thread.
-
-  MOZ_RELEASE_ASSERT(gPS);
-
-  PS::AutoLock lock(gPSMutex);
-
-  gPS->SetWasPaused(lock, gPS->IsPaused(lock));
-  gPS->SetIsPaused(lock, true);
-}
-
-// In the parent, after the fork, return pausedness to the pre-fork state.
-static void
-paf_parent()
-{
-  // This function can run off the main thread.
-
-  MOZ_RELEASE_ASSERT(gPS);
-
-  PS::AutoLock lock(gPSMutex);
-
-  gPS->SetIsPaused(lock, gPS->WasPaused(lock));
-  gPS->SetWasPaused(lock, false);
-}
-
-// In the child, after the fork, leave the profiler paused.
-static void
-paf_child()
-{
-  // This function can run off the main thread.
-
-  MOZ_RELEASE_ASSERT(gPS);
-
-  PS::AutoLock lock(gPSMutex);
-
-  gPS->SetWasPaused(lock, false);
-}
-
-// Set up the fork handlers.
-static void*
-setup_atfork()
-{
-  pthread_atfork(paf_prepare, paf_parent, paf_child);
-  return nullptr;
-}
-
-#endif /* !defined(GP_OS_android) */
-
 static void SetSampleContext(TickSample* sample, mcontext_t& mcontext)
 {
   // Extracting the sample from the context is extremely machine dependent.
 #if defined(GP_ARCH_x86)
   sample->pc = reinterpret_cast<Address>(mcontext.gregs[REG_EIP]);
   sample->sp = reinterpret_cast<Address>(mcontext.gregs[REG_ESP]);
   sample->fp = reinterpret_cast<Address>(mcontext.gregs[REG_EBP]);
 #elif defined(GP_ARCH_amd64)
@@ -576,16 +512,17 @@ private:
   SamplerThread(const SamplerThread&) = delete;
   void operator=(const SamplerThread&) = delete;
 };
 
 SamplerThread::SigHandlerCoordinator* SamplerThread::sSigHandlerCoordinator =
   nullptr;
 
 #if defined(GP_OS_android)
+
 static struct sigaction gOldSigstartHandler;
 const int SIGSTART = SIGUSR2;
 
 static void freeArray(const char** array, int size) {
   for (int i = 0; i < size; i++) {
     free((void*) array[i]);
   }
 }
@@ -689,23 +626,65 @@ PlatformInit(PS::LockRef aLock)
   sa.sa_sigaction = SigstartHandler;
   sigemptyset(&sa.sa_mask);
   sa.sa_flags = SA_RESTART | SA_SIGINFO;
   if (sigaction(SIGSTART, &sa, &gOldSigstartHandler) != 0) {
     MOZ_CRASH("Error installing SIGSTART handler in the profiler");
   }
 }
 
-#else
+#else /* !defined(GP_OS_android) */
+
+// We use pthread_atfork() to temporarily disable signal delivery during any
+// fork() call. Without that, fork() can be repeatedly interrupted by signal
+// delivery, requiring it to be repeatedly restarted, which can lead to *long*
+// delays. See bug 837390.
+//
+// We provide no paf_child() function to run in the child after forking. This
+// is fine because we always immediately exec() after fork(), and exec()
+// clobbers all process state. (At one point we did have a paf_child()
+// function, but it caused problems related to locking gPSMutex. See bug
+// 1348374.)
+//
+// Unfortunately all this is only doable on non-Android because Bionic doesn't
+// have pthread_atfork.
+
+// In the parent, before the fork, record IsPaused, and then pause.
+static void
+paf_prepare()
+{
+  // This function can run off the main thread.
+
+  MOZ_RELEASE_ASSERT(gPS);
+
+  PS::AutoLock lock(gPSMutex);
+
+  gPS->SetWasPaused(lock, gPS->IsPaused(lock));
+  gPS->SetIsPaused(lock, true);
+}
+
+// In the parent, after the fork, return IsPaused to the pre-fork state.
+static void
+paf_parent()
+{
+  // This function can run off the main thread.
+
+  MOZ_RELEASE_ASSERT(gPS);
+
+  PS::AutoLock lock(gPSMutex);
+
+  gPS->SetIsPaused(lock, gPS->WasPaused(lock));
+  gPS->SetWasPaused(lock, false);
+}
 
 static void
 PlatformInit(PS::LockRef aLock)
 {
   // Set up the fork handlers.
-  setup_atfork();
+  pthread_atfork(paf_prepare, paf_parent, nullptr);
 }
 
 #endif
 
 void TickSample::PopulateContext(void* aContext)
 {
   MOZ_ASSERT(aContext);
   ucontext_t* pContext = reinterpret_cast<ucontext_t*>(aContext);