Bug 1536675 - Take the crashing out of MOZ_CrashPrintf r=froydnj
authorDavid Major <dmajor@mozilla.com>
Tue, 02 Apr 2019 19:20:41 +0000
changeset 467649 cc2ac1b5534f166fbc0f225c5d66946fd8f410af
parent 467648 10a446ed2a38d6af1eaa8b6d51ad9f1333b247d8
child 467650 b09ce6eebb4942cd2e570bf785c660ca6abb6258
push id35806
push userrgurzau@mozilla.com
push dateWed, 03 Apr 2019 04:07:39 +0000
treeherdermozilla-central@45808ab18609 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1536675
milestone68.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 1536675 - Take the crashing out of MOZ_CrashPrintf r=froydnj It would be helpful if MOZ_CRASH_UNSAFE_PRINTF would do its crashing inline at the caller, so that CI failure logs can blame the right code. Before this patch, MOZ_CRASH_UNSAFE_PRINTF calls MOZ_CrashPrintf, which does the printf work and crashes. This patch pulls out the crashing piece at the end, so that MOZ_CrashPrintf only does the printf work, and returns the string to the caller, who will MOZ_Crash inline. Differential Revision: https://phabricator.services.mozilla.com/D25329
mfbt/Assertions.cpp
mfbt/Assertions.h
--- a/mfbt/Assertions.cpp
+++ b/mfbt/Assertions.cpp
@@ -22,38 +22,27 @@ static char sPrintfCrashReason[sPrintfCr
 
 // Accesses to this atomic are not included in web replay recordings, so that
 // if we crash in an area where recorded events are not allowed the true reason
 // for the crash is not obscured by a record/replay error.
 static mozilla::Atomic<bool, mozilla::SequentiallyConsistent,
                        mozilla::recordreplay::Behavior::DontPreserve>
     sCrashing(false);
 
-#ifndef DEBUG
-MFBT_API MOZ_COLD MOZ_NORETURN MOZ_NEVER_INLINE MOZ_FORMAT_PRINTF(
-    2, 3) void MOZ_CrashPrintf(int aLine, const char* aFormat, ...)
-#else
-MFBT_API MOZ_COLD MOZ_NORETURN MOZ_NEVER_INLINE
-MOZ_FORMAT_PRINTF(3, 4) void MOZ_CrashPrintf(const char* aFilename, int aLine,
-                                             const char* aFormat, ...)
-#endif
-{
+MFBT_API MOZ_COLD MOZ_NEVER_INLINE MOZ_FORMAT_PRINTF(1, 2) const
+    char* MOZ_CrashPrintf(const char* aFormat, ...) {
   if (!sCrashing.compareExchange(false, true)) {
     // In the unlikely event of a race condition, skip
     // setting the crash reason and just crash safely.
-    MOZ_REALLY_CRASH(aLine);
+    MOZ_RELEASE_ASSERT(false);
   }
   va_list aArgs;
   va_start(aArgs, aFormat);
   int ret =
       vsnprintf(sPrintfCrashReason, sPrintfCrashReasonSize, aFormat, aArgs);
   va_end(aArgs);
   MOZ_RELEASE_ASSERT(
       ret >= 0 && size_t(ret) < sPrintfCrashReasonSize,
       "Could not write the explanation string to the supplied buffer!");
-#ifdef DEBUG
-  MOZ_Crash(aFilename, aLine, sPrintfCrashReason);
-#else
-  MOZ_Crash(nullptr, aLine, sPrintfCrashReason);
-#endif
+  return sPrintfCrashReason;
 }
 
 MOZ_END_EXTERN_C
--- a/mfbt/Assertions.h
+++ b/mfbt/Assertions.h
@@ -299,65 +299,54 @@ MOZ_NoReturn(int aLine) {
  * strings from a potentially compromised process is not without risk. If the
  * string being passed is the result of a printf-style function, consider using
  * MOZ_CRASH_UNSAFE_PRINTF instead.
  *
  * @note This macro causes data collection because crash strings are annotated
  * to crash-stats and are publicly visible. Firefox data stewards must do data
  * review on usages of this macro.
  */
-static inline MOZ_COLD MOZ_NORETURN void MOZ_Crash(const char* aFilename,
-                                                   int aLine,
-                                                   const char* aReason) {
+static MOZ_ALWAYS_INLINE MOZ_COLD MOZ_NORETURN void MOZ_Crash(
+    const char* aFilename, int aLine, const char* aReason) {
 #ifdef DEBUG
   MOZ_ReportCrash(aReason, aFilename, aLine);
 #endif
   MOZ_CRASH_ANNOTATE(aReason);
   MOZ_REALLY_CRASH(aLine);
 }
 #define MOZ_CRASH_UNSAFE(reason) MOZ_Crash(__FILE__, __LINE__, reason)
 
 static const size_t sPrintfMaxArgs = 4;
 static const size_t sPrintfCrashReasonSize = 1024;
 
-#ifndef DEBUG
-MFBT_API MOZ_COLD MOZ_NORETURN MOZ_NEVER_INLINE MOZ_FORMAT_PRINTF(
-    2, 3) void MOZ_CrashPrintf(int aLine, const char* aFormat, ...);
-#  define MOZ_CALL_CRASH_PRINTF(format, ...) \
-    MOZ_CrashPrintf(__LINE__, format, __VA_ARGS__)
-#else
-MFBT_API MOZ_COLD MOZ_NORETURN MOZ_NEVER_INLINE MOZ_FORMAT_PRINTF(
-    3, 4) void MOZ_CrashPrintf(const char* aFilename, int aLine,
-                               const char* aFormat, ...);
-#  define MOZ_CALL_CRASH_PRINTF(format, ...) \
-    MOZ_CrashPrintf(__FILE__, __LINE__, format, __VA_ARGS__)
-#endif
+MFBT_API MOZ_COLD MOZ_NEVER_INLINE MOZ_FORMAT_PRINTF(1, 2) const
+    char* MOZ_CrashPrintf(const char* aFormat, ...);
 
 /*
  * MOZ_CRASH_UNSAFE_PRINTF(format, arg1 [, args]) can be used when more
  * information is desired than a string literal can supply. The caller provides
  * a printf-style format string, which must be a string literal and between
  * 1 and 4 additional arguments. A regular MOZ_CRASH() is preferred wherever
  * possible, as passing arbitrary strings to printf from a potentially
  * compromised process is not without risk.
  *
  * @note This macro causes data collection because crash strings are annotated
  * to crash-stats and are publicly visible. Firefox data stewards must do data
  * review on usages of this macro.
  */
-#define MOZ_CRASH_UNSAFE_PRINTF(format, ...)                              \
-  do {                                                                    \
-    static_assert(MOZ_ARG_COUNT(__VA_ARGS__) > 0,                         \
-                  "Did you forget arguments to MOZ_CRASH_UNSAFE_PRINTF? " \
-                  "Or maybe you want MOZ_CRASH instead?");                \
-    static_assert(MOZ_ARG_COUNT(__VA_ARGS__) <= sPrintfMaxArgs,           \
-                  "Only up to 4 additional arguments are allowed!");      \
-    static_assert(sizeof(format) <= sPrintfCrashReasonSize,               \
-                  "The supplied format string is too long!");             \
-    MOZ_CALL_CRASH_PRINTF("" format, __VA_ARGS__);                        \
+#define MOZ_CRASH_UNSAFE_PRINTF(format, ...)                                \
+  do {                                                                      \
+    static_assert(MOZ_ARG_COUNT(__VA_ARGS__) > 0,                           \
+                  "Did you forget arguments to MOZ_CRASH_UNSAFE_PRINTF? "   \
+                  "Or maybe you want MOZ_CRASH instead?");                  \
+    static_assert(MOZ_ARG_COUNT(__VA_ARGS__) <= sPrintfMaxArgs,             \
+                  "Only up to 4 additional arguments are allowed!");        \
+    static_assert(sizeof(format) <= sPrintfCrashReasonSize,                 \
+                  "The supplied format string is too long!");               \
+    MOZ_Crash(__FILE__, __LINE__, MOZ_CrashPrintf("" format, __VA_ARGS__)); \
   } while (false)
 
 MOZ_END_EXTERN_C
 
 /*
  * MOZ_ASSERT(expr [, explanation-string]) asserts that |expr| must be truthy in
  * debug builds.  If it is, execution continues.  Otherwise, an error message
  * including the expression and the explanation-string (if provided) is printed,