Bug 1831092 - Use the new pull-based API for all crash annotations and remove the global annotations table r=jgilbert,necko-reviewers,media-playback-reviewers,profiler-reviewers,win-reviewers,padenot,handyman,afranchuk,valentin,alwu,sotaro

This changes comes with several different refactorings all rolled into one,
unfotunately I couldn't find a way to pull them apart:
- First of all annotations now can either recorded (that is, we copy the value
  and have the crash reporting code own the copy) or registered. Several
  annotations are changed to use this functionality so that we don't need to
  update them as their value change.
- The code in the exception handler is modified to read the annotations from
  the mozannotation_client crate. This has the unfortunate side-effect that
  we need three different bits of code to serialize them: one for annotations
  read from a child process, one for reading annotations from the main process
  outside of the exception handler and one for reading annotations from the
  main process within the exception handler. As we move to fully
  out-of-process crash reporting the last two methods will go away.
- The mozannotation_client crate now doesn't record annotation types anymore.
  I realized as I was working on this that storing types at runtime has two
  issues: the first one is that buggy code might change the type of an
  annotation (that is record it under two different types at two different
  moments), the second issue is that types might become corrupt during a
  crash, so better enforce them at annotation-writing time. The end result is
  that the mozannotation_* crates now only store byte buffers, track the
  format the data is stored in (null-terminated string, fixed size buffer,
  etc...) but not the type of data each annotation is supposed to contain.
- Which brings us to the next change: concrete types for annotations are now
  enforced when they're written out. If an annotation doesn't match the
  expected type it's skipped. Storing an annotation with the wrong type will
  also trigger an assertion in debug builds.

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

Author: gabrielesvelto

Cargo.lock

@@ -1522,8 +1522,8 @@ bool nsAccessibilityService::Init() {
   NS_ADDREF(gApplicationAccessible);  // will release in Shutdown()
   gApplicationAccessible->Init();
 
-  CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::Accessibility,
-                                     "Active"_ns);
+  CrashReporter::RecordAnnotationCString(
+      CrashReporter::Annotation::Accessibility, "Active");
 
   // Now its safe to start platform accessibility.
   if (XRE_IsParentProcess()) PlatformInit();

accessible/base/nsAccessibilityService.cpp

