Bug 1622452 - Remove the remote exception handler when a content process shuts down r=froydnj

This works on all platforms with the exception of Linux where we remove the
exception handler only if the sandbox is disabled. With the sandbox enabled we
would have to whitelist sigaltstack() which breakpad uses to remove the
alternate signal stack which is not worth the fuss.

Besides this patch refactors the code that sets and unsets the exception
handler, cutting down on the duplication:

* The XRE_UnsetRemoteExceptionHandler() call is removed from XULAppAPI.h since it
  was no longer used
* The duplicate checks for the special strings used to disable the remote exception
  handler have been removed from CrashReporter::UnsetRemoteExceptionHandler()
  leaving them in the calling code
* The SetRemoteExceptionHandler() function was consolidated into only one
  piece of code with only one non-platform-specific shared prototype
* Some additional code was factored out to improve the readability

These changes pave the way both for bug 1614933 and for the oxidation of the
exception handler code.

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

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

toolkit/crashreporter/ThreadAnnotation.h

@@ -19,21 +19,6 @@ void ShutdownThreadAnnotation();
 void GetFlatThreadAnnotation(const std::function<void(const char*)>& aCallback,
                              bool aIsHandlingException = false);
 
-class InitThreadAnnotationRAII {
- public:
-  InitThreadAnnotationRAII() {
-    if (GetEnabled()) {
-      InitThreadAnnotation();
-    }
-  }
-
-  ~InitThreadAnnotationRAII() {
-    if (GetEnabled()) {
-      ShutdownThreadAnnotation();
-    }
-  }
-};
-
 }  // namespace CrashReporter
 
 #endif

toolkit/crashreporter/nsExceptionHandler.cpp

@@ -3488,37 +3488,7 @@ void DeregisterChildCrashAnnotationFileDescriptor(ProcessId aProcess) {
   }
 }
 
