Bug 1683288 - Directly query OOMAllocationSize value and enable tests on linux/macOS r=gsvelto

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

memory/mozalloc/mozalloc_oom.cpp

@@ -9,7 +9,7 @@
 #include "mozilla/mozalloc_oom.h"
 #include "mozilla/Assertions.h"
 
-static mozalloc_oom_abort_handler gAbortHandler;
+static size_t gMozallocOOMAllocationSize = 0;
 
 #define OOM_MSG_LEADER "out of memory: 0x"
 #define OOM_MSG_DIGITS "0000000000000000"  // large enough for 2^64
@@ -36,7 +36,7 @@ void mozalloc_handle_oom(size_t size) {
   // which might be in use at the time under querySelector or
   // querySelectorAll.
 
-  if (gAbortHandler) gAbortHandler(size);
+  gMozallocOOMAllocationSize = size;
 
   static_assert(OOM_MSG_FIRST_DIGIT_OFFSET > 0,
                 "Loop below will never terminate (i can't go below 0)");
@@ -51,6 +51,4 @@ void mozalloc_handle_oom(size_t size) {
   mozalloc_abort(oomMsg);
 }
 
-void mozalloc_set_oom_abort_handler(mozalloc_oom_abort_handler handler) {
-  gAbortHandler = handler;
-}
+size_t mozalloc_get_oom_abort_size(void) { return gMozallocOOMAllocationSize; }

memory/mozalloc/mozalloc_oom.h

@@ -17,13 +17,9 @@
 MFBT_API void mozalloc_handle_oom(size_t requestedSize);
 
 /**
- * Called by embedders (specifically Mozilla breakpad) which wants to be
- * notified of an intentional abort, to annotate any crash report with
- * the size of the allocation on which we aborted.
+ * Called by crashreporter to fill OOMAllocationSize
  */
-typedef void (*mozalloc_oom_abort_handler)(size_t size);
-MFBT_API void mozalloc_set_oom_abort_handler(
-    mozalloc_oom_abort_handler handler);
+MFBT_API size_t mozalloc_get_oom_abort_size(void);
 
 /* TODO: functions to query system memory usage and register
  * critical-memory handlers. */

toolkit/crashreporter/nsExceptionHandler.cpp

@@ -1284,6 +1284,23 @@ static void WriteMainThreadRunnableName(AnnotationWriter& aWriter) {
 #endif
 }
 
+static void WriteOOMAllocationSize(AnnotationWriter& aWriter) {
+  // See bug 1683288 and bug 1682975#c7 for context
+  //
+  // We have two cases: either gOOMAllocationSize can be set from remote
+  // via CrashReporter::AnnotateOOMAllocationSize()
+  //
+  // BUT if gOOMAllocationSize is 0 we should try and fetch value from mozalloc
+  if (!gOOMAllocationSize) {
+    gOOMAllocationSize = mozalloc_get_oom_abort_size();
+  }
+
+  // This way we still are guarded by a gOOMAllocationSize=0
+  if (gOOMAllocationSize) {
+    aWriter.Write(Annotation::OOMAllocationSize, gOOMAllocationSize);
+  }
+}
+
 static void WriteMozCrashReason(AnnotationWriter& aWriter) {
   if (gMozCrashReason != nullptr) {
     aWriter.Write(Annotation::MozCrashReason, gMozCrashReason);
@@ -1352,9 +1369,7 @@ static void WriteAnnotationsForMainProcessCrash(PlatformWriter& pw,
 
   WriteMainThreadRunnableName(writer);
 
-  if (gOOMAllocationSize) {
-    writer.Write(Annotation::OOMAllocationSize, gOOMAllocationSize);
-  }
+  WriteOOMAllocationSize(writer);
 
   if (gTexturesSize) {
     writer.Write(Annotation::TextureUsage, gTexturesSize);
@@ -1640,14 +1655,12 @@ static void PrepareChildExceptionTimeAnnotations(
   apiData.OpenHandle(GetAnnotationTimeCrashFd());
   BinaryAnnotationWriter writer(apiData);
 
-  if (gOOMAllocationSize) {
-    writer.Write(Annotation::OOMAllocationSize, gOOMAllocationSize);
-  }
-
   WriteMozCrashReason(writer);
 
   WriteMainThreadRunnableName(writer);
 
+  WriteOOMAllocationSize(writer);
+
 #ifdef MOZ_PHC
   WritePHCAddrInfo(writer, addrInfo);
 #endif
@@ -2151,8 +2164,6 @@ nsresult SetExceptionHandler(nsIFile* aXREDirectory, bool force /*=false*/) {
   if (keyExistsAndHasValidFormat) showOSCrashReporter = prefValue;
 #endif
 
-  mozalloc_set_oom_abort_handler(AnnotateOOMAllocationSize);
-
   oldTerminateHandler = std::set_terminate(&TerminateHandler);
 
   return NS_OK;
@@ -3555,8 +3566,6 @@ bool SetRemoteExceptionHandler(const char* aCrashPipe,
       aCrashPipe);
 #endif
 
-  mozalloc_set_oom_abort_handler(AnnotateOOMAllocationSize);
-
   oldTerminateHandler = std::set_terminate(&TerminateHandler);
 
   // we either do remote or nothing, no fallback to regular crash reporting

toolkit/crashreporter/test/unit/test_oom_annotation.js

@@ -34,6 +34,7 @@ add_task(async function run_test() {
           break;
         case "linux":
           annotations = [
+            ["OOMAllocationSize", true],
             ["AvailablePageFile", false],
             ["AvailablePhysicalMemory", false],
             ["AvailableSwapMemory", false],
@@ -44,6 +45,7 @@ add_task(async function run_test() {
           break;
         case "macosx":
           annotations = [
+            ["OOMAllocationSize", true],
             ["AvailablePhysicalMemory", false],
             ["AvailableSwapMemory", false],
             ["PurgeablePhysicalMemory", false],