@@ -256,7 +256,7 @@ - (void)accessibilitySetValue:(id)value forAttribute:(NSString*)attribute {
       mozilla::Telemetry::ScalarSet(
           mozilla::Telemetry::ScalarID::A11Y_INSTANTIATORS, client);
 #endif  // defined(MOZ_TELEMETRY_REPORTING)
-      CrashReporter::AnnotateCrashReport(
+      CrashReporter::RecordAnnotationNSCString(
           CrashReporter::Annotation::AccessibilityClient,
           NS_ConvertUTF16toUTF8(client));
     }

accessible/mac/Platform.mm

@@ -103,7 +103,7 @@ void Compatibility::Init() {
   // Note we collect some AT statistics/telemetry here for convenience.
   InitConsumers();
 
-  CrashReporter::AnnotateCrashReport(
+  CrashReporter::RecordAnnotationNSCString(
       CrashReporter::Annotation::AccessibilityInProcClient,
       nsPrintfCString("0x%X", sConsumers));
 

accessible/windows/msaa/Compatibility.cpp

@@ -197,9 +197,8 @@ static void AccumulateInstantiatorTelemetry(const nsAString& aValue) {
 #if defined(MOZ_TELEMETRY_REPORTING)
     Telemetry::ScalarSet(Telemetry::ScalarID::A11Y_INSTANTIATORS, aValue);
 #endif  // defined(MOZ_TELEMETRY_REPORTING)
-    CrashReporter::AnnotateCrashReport(
-        CrashReporter::Annotation::AccessibilityClient,
-        NS_ConvertUTF16toUTF8(aValue));
+    CrashReporter::RecordAnnotationNSString(
+        CrashReporter::Annotation::AccessibilityClient, aValue);
   }
 }
 

accessible/windows/msaa/Platform.cpp

@@ -11,7 +11,6 @@
 #include "mozilla/Types.h"
 #include "mozilla/WindowsDllBlocklist.h"
 
-#include "CrashAnnotations.h"
 #include "DllBlocklist.h"
 #include "LoaderPrivateAPI.h"
 #include "ModuleLoadFrame.h"
@@ -136,12 +135,8 @@ void NativeNtBlockSet::Write(WritableBuffer& aBuffer) {
 
 static NativeNtBlockSet gBlockSet;
 
-extern "C" void MOZ_EXPORT
-NativeNtBlockSet_Write(CrashReporter::AnnotationWriter& aWriter) {
-  WritableBuffer buffer;
-  gBlockSet.Write(buffer);
-  aWriter.Write(CrashReporter::Annotation::BlockedDllList, buffer.Data(),
-                buffer.Length());
+extern "C" void MOZ_EXPORT NativeNtBlockSet_Write(WritableBuffer& aBuffer) {
+  gBlockSet.Write(aBuffer);
 }
 
 enum class BlockAction {

browser/app/winlauncher/freestanding/DllBlocklist.cpp

@@ -38,7 +38,7 @@ void moz_wasm2c_memgrow_failed() {
 #  ifdef WASM_RT_GROW_FAILED_CRASH
   MOZ_CRASH("wasm2c memory grow failed");
 #  else
-  CrashReporter::AnnotateCrashReport(
+  CrashReporter::RecordAnnotationBool(
       CrashReporter::Annotation::WasmLibrarySandboxMallocFailed, true);
 #  endif
 }

config/external/rlbox_wasm2c_sandbox/rlbox_wasm2c_thread_locals.cpp

@@ -161,6 +161,9 @@ static StaticAutoPtr<BrowsingContextMap> sBrowsingContexts;
 // Top-level Content BrowsingContexts only, indexed by BrowserId instead of Id
 static StaticAutoPtr<BrowsingContextMap> sCurrentTopByBrowserId;
 
+static bool gIPCEnabledAnnotation = false;
+static bool gFissionEnabledAnnotation = false;
+
 static void UnregisterBrowserId(BrowsingContext* aBrowsingContext) {
   if (!aBrowsingContext->IsTopContent() || !sCurrentTopByBrowserId) {
     return;
@@ -248,6 +251,11 @@ void BrowsingContext::Init() {
     sCurrentTopByBrowserId = new BrowsingContextMap();
     ClearOnShutdown(&sBrowsingContexts);
     ClearOnShutdown(&sCurrentTopByBrowserId);
+    CrashReporter::RegisterAnnotationBool(
+        CrashReporter::Annotation::DOMIPCEnabled, &gIPCEnabledAnnotation);
+    CrashReporter::RegisterAnnotationBool(
+        CrashReporter::Annotation::DOMFissionEnabled,
+        &gFissionEnabledAnnotation);
   }
 }
 
@@ -1658,11 +1666,8 @@ NS_IMETHODIMP BrowsingContext::SetRemoteTabs(bool aUseRemoteTabs) {
     return NS_ERROR_FAILURE;
   }
 
-  static bool annotated = false;
-  if (aUseRemoteTabs && !annotated) {
-    annotated = true;
-    CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::DOMIPCEnabled,
-                                       true);
+  if (aUseRemoteTabs && !gIPCEnabledAnnotation) {
+    gIPCEnabledAnnotation = true;
   }
 
   // Don't allow non-remote tabs with remote subframes.
@@ -1686,11 +1691,8 @@ NS_IMETHODIMP BrowsingContext::SetRemoteSubframes(bool aUseRemoteSubframes) {
     return NS_ERROR_FAILURE;
   }
 
-  static bool annotated = false;
-  if (aUseRemoteSubframes && !annotated) {
-    annotated = true;
-    CrashReporter::AnnotateCrashReport(
-        CrashReporter::Annotation::DOMFissionEnabled, true);
+  if (aUseRemoteSubframes && !gFissionEnabledAnnotation) {
+    gFissionEnabledAnnotation = true;
   }
 
   // Don't allow non-remote tabs with remote subframes.

docshell/base/BrowsingContext.cpp

@@ -354,13 +354,13 @@ struct EnumSerializer {
   static bool Read(ConsumerView<U>& aConsumerView, ParamType* aResult) {
     DataType value;
     if (!aConsumerView.ReadParam(&value)) {
-      CrashReporter::AnnotateCrashReport(
-          CrashReporter::Annotation::IPCReadErrorReason, "Bad iter"_ns);
+      CrashReporter::RecordAnnotationCString(
+          CrashReporter::Annotation::IPCReadErrorReason, "Bad iter");
       return false;
     }
     if (!EnumValidator::IsLegalValue(static_cast<DataType>(value))) {
-      CrashReporter::AnnotateCrashReport(
-          CrashReporter::Annotation::IPCReadErrorReason, "Illegal value"_ns);
+      CrashReporter::RecordAnnotationCString(
+          CrashReporter::Annotation::IPCReadErrorReason, "Illegal value");
       return false;
     }
 

dom/canvas/QueueParamTraits.h

@@ -233,7 +233,7 @@ bool TouchEvent::PrefEnabled(nsIDocShell* aDocShell) {
       // The touch screen data seems to be inaccurate in the parent process,
       // and we really need the crash annotation in child processes.
       if (firstTime && !XRE_IsParentProcess()) {
-        CrashReporter::AnnotateCrashReport(
+        CrashReporter::RecordAnnotationBool(
             CrashReporter::Annotation::HasDeviceTouchScreen, enabled);
         firstTime = false;
       }

dom/events/TouchEvent.cpp

@@ -814,7 +814,8 @@ mozilla::ipc::IPCResult BrowserChild::RecvLoadURL(
   }
   docShell->LoadURI(aLoadState, true);
 
-  CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::URL, spec);
+  CrashReporter::RecordAnnotationNSCString(CrashReporter::Annotation::URL,
+                                           spec);
   return IPC_OK();
 }
 
@@ -3481,8 +3482,8 @@ NS_IMETHODIMP BrowserChild::OnLocationChange(nsIWebProgress* aWebProgress,
         annotationURI = aLocation;
       }
 
-      CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::URL,
-                                         annotationURI->GetSpecOrDefault());
+      CrashReporter::RecordAnnotationNSCString(
+          CrashReporter::Annotation::URL, annotationURI->GetSpecOrDefault());
     }
 #endif
   }

dom/ipc/BrowserChild.cpp

@@ -1714,12 +1714,12 @@ mozilla::ipc::IPCResult ContentChild::RecvSetProcessSandbox(
   DisconnectWindowServer(sandboxEnabled);
 #  endif
 
-  CrashReporter::AnnotateCrashReport(
+  CrashReporter::RecordAnnotationBool(
       CrashReporter::Annotation::ContentSandboxEnabled, sandboxEnabled);
 #  if defined(XP_LINUX) && !defined(ANDROID)
-  CrashReporter::AnnotateCrashReport(
+  CrashReporter::RecordAnnotationU32(
       CrashReporter::Annotation::ContentSandboxCapabilities,
-      static_cast<int>(SandboxInfo::Get().AsInteger()));
+      SandboxInfo::Get().AsInteger());
 #  endif /* XP_LINUX && !ANDROID */
 #endif   /* MOZ_SANDBOX */
 
@@ -2223,9 +2223,8 @@ void ContentChild::ProcessingError(Result aCode, const char* aReason) {
       MOZ_CRASH("not reached");
   }
 
-  nsDependentCString reason(aReason);
-  CrashReporter::AnnotateCrashReport(
-      CrashReporter::Annotation::ipc_channel_error, reason);
+  CrashReporter::RecordAnnotationCString(
+      CrashReporter::Annotation::ipc_channel_error, aReason);
 
   MOZ_CRASH("Content child abort due to IPC error");
 }
@@ -2677,8 +2676,8 @@ mozilla::ipc::IPCResult ContentChild::RecvRemoteType(
   }
 
   // Use the prefix to avoid URIs from Fission isolated processes.
-  CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::RemoteType,
-                                     remoteTypePrefix);
+  CrashReporter::RecordAnnotationNSCString(
+      CrashReporter::Annotation::RemoteType, remoteTypePrefix);
 
   // Defer RemoteWorkerService initialization until the child process does
   // receive its specific remoteType and can become actionable for the
@@ -2951,8 +2950,7 @@ void ContentChild::ForceKillTimerCallback(nsITimer* aTimer, void* aClosure) {
 }
 
 mozilla::ipc::IPCResult ContentChild::RecvShutdownConfirmedHP() {
-  CrashReporter::AppendToCrashReportAnnotation(
-      CrashReporter::Annotation::IPCShutdownState,
+  ProcessChild::AppendToIPCShutdownStateAnnotation(
       "RecvShutdownConfirmedHP entry"_ns);
 
   // Bug 1755376: If we see "RecvShutdownConfirmedHP entry" often in
@@ -2963,8 +2961,7 @@ mozilla::ipc::IPCResult ContentChild::RecvShutdownConfirmedHP() {
 }
 
 mozilla::ipc::IPCResult ContentChild::RecvShutdown() {
-  CrashReporter::AppendToCrashReportAnnotation(
-      CrashReporter::Annotation::IPCShutdownState, "RecvShutdown entry"_ns);
+  ProcessChild::AppendToIPCShutdownStateAnnotation("RecvShutdown entry"_ns);
 
   // Signal the ongoing shutdown to AppShutdown, this
   // will make abort nested SpinEventLoopUntilOrQuit loops
@@ -2973,8 +2970,7 @@ mozilla::ipc::IPCResult ContentChild::RecvShutdown() {
 
   nsCOMPtr<nsIObserverService> os = services::GetObserverService();
   if (os) {
-    CrashReporter::AppendToCrashReportAnnotation(
-        CrashReporter::Annotation::IPCShutdownState,
+    ProcessChild::AppendToIPCShutdownStateAnnotation(
         "content-child-will-shutdown started"_ns);
 
     os->NotifyObservers(ToSupports(this), "content-child-will-shutdown",
@@ -2986,8 +2982,7 @@ mozilla::ipc::IPCResult ContentChild::RecvShutdown() {
 }
 
 void ContentChild::ShutdownInternal() {
-  CrashReporter::AppendToCrashReportAnnotation(
-      CrashReporter::Annotation::IPCShutdownState, "ShutdownInternal entry"_ns);
+  ProcessChild::AppendToIPCShutdownStateAnnotation("ShutdownInternal entry"_ns);
 
   // If we receive the shutdown message from within a nested event loop, we want
   // to wait for that event loop to finish. Otherwise we could prematurely
@@ -3025,8 +3020,7 @@ void ContentChild::ShutdownInternal() {
 
   nsCOMPtr<nsIObserverService> os = services::GetObserverService();
   if (os) {
-    CrashReporter::AppendToCrashReportAnnotation(
-        CrashReporter::Annotation::IPCShutdownState,
+    ProcessChild::AppendToIPCShutdownStateAnnotation(
         "content-child-shutdown started"_ns);
     os->NotifyObservers(ToSupports(this), "content-child-shutdown", nullptr);
   }
@@ -3035,21 +3029,21 @@ void ContentChild::ShutdownInternal() {
 
   if (mProfilerController) {
     const bool isProfiling = profiler_is_active();
-    CrashReporter::AnnotateCrashReport(
+    CrashReporter::RecordAnnotationCString(
         CrashReporter::Annotation::ProfilerChildShutdownPhase,
-        isProfiling ? "Profiling - GrabShutdownProfileAndShutdown"_ns
-                    : "Not profiling - GrabShutdownProfileAndShutdown"_ns);
+        isProfiling ? "Profiling - GrabShutdownProfileAndShutdown"
+                    : "Not profiling - GrabShutdownProfileAndShutdown");
     ProfileAndAdditionalInformation shutdownProfileAndAdditionalInformation =
         mProfilerController->GrabShutdownProfileAndShutdown();
-    CrashReporter::AnnotateCrashReport(
+    CrashReporter::RecordAnnotationCString(
         CrashReporter::Annotation::ProfilerChildShutdownPhase,
-        isProfiling ? "Profiling - Destroying ChildProfilerController"_ns
-                    : "Not profiling - Destroying ChildProfilerController"_ns);
+        isProfiling ? "Profiling - Destroying ChildProfilerController"
+                    : "Not profiling - Destroying ChildProfilerController");
     mProfilerController = nullptr;
-    CrashReporter::AnnotateCrashReport(
+    CrashReporter::RecordAnnotationCString(
         CrashReporter::Annotation::ProfilerChildShutdownPhase,
-        isProfiling ? "Profiling - SendShutdownProfile (sending)"_ns
-                    : "Not profiling - SendShutdownProfile (sending)"_ns);
+        isProfiling ? "Profiling - SendShutdownProfile (sending)"
+                    : "Not profiling - SendShutdownProfile (sending)");
     if (const size_t len = shutdownProfileAndAdditionalInformation.SizeOf();
         len >= size_t(IPC::Channel::kMaximumMessageSize)) {
       shutdownProfileAndAdditionalInformation.mProfile = nsPrintfCString(
@@ -3061,13 +3055,12 @@ void ContentChild::ShutdownInternal() {
     // message channel, which we know will survive for long enough.
     bool sent =
         SendShutdownProfile(shutdownProfileAndAdditionalInformation.mProfile);
-    CrashReporter::AnnotateCrashReport(
+    CrashReporter::RecordAnnotationCString(
         CrashReporter::Annotation::ProfilerChildShutdownPhase,
-        sent ? (isProfiling ? "Profiling - SendShutdownProfile (sent)"_ns
-                            : "Not profiling - SendShutdownProfile (sent)"_ns)
-             : (isProfiling
-                    ? "Profiling - SendShutdownProfile (failed)"_ns
-                    : "Not profiling - SendShutdownProfile (failed)"_ns));
+        sent ? (isProfiling ? "Profiling - SendShutdownProfile (sent)"
+                            : "Not profiling - SendShutdownProfile (sent)")
+             : (isProfiling ? "Profiling - SendShutdownProfile (failed)"
+                            : "Not profiling - SendShutdownProfile (failed)"));
   }
 
   if (PerfStats::GetCollectionMask() != 0) {
@@ -3077,12 +3070,10 @@ void ContentChild::ShutdownInternal() {
   // Start a timer that will ensure we quickly exit after a reasonable period
   // of time. Prevents shutdown hangs after our connection to the parent
   // closes or when the parent is too busy to ever kill us.
-  CrashReporter::AppendToCrashReportAnnotation(
-      CrashReporter::Annotation::IPCShutdownState, "StartForceKillTimer"_ns);
+  ProcessChild::AppendToIPCShutdownStateAnnotation("StartForceKillTimer"_ns);
   StartForceKillTimer();
 
-  CrashReporter::AppendToCrashReportAnnotation(
-      CrashReporter::Annotation::IPCShutdownState,
+  ProcessChild::AppendToIPCShutdownStateAnnotation(
       "SendFinishShutdown (sending)"_ns);
 
   // Notify the parent that we are done with shutdown. This is sent with high
@@ -3094,8 +3085,7 @@ void ContentChild::ShutdownInternal() {
   // ever process for this ContentChild.
   bool sent = SendFinishShutdown();
 
-  CrashReporter::AppendToCrashReportAnnotation(
-      CrashReporter::Annotation::IPCShutdownState,
+  ProcessChild::AppendToIPCShutdownStateAnnotation(
       sent ? "SendFinishShutdown (sent)"_ns : "SendFinishShutdown (failed)"_ns);
 }
 
@@ -4345,8 +4335,8 @@ mozilla::ipc::IPCResult ContentChild::RecvLoadURI(
       annotationURI = aLoadState->URI();
     }
 
-    CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::URL,
-                                       annotationURI->GetSpecOrDefault());
+    CrashReporter::RecordAnnotationNSCString(CrashReporter::Annotation::URL,
+                                             annotationURI->GetSpecOrDefault());
   }
 #endif
 
@@ -4379,8 +4369,8 @@ mozilla::ipc::IPCResult ContentChild::RecvInternalLoad(
       annotationURI = aLoadState->URI();
     }
 
-    CrashReporter::AnnotateCrashReport(CrashReporter::Annotation::URL,
-                                       annotationURI->GetSpecOrDefault());
+    CrashReporter::RecordAnnotationNSCString(CrashReporter::Annotation::URL,
+                                             annotationURI->GetSpecOrDefault());
   }
 #endif
 

dom/ipc/ContentChild.cpp

@@ -4441,11 +4441,11 @@ void ContentParent::GeneratePairedMinidump(const char* aReason) {
     // minidump tagging along, so we have to tell the crash reporter that
     // it exists and is being appended.
     nsAutoCString additionalDumps("browser");
-    mCrashReporter->AddAnnotation(
+    mCrashReporter->AddAnnotationNSCString(
         CrashReporter::Annotation::additional_minidumps, additionalDumps);
     nsDependentCString reason(aReason);
-    mCrashReporter->AddAnnotation(CrashReporter::Annotation::ipc_channel_error,
-                                  reason);
+    mCrashReporter->AddAnnotationNSCString(
+        CrashReporter::Annotation::ipc_channel_error, reason);
 
     // Generate the report and insert into the queue for submittal.
     if (mCrashReporter->GenerateMinidumpAndPair(this, "browser"_ns)) {

dom/ipc/ContentParent.cpp

@@ -560,12 +560,10 @@ mozilla::ipc::IPCResult HangMonitorChild::RecvRequestContentJSInterrupt() {
   // In order to cancel JS execution on shutdown, we expect that
   // ProcessChild::NotifiedImpendingShutdown has been called before.
   if (mozilla::ipc::ProcessChild::ExpectingShutdown()) {
-    CrashReporter::AppendToCrashReportAnnotation(
-        CrashReporter::Annotation::IPCShutdownState,
+    ProcessChild::AppendToIPCShutdownStateAnnotation(
         "HangMonitorChild::RecvRequestContentJSInterrupt (expected)"_ns);
   } else {
-    CrashReporter::AppendToCrashReportAnnotation(
-        CrashReporter::Annotation::IPCShutdownState,
+    ProcessChild::AppendToIPCShutdownStateAnnotation(
         "HangMonitorChild::RecvRequestContentJSInterrupt (unexpected)"_ns);
   }
   JS_RequestInterruptCallback(mContext);