-#if defined(XP_WIN)
-// Child-side API
-bool SetRemoteExceptionHandler(const nsACString& crashPipe,
-                               uintptr_t aCrashTimeAnnotationFile) {
-  // crash reporting is disabled
-  if (crashPipe.Equals(kNullNotifyPipe)) return true;
-
-  MOZ_ASSERT(!gExceptionHandler, "crash client already init'd");
-
-  gExceptionHandler = new google_breakpad::ExceptionHandler(
-      L"", ChildFPEFilter,
-      nullptr,  // no minidump callback
-      reinterpret_cast<void*>(aCrashTimeAnnotationFile),
-      google_breakpad::ExceptionHandler::HANDLER_ALL, GetMinidumpType(),
-      NS_ConvertASCIItoUTF16(crashPipe).get(), nullptr);
-  gExceptionHandler->set_handle_debug_exceptions(true);
-
-#  if defined(HAVE_64BIT_BUILD)
-  SetJitExceptionHandler();
-#  endif
-
-  mozalloc_set_oom_abort_handler(AnnotateOOMAllocationSize);
-
-  oldTerminateHandler = std::set_terminate(&TerminateHandler);
-
-  // we either do remote or nothing, no fallback to regular crash reporting
-  return gExceptionHandler->IsOutOfProcess();
-}
-
-//--------------------------------------------------
-#elif defined(XP_LINUX)
+#if defined(XP_LINUX)
 
 // Parent-side API for children
 bool CreateNotificationPipeForChild(int* childCrashFd, int* childCrashRemapFd) {
@@ -3536,10 +3506,25 @@ bool CreateNotificationPipeForChild(int* childCrashFd, int* childCrashRemapFd) {
   return true;
 }
 
-// Child-side API
-bool SetRemoteExceptionHandler() {
+#endif  // defined(XP_LINUX)
+
+bool SetRemoteExceptionHandler(const char* aCrashPipe,
+                               uintptr_t aCrashTimeAnnotationFile) {
   MOZ_ASSERT(!gExceptionHandler, "crash client already init'd");
 
+#if defined(XP_WIN)
+  gExceptionHandler = new google_breakpad::ExceptionHandler(
+      L"", ChildFPEFilter,
+      nullptr,  // no minidump callback
+      reinterpret_cast<void*>(aCrashTimeAnnotationFile),
+      google_breakpad::ExceptionHandler::HANDLER_ALL, GetMinidumpType(),
+      NS_ConvertASCIItoUTF16(aCrashPipe).get(), nullptr);
+  gExceptionHandler->set_handle_debug_exceptions(true);
+
+#  if defined(HAVE_64BIT_BUILD)
+  SetJitExceptionHandler();
+#  endif
+#elif defined(XP_LINUX)
   // MinidumpDescriptor requires a non-empty path.
   google_breakpad::MinidumpDescriptor path(".");
 
@@ -3549,39 +3534,24 @@ bool SetRemoteExceptionHandler() {
                                             nullptr,  // no callback context
                                             true,     // install signal handlers
                                             gMagicChildCrashReportFd);
-
-  mozalloc_set_oom_abort_handler(AnnotateOOMAllocationSize);
-
-  oldTerminateHandler = std::set_terminate(&TerminateHandler);
-
-  // we either do remote or nothing, no fallback to regular crash reporting
-  return gExceptionHandler->IsOutOfProcess();
-}
-
-//--------------------------------------------------
 #elif defined(XP_MACOSX)
-// Child-side API
-bool SetRemoteExceptionHandler(const nsACString& crashPipe) {
-  // crash reporting is disabled
-  if (crashPipe.Equals(kNullNotifyPipe)) return true;
-
-  MOZ_ASSERT(!gExceptionHandler, "crash client already init'd");
-
   gExceptionHandler =
       new google_breakpad::ExceptionHandler("", ChildFilter,
                                             nullptr,  // no minidump callback
                                             nullptr,  // no callback context
                                             true,     // install signal handlers
-                                            crashPipe.BeginReading());
+                                            aCrashPipe);
+#endif
 
   mozalloc_set_oom_abort_handler(AnnotateOOMAllocationSize);
 
   oldTerminateHandler = std::set_terminate(&TerminateHandler);
 
+  InitThreadAnnotation();
+
   // we either do remote or nothing, no fallback to regular crash reporting
   return gExceptionHandler->IsOutOfProcess();
 }
-#endif  // XP_WIN
 
 void GetAnnotation(uint32_t childPid, Annotation annotation,
                    nsACString& outStr) {
@@ -3903,9 +3873,15 @@ bool CreateAdditionalChildMinidump(ProcessHandle childPid,
 }
 
 bool UnsetRemoteExceptionHandler() {
+  // On Linux we don't unset breakpad's exception handler if the sandbox is
+  // enabled because it requires invoking `sigaltstack` and we don't want to
+  // allow that syscall in the sandbox. See bug 1622452.
+#if !defined(XP_LINUX) || !defined(MOZ_SANDBOX)
   std::set_terminate(oldTerminateHandler);
   delete gExceptionHandler;
   gExceptionHandler = nullptr;
+#endif
+  ShutdownThreadAnnotation();
   return true;
 }
 

toolkit/crashreporter/nsExceptionHandler.h

@@ -285,15 +285,6 @@ class InjectorCrashCallback {
 void InjectCrashReporterIntoProcess(DWORD processID, InjectorCrashCallback* cb);
 void UnregisterInjectorCallback(DWORD processID);
 #  endif
-
-// Child-side API
-#  if defined(XP_WIN)
-bool SetRemoteExceptionHandler(const nsACString& crashPipe,
-                               uintptr_t aCrashTimeAnnotationFile);
-#  else
-bool SetRemoteExceptionHandler(const nsACString& crashPipe);
-#  endif
-
 #else
 // Parent-side API for children
 
@@ -307,11 +298,11 @@ bool SetRemoteExceptionHandler(const nsACString& crashPipe);
 // and |true| will be returned.
 bool CreateNotificationPipeForChild(int* childCrashFd, int* childCrashRemapFd);
 
-// Child-side API
-bool SetRemoteExceptionHandler();
-
 #endif  // XP_WIN
 
+// Child-side API
+bool SetRemoteExceptionHandler(const char* aCrashPipe = nullptr,
+                               uintptr_t aCrashTimeAnnotationFile = 0);
 bool UnsetRemoteExceptionHandler();
 
 #if defined(MOZ_WIDGET_ANDROID)

toolkit/xre/nsEmbedFunctions.cpp

@@ -40,7 +40,6 @@
 #ifdef MOZ_ASAN_REPORTER
 #  include "CmdLineAndEnvUtils.h"
 #endif
-#include "ThreadAnnotation.h"
 
 #include "mozilla/Omnijar.h"
 #if defined(XP_MACOSX)
@@ -278,24 +277,6 @@ void XRE_SetProcessType(const char* aProcessTypeString) {
   }
 }
 
-bool
-#if defined(XP_WIN)
-XRE_SetRemoteExceptionHandler(const char* aPipe /*= 0*/,
-                              uintptr_t aCrashTimeAnnotationFile)
-#else
-XRE_SetRemoteExceptionHandler(const char* aPipe /*= 0*/)
-#endif
-{
-#if defined(XP_WIN)
-  return CrashReporter::SetRemoteExceptionHandler(nsDependentCString(aPipe),
-                                                  aCrashTimeAnnotationFile);
-#elif defined(XP_MACOSX)
-  return CrashReporter::SetRemoteExceptionHandler(nsDependentCString(aPipe));
-#else
-  return CrashReporter::SetRemoteExceptionHandler();
-#endif
-}
-
 #if defined(XP_WIN)
 void SetTaskbarGroupId(const nsString& aId) {
   if (FAILED(SetCurrentProcessExplicitAppUserModelID(aId.get()))) {
@@ -337,6 +318,17 @@ int GetDebugChildPauseTime() {
 #endif
 }
 
+static bool IsCrashReporterEnabled(const char* aArg) {
+  // on windows and mac, |aArg| is the named pipe on which the server is
+  // listening for requests, or "-" if crash reporting is disabled.
+#if defined(XP_MACOSX) || defined(XP_WIN)
+  return 0 != strcmp("-", aArg);
+#else
+  // on POSIX, |aArg| is "true" if crash reporting is enabled, false otherwise
+  return 0 != strcmp("false", aArg);
+#endif
+}
+
 }  // namespace
 
 nsresult XRE_InitChildProcess(int aArgc, char* aArgv[],
@@ -511,6 +503,7 @@ nsresult XRE_InitChildProcess(int aArgc, char* aArgv[],
 
   SetupErrorHandling(aArgv[0]);
 
+  bool exceptionHandlerIsSet = false;
   if (!CrashReporter::IsDummy()) {
 #if defined(XP_WIN)
     if (aArgc < 1) {
@@ -524,35 +517,23 @@ nsresult XRE_InitChildProcess(int aArgc, char* aArgv[],
     if (aArgc < 1) return NS_ERROR_FAILURE;
     const char* const crashReporterArg = aArgv[--aArgc];
 
+    if (IsCrashReporterEnabled(crashReporterArg)) {
 #if defined(XP_MACOSX)
-    // on windows and mac, |crashReporterArg| is the named pipe on which the
-    // server is listening for requests, or "-" if crash reporting is
-    // disabled.
-    if (0 != strcmp("-", crashReporterArg) &&
-        !XRE_SetRemoteExceptionHandler(crashReporterArg)) {
-      // Bug 684322 will add better visibility into this condition
-      NS_WARNING("Could not setup crash reporting\n");
-    }
+      exceptionHandlerIsSet =
+          CrashReporter::SetRemoteExceptionHandler(crashReporterArg);
 #elif defined(XP_WIN)
-    if (0 != strcmp("-", crashReporterArg) &&
-        !XRE_SetRemoteExceptionHandler(crashReporterArg,
-                                       crashTimeAnnotationFile)) {
-      // Bug 684322 will add better visibility into this condition
-      NS_WARNING("Could not setup crash reporting\n");
-    }
+      exceptionHandlerIsSet = CrashReporter::SetRemoteExceptionHandler(
+          crashReporterArg, crashTimeAnnotationFile);
 #else
-    // on POSIX, |crashReporterArg| is "true" if crash reporting is
-    // enabled, false otherwise
-    if (0 != strcmp("false", crashReporterArg) &&
-        !XRE_SetRemoteExceptionHandler(nullptr)) {
-      // Bug 684322 will add better visibility into this condition
-      NS_WARNING("Could not setup crash reporting\n");
-    }
+      exceptionHandlerIsSet = CrashReporter::SetRemoteExceptionHandler();
 #endif
-  }
 
-  // For Init/Shutdown thread name annotations in the crash reporter.
-  CrashReporter::InitThreadAnnotationRAII annotation;
+      if (!exceptionHandlerIsSet) {
+        // Bug 684322 will add better visibility into this condition
+        NS_WARNING("Could not setup crash reporting\n");
+      }
+    }
+  }
 
   gArgv = aArgv;
   gArgc = aArgc;
@@ -772,6 +753,10 @@ nsresult XRE_InitChildProcess(int aArgc, char* aArgv[],
     }
   }
 
+  if (exceptionHandlerIsSet) {
+    CrashReporter::UnsetRemoteExceptionHandler();
+  }
+
   return XRE_DeinitCommandLine();
 }
 

xpcom/build/nsXULAppAPI.h

@@ -400,16 +400,6 @@ XRE_API(void, XRE_SetAndroidChildFds,
 
 XRE_API(void, XRE_SetProcessType, (const char* aProcessTypeString))
 
-// Used in child processes.
-#if defined(XP_WIN)
-// Uses uintptr_t, even though it's really a HANDLE, because including
-// <windows.h> here caused compilation issues.
-XRE_API(bool, XRE_SetRemoteExceptionHandler,
-        (const char* aPipe, uintptr_t aCrashTimeAnnotationFile))
-#else
-XRE_API(bool, XRE_SetRemoteExceptionHandler, (const char* aPipe))
-#endif
-
 XRE_API(nsresult, XRE_InitChildProcess,
         (int aArgc, char* aArgv[], const XREChildData* aChildData))