Bug 1547698 - Refactor the code that writes the .extra file for a content process crash or hang r=froydnj
authorGabriele Svelto <gsvelto@mozilla.com>
Sat, 18 May 2019 16:19:55 +0000
changeset 533269 0b3558c3da8582fc84467417af124f5a1a5349da
parent 533268 9497f265bed403af04ae44dc5f5fb16f530dff3d
child 533270 e013f1f17109a8c22cbc7abf6f78db55bd2a8efb
push id11276
push userrgurzau@mozilla.com
push dateMon, 20 May 2019 13:11:24 +0000
treeherdermozilla-beta@847755a7c325 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1547698
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 1547698 - Refactor the code that writes the .extra file for a content process crash or hang r=froydnj Upon a content process crash or hang crash annotations were incrementally written into the .extra file starting with the exception handler callback and then in a number of different places before the file was ready for submission. This had a number of downsides: since the annotations were directly added to the file it was impossible to tell which ones were already written at a certain point in time, additionally some were written twice or even thrice. The code doing the writing would also behave differently depending on the contents of the file, the parameters passed to it and the contents of global variables. This change overhauls the whole process by keeping the annotations into a temporary per-crash annotation table which is filled with all the required annotations before being written out in a single pass when they are ready. The annotations are gathered from the main process annotation table, the per-process one (held by the CrashReporterHost) and exception-time specific ones. The resulting annotations are slightly different than before the patch: first of all there are no more duplicate entries in the .extra file and secondly all content/plugin process hangs annotations are properly filtered, before annotations that were main process-only would leak into them. Differential Revision: https://phabricator.services.mozilla.com/D31069
dom/plugins/ipc/PluginModuleParent.cpp
ipc/glue/CrashReporterHost.cpp
ipc/glue/CrashReporterHost.h
toolkit/crashreporter/nsDummyExceptionHandler.cpp
toolkit/crashreporter/nsExceptionHandler.cpp
toolkit/crashreporter/nsExceptionHandler.h
--- a/dom/plugins/ipc/PluginModuleParent.cpp
+++ b/dom/plugins/ipc/PluginModuleParent.cpp
@@ -1288,43 +1288,44 @@ void PluginModuleChromeParent::ProcessFi
     // process hang monitor has already collected a 3-way browser, plugin,
     // content crash report. If so, update the existing report with our
     // annotations and finalize it. If not, fall through for standard
     // plugin crash report handling.
     mCrashReporter->FinalizeCrashReport();
     return;
   }
 
+  AnnotationTable annotations;
   uint32_t sequence = UINT32_MAX;
   nsAutoCString flashProcessType;
   RefPtr<nsIFile> dumpFile =
       mCrashReporter->TakeCrashedChildMinidump(OtherPid(), &sequence);
 
 #ifdef MOZ_CRASHREPORTER_INJECTOR
   nsCOMPtr<nsIFile> childDumpFile;
   uint32_t childSequence;
 
   if (mFlashProcess1 &&
       TakeMinidumpForChild(mFlashProcess1, getter_AddRefs(childDumpFile),
-                           &childSequence)) {
+                           annotations, &childSequence)) {
     if (childSequence < sequence &&
-        mCrashReporter->AdoptMinidump(childDumpFile)) {
+        mCrashReporter->AdoptMinidump(childDumpFile, annotations)) {
       RemoveMinidump(dumpFile);
       dumpFile = childDumpFile;
       sequence = childSequence;
       flashProcessType.AssignLiteral("Broker");
     } else {
       RemoveMinidump(childDumpFile);
     }
   }
   if (mFlashProcess2 &&
       TakeMinidumpForChild(mFlashProcess2, getter_AddRefs(childDumpFile),
-                           &childSequence)) {
+                           annotations, &childSequence)) {
     if (childSequence < sequence &&
-        mCrashReporter->AdoptMinidump(childDumpFile)) {
+        mCrashReporter->AdoptMinidump(childDumpFile, annotations)) {
       RemoveMinidump(dumpFile);
       dumpFile = childDumpFile;
       sequence = childSequence;
       flashProcessType.AssignLiteral("Sandbox");
     } else {
       RemoveMinidump(childDumpFile);
     }
   }
--- a/ipc/glue/CrashReporterHost.cpp
+++ b/ipc/glue/CrashReporterHost.cpp
@@ -69,43 +69,46 @@ bool CrashReporterHost::GenerateCrashRep
   if (!TakeCrashedChildMinidump(aPid, nullptr)) {
     return false;
   }
   return FinalizeCrashReport();
 }
 
 RefPtr<nsIFile> CrashReporterHost::TakeCrashedChildMinidump(
     base::ProcessId aPid, uint32_t* aOutSequence) {
+  CrashReporter::AnnotationTable annotations;
   MOZ_ASSERT(!HasMinidump());
 
   RefPtr<nsIFile> crashDump;
   if (!CrashReporter::TakeMinidumpForChild(aPid, getter_AddRefs(crashDump),
-                                           aOutSequence)) {
+                                           annotations, aOutSequence)) {
     return nullptr;
   }
-  if (!AdoptMinidump(crashDump)) {
+  if (!AdoptMinidump(crashDump, annotations)) {
     return nullptr;
   }
-  return crashDump.get();
+  return crashDump;
 }
 
-bool CrashReporterHost::AdoptMinidump(nsIFile* aFile) {
-  return CrashReporter::GetIDFromMinidump(aFile, mDumpID);
+bool CrashReporterHost::AdoptMinidump(nsIFile* aFile,
+                                      const AnnotationTable& aAnnotations) {
+  if (!CrashReporter::GetIDFromMinidump(aFile, mDumpID)) {
+    return false;
+  }
+
+  MergeCrashAnnotations(mExtraAnnotations, aAnnotations);
+  return true;
 }
 
-int32_t CrashReporterHost::GetCrashType(
-    const CrashReporter::AnnotationTable& aAnnotations) {
-  // RecordReplayHang is set in the middleman content process, so check
-  // aAnnotations.
-  if (aAnnotations[CrashReporter::Annotation::RecordReplayHang].EqualsLiteral(
-          "1")) {
+int32_t CrashReporterHost::GetCrashType() {
+  if (mExtraAnnotations[CrashReporter::Annotation::RecordReplayHang]
+          .EqualsLiteral("1")) {
     return nsICrashService::CRASH_TYPE_HANG;
   }
 
-  // PluginHang is set in the parent process, so check mExtraAnnotations.
   if (mExtraAnnotations[CrashReporter::Annotation::PluginHang].EqualsLiteral(
           "1")) {
     return nsICrashService::CRASH_TYPE_HANG;
   }
 
   return nsICrashService::CRASH_TYPE_CRASH;
 }
 
@@ -143,20 +146,21 @@ bool CrashReporterHost::FinalizeCrashRep
   SprintfLiteral(startTime, "%lld", static_cast<long long>(mStartTime));
   annotations[CrashReporter::Annotation::StartupTime] =
       nsDependentCString(startTime);
 
   // We might not have shmem (for example, when running crashreporter tests).
   if (mShmem.IsReadable()) {
     CrashReporterMetadataShmem::ReadAppNotes(mShmem, annotations);
   }
-  CrashReporter::AppendExtraData(mDumpID, mExtraAnnotations);
-  CrashReporter::AppendExtraData(mDumpID, annotations);
 
-  int32_t crashType = GetCrashType(annotations);
+  MergeCrashAnnotations(mExtraAnnotations, annotations);
+  CrashReporter::WriteExtraFile(mDumpID, mExtraAnnotations);
+
+  int32_t crashType = GetCrashType();
   NotifyCrashService(mProcessType, crashType, mDumpID);
 
   mFinalized = true;
   return true;
 }
 
 /* static */
 void CrashReporterHost::NotifyCrashService(GeckoProcessType aProcessType,
--- a/ipc/glue/CrashReporterHost.h
+++ b/ipc/glue/CrashReporterHost.h
@@ -38,21 +38,21 @@ class CrashReporterHost {
 
   // Given an existing minidump for a crashed child process, take ownership of
   // it from IPDL. After this, FinalizeCrashReport may be called.
   RefPtr<nsIFile> TakeCrashedChildMinidump(base::ProcessId aPid,
                                            uint32_t* aOutSequence);
 
   // Replace the stored minidump with a new one. After this,
   // FinalizeCrashReport may be called.
-  bool AdoptMinidump(nsIFile* aFile);
+  bool AdoptMinidump(nsIFile* aFile, const AnnotationTable& aAnnotations);
 
   // If a minidump was already captured (e.g. via the hang reporter), this
-  // finalizes the existing report by attaching metadata and notifying the
-  // crash service.
+  // finalizes the existing report by attaching metadata, writing out the
+  // .extra file and notifying the crash service.
   bool FinalizeCrashReport();
 
   // Generate a paired minidump. This does not take the crash report, as
   // GenerateCrashReport does. After this, FinalizeCrashReport may be called.
   //
   // This calls TakeCrashedChildMinidump and FinalizeCrashReport.
   template <typename Toplevel>
   bool GenerateMinidumpAndPair(Toplevel* aToplevelProtocol,
@@ -65,19 +65,19 @@ class CrashReporterHost {
     if (!base::OpenPrivilegedProcessHandle(aToplevelProtocol->OtherPid(),
                                            &childHandle.rwget())) {
       NS_WARNING("Failed to open child process handle.");
       return false;
     }
 #endif
 
     nsCOMPtr<nsIFile> targetDump;
-    if (!CrashReporter::CreateMinidumpsAndPair(childHandle, mThreadId,
-                                               aPairName, aMinidumpToPair,
-                                               getter_AddRefs(targetDump))) {
+    if (!CrashReporter::CreateMinidumpsAndPair(
+            childHandle, mThreadId, aPairName, aMinidumpToPair,
+            mExtraAnnotations, getter_AddRefs(targetDump))) {
       return false;
     }
 
     return CrashReporter::GetIDFromMinidump(targetDump, mDumpID);
   }
 
   void AddAnnotation(CrashReporter::Annotation aKey, bool aValue);
   void AddAnnotation(CrashReporter::Annotation aKey, int aValue);
@@ -90,17 +90,17 @@ class CrashReporterHost {
     return mDumpID;
   }
 
  private:
   static void AsyncAddCrash(int32_t aProcessType, int32_t aCrashType,
                             const nsString& aChildDumpID);
 
   // Get the nsICrashService crash type to use for an impending crash.
-  int32_t GetCrashType(const CrashReporter::AnnotationTable& aAnnotations);
+  int32_t GetCrashType();
 
   // This is a static helper function to notify the crash service that a
   // crash has occurred. This can be called from any thread, and if not
   // called from the main thread, will post a synchronous message to the
   // main thread.
   static void NotifyCrashService(GeckoProcessType aProcessType,
                                  int32_t aCrashType,
                                  const nsString& aChildDumpID);
--- a/toolkit/crashreporter/nsDummyExceptionHandler.cpp
+++ b/toolkit/crashreporter/nsDummyExceptionHandler.cpp
@@ -55,16 +55,19 @@ nsresult AnnotateCrashReport(Annotation 
 nsresult AnnotateCrashReport(Annotation key, const nsACString& data) {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 nsresult RemoveCrashReportAnnotation(Annotation key) {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
+void MergeCrashAnnotations(AnnotationTable& aDst, const AnnotationTable& aSrc) {
+}
+
 nsresult SetGarbageCollecting(bool collecting) {
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 void SetEventloopNestingLevel(uint32_t level) {}
 
 void SetMinidumpAnalysisAllThreads() {}
 
@@ -152,21 +155,17 @@ bool GetIDFromMinidump(nsIFile* minidump
 bool GetExtraFileForID(const nsAString& id, nsIFile** extraFile) {
   return false;
 }
 
 bool GetExtraFileForMinidump(nsIFile* minidump, nsIFile** extraFile) {
   return false;
 }
 
-bool AppendExtraData(const nsAString& id, const AnnotationTable& data) {
-  return false;
-}
-
-bool AppendExtraData(nsIFile* extraFile, const AnnotationTable& data) {
+bool WriteExtraFile(const nsAString& id, const AnnotationTable& annotations) {
   return false;
 }
 
 void OOPInit() {}
 
 #if defined(XP_WIN) || defined(XP_MACOSX)
 const char* GetChildNotificationPipe() { return nullptr; }
 #endif
@@ -196,28 +195,29 @@ bool SetRemoteExceptionHandler(const nsA
 bool CreateNotificationPipeForChild(int* childCrashFd, int* childCrashRemapFd) {
   return false;
 }
 
 bool SetRemoteExceptionHandler() { return false; }
 #endif  // XP_WIN
 
 bool TakeMinidumpForChild(uint32_t childPid, nsIFile** dump,
-                          uint32_t* aSequence) {
+                          AnnotationTable& aAnnotations, uint32_t* aSequence) {
   return false;
 }
 
 ThreadId CurrentThreadId() { return -1; }
 
 bool TakeMinidump(nsIFile** aResult, bool aMoveToPending) { return false; }
 
 bool CreateMinidumpsAndPair(ProcessHandle aTargetPid,
                             ThreadId aTargetBlamedThread,
                             const nsACString& aIncomingPairName,
                             nsIFile* aIncomingDumpToPair,
+                            AnnotationTable& aTargetAnnotations,
                             nsIFile** aTargetDumpOut) {
   return false;
 }
 
 bool CreateAdditionalChildMinidump(ProcessHandle childPid,
                                    ThreadId childBlamedThread,
                                    nsIFile* parentMinidump,
                                    const nsACString& name) {
--- a/toolkit/crashreporter/nsExceptionHandler.cpp
+++ b/toolkit/crashreporter/nsExceptionHandler.cpp
@@ -12,16 +12,17 @@
 #include "nsDirectoryService.h"
 #include "nsDataHashtable.h"
 #include "mozilla/ArrayUtils.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/EnumeratedRange.h"
 #include "mozilla/Services.h"
 #include "nsIObserverService.h"
 #include "mozilla/Unused.h"
+#include "mozilla/UniquePtr.h"
 #include "mozilla/Printf.h"
 #include "mozilla/Sprintf.h"
 #include "mozilla/StaticMutex.h"
 #include "mozilla/SyncRunnable.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/ipc/CrashReporterClient.h"
 
 #include "nsThreadUtils.h"
@@ -272,28 +273,30 @@ static int gMagicChildCrashReportFd =
 static int gChildCrashAnnotationReportFd = -1;
 #endif
 
 // |dumpMapLock| must protect all access to |pidToMinidump|.
 static Mutex* dumpMapLock;
 struct ChildProcessData : public nsUint32HashKey {
   explicit ChildProcessData(KeyTypePointer aKey)
       : nsUint32HashKey(aKey),
-        sequence(0)
+        sequence(0),
+        annotations(nullptr)
 #ifdef MOZ_CRASHREPORTER_INJECTOR
         ,
         callback(nullptr)
 #endif
   {
   }
 
   nsCOMPtr<nsIFile> minidump;
   // Each crashing process is assigned an increasing sequence number to
   // indicate which process crashed first.
   uint32_t sequence;
+  UniquePtr<AnnotationTable> annotations;
 #ifdef MOZ_CRASHREPORTER_INJECTOR
   InjectorCrashCallback* callback;
 #endif
 };
 
 typedef nsTHashtable<ChildProcessData> ChildMinidumpMap;
 static ChildMinidumpMap* pidToMinidump;
 static uint32_t crashSequence;
@@ -524,26 +527,40 @@ bool copy_file(const char* from, const c
 
   sys_close(fdfrom);
   sys_close(fdto);
 
   return ok;
 }
 #endif
 
+/**
+ * The PlatformWriter class provides a tool to create and write to a file that
+ * is safe to call from within an exception handler. To use it this way the
+ * file path needs to be provided as a bare C string. If the writer is created
+ * using an nsIFile instance it will *not* be safe to use from a crashed
+ * context.
+ */
 #ifdef XP_WIN
 
 class PlatformWriter {
  public:
   PlatformWriter() : mHandle(INVALID_HANDLE_VALUE) {}
 
   explicit PlatformWriter(const wchar_t* path) : PlatformWriter() {
     Open(path);
   }
 
+  explicit PlatformWriter(nsIFile* file) : PlatformWriter() {
+    nsAutoString path;
+    if (NS_SUCCEEDED(file->GetPath(path))) {
+      Open(path.get());
+    }
+  }
+
   ~PlatformWriter() {
     if (Valid()) {
       CloseHandle(mHandle);
     }
   }
 
   void Open(const wchar_t* path) {
     mHandle = CreateFile(path, GENERIC_WRITE, 0, nullptr, CREATE_ALWAYS,
@@ -571,16 +588,23 @@ class PlatformWriter {
 #elif defined(XP_UNIX)
 
 class PlatformWriter {
  public:
   PlatformWriter() : mFD(-1) {}
 
   explicit PlatformWriter(const char* path) : PlatformWriter() { Open(path); }
 
+  explicit PlatformWriter(nsIFile* file) : PlatformWriter() {
+    nsAutoCString path;
+    if (NS_SUCCEEDED(file->GetNativePath(path))) {
+      Open(path.get());
+    }
+  }
+
   ~PlatformWriter() {
     if (Valid()) {
       sys_close(mFD);
     }
   }
 
   void Open(const char* path) {
     mFD = sys_open(path, O_WRONLY | O_CREAT | O_TRUNC, 0600);
@@ -616,24 +640,20 @@ static void WriteString(PlatformWriter& 
 #else
   size_t len = strlen(str);
 #endif
 
   pw.WriteBuffer(str, len);
 }
 
 static void WriteAnnotation(PlatformWriter& pw, const Annotation name,
-                            const char* value, size_t len = 0) {
+                            const char* value) {
   WriteString(pw, AnnotationToString(name));
   WriteLiteral(pw, "=");
-  if (len == 0) {
-    WriteString(pw, value);
-  } else {
-    pw.WriteBuffer(value, len);
-  }
+  WriteString(pw, value);
   WriteLiteral(pw, "\n");
 };
 
 /**
  * If minidump_id is null, we assume that dump_path contains the full
  * dump file path.
  */
 static void OpenAPIData(PlatformWriter& aWriter, const XP_CHAR* dump_path,
@@ -809,17 +829,17 @@ static bool LaunchCrashHandlerService(XP
     Unused << HANDLE_EINTR(sys_waitpid(pid, &status, __WALL));
   }
 
   return true;
 }
 
 #endif
 
-void WriteEscapedMozCrashReason(PlatformWriter& aWriter) {
+static void WriteEscapedMozCrashReason(PlatformWriter& aWriter) {
   const char* reason;
   size_t len;
 
   if (gMozCrashReason != nullptr) {
     reason = gMozCrashReason;
     len = strlen(reason);
   } else {
     return;  // No crash reason, bail out
@@ -1239,31 +1259,29 @@ static void ReserveBreakpadVM() {
 }
 
 static void FreeBreakpadVM() {
   if (gBreakpadReservedVM) {
     VirtualFree(gBreakpadReservedVM, 0, MEM_RELEASE);
   }
 }
 
-#  if defined(XP_WIN)
-
 /**
  * Filters out floating point exceptions which are handled by nsSigHandlers.cpp
  * and should not be handled as crashes.
  *
  * Also calls FreeBreakpadVM if appropriate.
  */
 static bool FPEFilter(void* context, EXCEPTION_POINTERS* exinfo,
                       MDRawAssertionInfo* assertion) {
   if (!exinfo) {
     mozilla::IOInterposer::Disable();
-#    if defined(DEBUG) && defined(HAS_DLL_BLOCKLIST)
+#  if defined(DEBUG) && defined(HAS_DLL_BLOCKLIST)
     DllBlocklist_Shutdown();
-#    endif
+#  endif
     FreeBreakpadVM();
     return true;
   }
 
   PEXCEPTION_RECORD e = (PEXCEPTION_RECORD)exinfo->ExceptionRecord;
   switch (e->ExceptionCode) {
     case STATUS_FLOAT_DENORMAL_OPERAND:
     case STATUS_FLOAT_DIVIDE_BY_ZERO:
@@ -1272,34 +1290,32 @@ static bool FPEFilter(void* context, EXC
     case STATUS_FLOAT_OVERFLOW:
     case STATUS_FLOAT_STACK_CHECK:
     case STATUS_FLOAT_UNDERFLOW:
     case STATUS_FLOAT_MULTIPLE_FAULTS:
     case STATUS_FLOAT_MULTIPLE_TRAPS:
       return false;  // Don't write minidump, continue exception search
   }
   mozilla::IOInterposer::Disable();
-#    if defined(DEBUG) && defined(HAS_DLL_BLOCKLIST)
+#  if defined(DEBUG) && defined(HAS_DLL_BLOCKLIST)
   DllBlocklist_Shutdown();
-#    endif
+#  endif
   FreeBreakpadVM();
   return true;
 }
 
 static bool ChildFPEFilter(void* context, EXCEPTION_POINTERS* exinfo,
                            MDRawAssertionInfo* assertion) {
   bool result = FPEFilter(context, exinfo, assertion);
   if (result) {
     PrepareChildExceptionTimeAnnotations(context);
   }
   return result;
 }
 
-#  endif  // defined(XP_WIN)
-
 static MINIDUMP_TYPE GetMinidumpType() {
   MINIDUMP_TYPE minidump_type = static_cast<MINIDUMP_TYPE>(
       MiniDumpWithFullMemoryInfo | MiniDumpWithUnloadedModules);
 
 #  ifdef NIGHTLY_BUILD
   // This is Nightly only because this doubles the size of minidumps based
   // on the experimental data.
   minidump_type =
@@ -2030,17 +2046,17 @@ nsresult AnnotateCrashReport(Annotation 
 
   crashReporterAPIData_Table[key] = escapedData;
 
   // now rebuild the file contents
   crashReporterAPIData->Truncate(0);
   crashEventAPIData->Truncate(0);
   for (auto key : MakeEnumeratedRange(Annotation::Count)) {
     nsDependentCString str(AnnotationToString(key));
-    nsCString entry = crashReporterAPIData_Table[key];
+    const nsCString& entry = crashReporterAPIData_Table[key];
     if (!entry.IsEmpty()) {
       NS_NAMED_LITERAL_CSTRING(kEquals, "=");
       NS_NAMED_LITERAL_CSTRING(kNewline, "\n");
       nsAutoCString line = str + kEquals + entry + kNewline;
 
       crashReporterAPIData->Append(line);
       crashEventAPIData->Append(line);
     }
@@ -2048,16 +2064,56 @@ nsresult AnnotateCrashReport(Annotation 
 
   return NS_OK;
 }
 
 nsresult RemoveCrashReportAnnotation(Annotation key) {
   return AnnotateCrashReport(key, EmptyCString());
 }
 
+void MergeCrashAnnotations(AnnotationTable& aDst, const AnnotationTable& aSrc) {
+  for (auto key : MakeEnumeratedRange(Annotation::Count)) {
+    const nsCString& value = aSrc[key];
+    if (value.IsEmpty()) {
+      continue;
+    }
+
+    aDst[key] = value;
+  }
+}
+
+static void MergeContentCrashAnnotations(AnnotationTable& aDst,
+                                         const AnnotationTable& aSrc) {
+  for (auto key : MakeEnumeratedRange(Annotation::Count)) {
+    const nsCString& value = aSrc[key];
+    if (value.IsEmpty() || IsAnnotationBlacklistedForContent(key)) {
+      continue;
+    }
+
+    aDst[key] = value;
+  }
+}
+
+// Adds crash time, uptime and memory report annotations
+static void AddCommonAnnotations(AnnotationTable& aAnnotations) {
+  nsAutoCString crashTime;
+  crashTime.AppendInt((uint64_t)time(nullptr));
+  aAnnotations[Annotation::CrashTime] = crashTime;
+
+  double uptimeTS = (TimeStamp::NowLoRes() - TimeStamp::ProcessCreation())
+                        .ToSecondsSigDigits();
+  nsAutoCString uptimeStr;
+  uptimeStr.AppendFloat(uptimeTS);
+  aAnnotations[Annotation::UptimeTS] = uptimeStr;
+
+  if (memoryReportPath) {
+    aAnnotations[Annotation::ContainsMemoryReport] = NS_LITERAL_CSTRING("1");
+  }
+}
+
 nsresult SetGarbageCollecting(bool collecting) {
   if (!GetEnabled()) return NS_ERROR_NOT_INITIALIZED;
 
   isGarbageCollecting = collecting;
 
   return NS_OK;
 }
 
@@ -2101,17 +2157,17 @@ nsresult AppendAppNotesToCrashReport(con
   return AnnotateCrashReport(Annotation::Notes, *notesField);
 }
 
 // Returns true if found, false if not found.
 static bool GetAnnotation(CrashReporter::Annotation key, nsACString& data) {
   if (!gExceptionHandler) return false;
 
   MutexAutoLock lock(*crashReporterAPILock);
-  nsCString entry = crashReporterAPIData_Table[key];
+  const nsCString& entry = crashReporterAPIData_Table[key];
   if (entry.IsEmpty()) {
     return false;
   }
 
   data = entry;
   return true;
 }
 
@@ -2651,23 +2707,23 @@ static bool GetMinidumpLimboDir(nsIFile*
 #endif
     return nullptr != *dir;
   }
 }
 
 void DeleteMinidumpFilesForID(const nsAString& id) {
   nsCOMPtr<nsIFile> minidumpFile;
   if (GetMinidumpForID(id, getter_AddRefs(minidumpFile))) {
-    nsCOMPtr<nsIFile> childExtraFile;
-    GetExtraFileForMinidump(minidumpFile, getter_AddRefs(childExtraFile));
-    if (childExtraFile) {
-      childExtraFile->Remove(false);
-    }
     minidumpFile->Remove(false);
   }
+
+  nsCOMPtr<nsIFile> extraFile;
+  if (GetExtraFileForID(id, getter_AddRefs(extraFile))) {
+    extraFile->Remove(false);
+  }
 }
 
 bool GetMinidumpForID(const nsAString& id, nsIFile** minidump) {
   if (!GetMinidumpLimboDir(minidump)) {
     return false;
   }
 
   (*minidump)->Append(id + NS_LITERAL_STRING(".dmp"));
@@ -2716,200 +2772,122 @@ bool GetExtraFileForMinidump(nsIFile* mi
   rv = extraF->SetLeafName(leafName);
   if (NS_FAILED(rv)) return false;
 
   *extraFile = nullptr;
   extraF.swap(*extraFile);
   return true;
 }
 
-bool AppendExtraData(const nsAString& id, const AnnotationTable& data) {
-  nsCOMPtr<nsIFile> extraFile;
-  if (!GetExtraFileForID(id, getter_AddRefs(extraFile))) return false;
-  return AppendExtraData(extraFile, data);
-}
-
-//-----------------------------------------------------------------------------
-// Helpers for AppendExtraData()
-//
-static void WriteAnnotation(PRFileDesc* fd, const Annotation key,
-                            const nsACString& value) {
-  const char* annotation = AnnotationToString(key);
-  PR_Write(fd, annotation, strlen(annotation));
-  PR_Write(fd, "=", 1);
-  PR_Write(fd, value.BeginReading(), value.Length());
-  PR_Write(fd, "\n", 1);
-}
-
-template <int N>
-static void WriteLiteral(PRFileDesc* fd, const char (&str)[N]) {
-  PR_Write(fd, str, N - 1);
-}
-
-/*
- * If accessing the AnnotationTable |data| argument requires locks, the
- * caller should ensure the required locks are already held.
- */
-static bool WriteExtraData(nsIFile* extraFile, const AnnotationTable& data,
-                           bool writeCrashTime = false, bool truncate = false,
-                           bool content = false) {
-  PRFileDesc* fd;
-  int truncOrAppend = truncate ? PR_TRUNCATE : PR_APPEND;
-  nsresult rv = extraFile->OpenNSPRFileDesc(
-      PR_WRONLY | PR_CREATE_FILE | truncOrAppend, 0600, &fd);
-  if (NS_FAILED(rv)) return false;
-
-  for (auto key : MakeEnumeratedRange(Annotation::Count)) {
-    nsCString value = data[key];
-    // Skip entries in the blacklist and empty entries.
-    if ((content && IsAnnotationBlacklistedForContent(key)) ||
-        value.IsEmpty()) {
-      continue;
-    }
-    WriteAnnotation(fd, key, value);
-  }
-
-  if (content && currentSessionId) {
-    WriteAnnotation(fd, Annotation::TelemetrySessionId,
-                    nsDependentCString(currentSessionId));
-  }
-
-  if (writeCrashTime) {
-    time_t crashTime = time(nullptr);
-    char crashTimeString[32];
-    XP_TTOA(crashTime, crashTimeString, 10);
-
-    WriteAnnotation(fd, Annotation::CrashTime,
-                    nsDependentCString(crashTimeString));
-
-    double uptimeTS = (TimeStamp::NowLoRes() - TimeStamp::ProcessCreation())
-                          .ToSecondsSigDigits();
-    char uptimeTSString[64];
-    SimpleNoCLibDtoA(uptimeTS, uptimeTSString, sizeof(uptimeTSString));
-
-    WriteAnnotation(fd, Annotation::UptimeTS,
-                    nsDependentCString(uptimeTSString));
-  }
-
-  if (memoryReportPath) {
-    WriteAnnotation(fd, Annotation::ContainsMemoryReport,
-                    NS_LITERAL_CSTRING("1"));
-  }
-
-  PR_Close(fd);
-  return true;
-}
-
-bool AppendExtraData(nsIFile* extraFile, const AnnotationTable& data) {
-  return WriteExtraData(extraFile, data);
-}
-
-static bool IsDataEscaped(char* aData) {
+static bool IsDataEscaped(const char* aData) {
   if (strchr(aData, '\n')) {
     // There should not be any newlines
     return false;
   }
-  char* pos = aData;
+  const char* pos = aData;
   while ((pos = strchr(pos, '\\'))) {
     if (*(pos + 1) != '\\' && *(pos + 1) != 'n') {
       return false;
     }
     // Add 2 to account for the second pos
     pos += 2;
   }
   return true;
 }
 
 static void ReadAndValidateExceptionTimeAnnotations(
-    FILE*& aFd, AnnotationTable& aAnnotations) {
-  char line[0x1000];
-  while (fgets(line, sizeof(line), aFd)) {
-    char* data = strchr(line, '=');
-    if (!data) {
-      // bad data? Abort!
-      break;
+    PRFileDesc* aFd, AnnotationTable& aAnnotations) {
+  PRInt32 res;
+  do {
+    char c;
+    nsAutoCString annotationString;
+    while ((res = PR_Read(aFd, &c, 1)) > 0) {
+      if (c == '=') {
+        break;
+      }
+      annotationString.Append(c);
     }
-    // Move past the '='
-    *data = 0;
-    ++data;
-    size_t dataLen = strlen(data);
-    // Chop off any trailing newline
-    if (dataLen > 0 && data[dataLen - 1] == '\n') {
-      data[dataLen - 1] = 0;
-      --dataLen;
+
+    nsAutoCString value;
+    while ((res = PR_Read(aFd, &c, 1)) > 0) {
+      if (c == '\n') {
+        break;
+      }
+      value.Append(c);
     }
-    // There should not be any newlines in the key
-    if (strchr(line, '\n')) {
-      break;
-    }
+
     // The annotation sould be known
     Annotation annotation;
-    if (!AnnotationFromString(annotation, line)) {
+    if (!AnnotationFromString(annotation, annotationString.get())) {
       break;
     }
     // Data should have been escaped by the child
-    if (!IsDataEscaped(data)) {
+    if (!IsDataEscaped(value.get())) {
       break;
     }
     // Looks good, save the (line,data) pair
-    aAnnotations[annotation] = nsDependentCString(data, dataLen);
-  }
+    aAnnotations[annotation] = value;
+  } while (res > 0);
 }
 
-/**
- * Writes extra data in the .extra file corresponding to the specified
- * minidump. If `content` is set to true then this assumes that of a child
- * process.
- *
- * NOTE: One side effect of this function is that it deletes the
- * GeckoChildCrash<pid>.extra file if it exists, once processed.
- */
-static bool WriteExtraForMinidump(nsIFile* minidump, uint32_t pid, bool content,
-                                  nsIFile** extraFile) {
-  nsCOMPtr<nsIFile> extra;
-  if (!GetExtraFileForMinidump(minidump, getter_AddRefs(extra))) {
+static bool WriteExtraFile(PlatformWriter pw,
+                           const AnnotationTable& aAnnotations) {
+  if (!pw.Valid()) {
     return false;
   }
 
-  {
-    MutexAutoLock lock(*crashReporterAPILock);
-    if (!WriteExtraData(extra, crashReporterAPIData_Table,
-                        true /*write crash time*/, true /*truncate*/,
-                        content)) {
-      return false;
+  for (auto key : MakeEnumeratedRange(Annotation::Count)) {
+    const nsCString& value = aAnnotations[key];
+    if (!value.IsEmpty()) {
+      WriteAnnotation(pw, key, value.get());
     }
   }
 
+  return true;
+}
+
+bool WriteExtraFile(const nsAString& id, const AnnotationTable& annotations) {
+  nsCOMPtr<nsIFile> extra;
+  if (!GetMinidumpLimboDir(getter_AddRefs(extra))) {
+    return false;
+  }
+
+  extra->Append(id + NS_LITERAL_STRING(".extra"));
+
+  return WriteExtraFile(PlatformWriter(extra), annotations);
+}
+
+static void ReadExceptionTimeAnnotations(AnnotationTable& aAnnotations,
+                                         uint32_t aPid) {
+  // Read exception-time annotations
   StaticMutexAutoLock pidMapLock(processMapLock);
-  if (pid && processToCrashFd.count(pid)) {
-    PRFileDesc* prFd = processToCrashFd[pid];
-    processToCrashFd.erase(pid);
-    FILE* fd;
-#if defined(XP_WIN)
-    int nativeFd = _open_osfhandle(PR_FileDesc2NativeHandle(prFd), 0);
-    if (nativeFd == -1) {
-      return false;
-    }
-    fd = fdopen(nativeFd, "r");
-#else
-    fd = fdopen(PR_FileDesc2NativeHandle(prFd), "r");
-#endif
-    if (fd) {
-      AnnotationTable exceptionTimeAnnotations;
-      ReadAndValidateExceptionTimeAnnotations(fd, exceptionTimeAnnotations);
-      PR_Close(prFd);
-      if (!AppendExtraData(extra, exceptionTimeAnnotations)) {
-        return false;
-      }
-    }
+  if (aPid && processToCrashFd.count(aPid)) {
+    PRFileDesc* prFd = processToCrashFd[aPid];
+    processToCrashFd.erase(aPid);
+    AnnotationTable exceptionTimeAnnotations;
+    ReadAndValidateExceptionTimeAnnotations(prFd, exceptionTimeAnnotations);
+    MergeCrashAnnotations(aAnnotations, exceptionTimeAnnotations);
+    PR_Close(prFd);
   }
-
-  extra.forget(extraFile);
-
-  return true;
+}
+
+static void PopulateContentProcessAnnotations(AnnotationTable& aAnnotations,
+                                              uint32_t aPid) {
+  {
+    MutexAutoLock lock(*crashReporterAPILock);
+    MergeContentCrashAnnotations(aAnnotations, crashReporterAPIData_Table);
+  }
+
+  if (currentSessionId) {
+    aAnnotations[Annotation::TelemetrySessionId] =
+        nsDependentCString(currentSessionId);
+  }
+
+  AddCommonAnnotations(aAnnotations);
+  ReadExceptionTimeAnnotations(aAnnotations, aPid);
 }
 
 // It really only makes sense to call this function when
 // ShouldReport() is true.
 // Uses dumpFile's filename to generate memoryReport's filename (same name with
 // a different extension)
 static bool MoveToPending(nsIFile* dumpFile, nsIFile* extraFile,
                           nsIFile* memoryReport) {
@@ -2941,52 +2919,48 @@ static bool MoveToPending(nsIFile* dumpF
 
   return true;
 }
 
 static void OnChildProcessDumpRequested(void* aContext,
                                         const ClientInfo& aClientInfo,
                                         const xpstring& aFilePath) {
   nsCOMPtr<nsIFile> minidump;
-  nsCOMPtr<nsIFile> extraFile;
 
   // Hold the mutex until the current dump request is complete, to
   // prevent UnsetExceptionHandler() from pulling the rug out from
   // under us.
   MutexAutoLock lock(*dumpSafetyLock);
   if (!isSafeToDump) return;
 
   CreateFileFromPath(aFilePath, getter_AddRefs(minidump));
 
   uint32_t pid = aClientInfo.pid();
 
-  if (!WriteExtraForMinidump(minidump, pid, /* content */ true,
-                             getter_AddRefs(extraFile))) {
-    return;
-  }
-
   if (ShouldReport()) {
     nsCOMPtr<nsIFile> memoryReport;
     if (memoryReportPath) {
       CreateFileFromPath(memoryReportPath, getter_AddRefs(memoryReport));
       MOZ_ASSERT(memoryReport);
     }
-    MoveToPending(minidump, extraFile, memoryReport);
+    MoveToPending(minidump, nullptr, memoryReport);
   }
 
   {
 #ifdef MOZ_CRASHREPORTER_INJECTOR
     bool runCallback;
 #endif
     {
       MutexAutoLock lock(*dumpMapLock);
       ChildProcessData* pd = pidToMinidump->PutEntry(pid);
       MOZ_ASSERT(!pd->minidump);
       pd->minidump = minidump;
       pd->sequence = ++crashSequence;
+      pd->annotations = MakeUnique<AnnotationTable>();
+      PopulateContentProcessAnnotations(*(pd->annotations), pid);
 #ifdef MOZ_CRASHREPORTER_INJECTOR
       runCallback = nullptr != pd->callback;
 #endif
     }
 #ifdef MOZ_CRASHREPORTER_INJECTOR
     if (runCallback) NS_DispatchToMainThread(new ReportInjectedCrash(pid));
 #endif
   }
@@ -3304,25 +3278,26 @@ bool SetRemoteExceptionHandler(const nsA
   oldTerminateHandler = std::set_terminate(&TerminateHandler);
 
   // we either do remote or nothing, no fallback to regular crash reporting
   return gExceptionHandler->IsOutOfProcess();
 }
 #endif  // XP_WIN
 
 bool TakeMinidumpForChild(uint32_t childPid, nsIFile** dump,
-                          uint32_t* aSequence) {
+                          AnnotationTable& aAnnotations, uint32_t* aSequence) {
   if (!GetEnabled()) return false;
 
   MutexAutoLock lock(*dumpMapLock);
 
   ChildProcessData* pd = pidToMinidump->GetEntry(childPid);
   if (!pd) return false;
 
   NS_IF_ADDREF(*dump = pd->minidump);
+  aAnnotations = *(pd->annotations);
   if (aSequence) {
     *aSequence = pd->sequence;
   }
 
   pidToMinidump->RemoveEntry(pd);
 
   return !!*dump;
 }
@@ -3354,71 +3329,42 @@ static void RenameAdditionalHangMinidump
   // turn "<id>.dmp" into "<id>-<name>.dmp
   leafName.Insert(NS_LITERAL_CSTRING("-") + name, leafName.Length() - 4);
 
   if (NS_FAILED(minidump->MoveToNative(directory, leafName))) {
     NS_WARNING("RenameAdditionalHangMinidump failed to move minidump.");
   }
 }
 
+// Stores the minidump in the nsIFile pointed by the |context| parameter.
 static bool PairedDumpCallback(
 #ifdef XP_LINUX
     const MinidumpDescriptor& descriptor,
 #else
     const XP_CHAR* dump_path, const XP_CHAR* minidump_id,
 #endif
     void* context,
 #ifdef XP_WIN
     EXCEPTION_POINTERS* /*unused*/, MDRawAssertionInfo* /*unused*/,
 #endif
     bool succeeded) {
   nsCOMPtr<nsIFile>& minidump = *static_cast<nsCOMPtr<nsIFile>*>(context);
 
-  xpstring dump;
+  xpstring path;
 #ifdef XP_LINUX
-  dump = descriptor.path();
+  path = descriptor.path();
 #else
-  dump = dump_path;
-  dump += XP_PATH_SEPARATOR;
-  dump += minidump_id;
-  dump += dumpFileExtension;
-#endif
-
-  CreateFileFromPath(dump, getter_AddRefs(minidump));
-  return true;
-}
-
-static bool PairedDumpCallbackExtra(
-#ifdef XP_LINUX
-    const MinidumpDescriptor& descriptor,
-#else
-    const XP_CHAR* dump_path, const XP_CHAR* minidump_id,
+  path = dump_path;
+  path += XP_PATH_SEPARATOR;
+  path += minidump_id;
+  path += dumpFileExtension;
 #endif
-    void* context,
-#ifdef XP_WIN
-    EXCEPTION_POINTERS* /*unused*/, MDRawAssertionInfo* /*unused*/,
-#endif
-    bool succeeded) {
-  PairedDumpCallback(
-#ifdef XP_LINUX
-      descriptor,
-#else
-      dump_path, minidump_id,
-#endif
-      context,
-#ifdef XP_WIN
-      nullptr, nullptr,
-#endif
-      succeeded);
-
-  nsCOMPtr<nsIFile>& minidump = *static_cast<nsCOMPtr<nsIFile>*>(context);
-
-  nsCOMPtr<nsIFile> extra;
-  return WriteExtraForMinidump(minidump, 0, /* content */ false,
-                               getter_AddRefs(extra));
+
+  CreateFileFromPath(path, getter_AddRefs(minidump));
+  return true;
 }
 
 ThreadId CurrentThreadId() {
 #if defined(XP_WIN)
   return ::GetCurrentThreadId();
 #elif defined(XP_LINUX)
   return sys_gettid();
 #elif defined(XP_MACOSX)
@@ -3491,16 +3437,17 @@ bool TakeMinidump(nsIFile** aResult, boo
   }
   return true;
 }
 
 bool CreateMinidumpsAndPair(ProcessHandle aTargetPid,
                             ThreadId aTargetBlamedThread,
                             const nsACString& aIncomingPairName,
                             nsIFile* aIncomingDumpToPair,
+                            AnnotationTable& aTargetAnnotations,
                             nsIFile** aMainDumpOut) {
   if (!GetEnabled()) {
     return false;
   }
 
   AutoIOInterposerDisable disableIOInterposition;
 
 #ifdef XP_MACOSX
@@ -3514,61 +3461,65 @@ bool CreateMinidumpsAndPair(ProcessHandl
   dump_path = gExceptionHandler->dump_path();
 #else
   dump_path = gExceptionHandler->minidump_descriptor().directory();
 #endif
 
   // dump the target
   nsCOMPtr<nsIFile> targetMinidump;
   if (!google_breakpad::ExceptionHandler::WriteMinidumpForChild(
-          aTargetPid, targetThread, dump_path, PairedDumpCallbackExtra,
+          aTargetPid, targetThread, dump_path, PairedDumpCallback,
           static_cast<void*>(&targetMinidump)
 #ifdef XP_WIN
               ,
           GetMinidumpType()
 #endif
               )) {
     return false;
   }
 
-  nsCOMPtr<nsIFile> targetExtra;
-  GetExtraFileForMinidump(targetMinidump, getter_AddRefs(targetExtra));
-
   // If aIncomingDumpToPair isn't valid, create a dump of this process.
   nsCOMPtr<nsIFile> incomingDump;
   if (aIncomingDumpToPair == nullptr) {
     if (!google_breakpad::ExceptionHandler::WriteMinidump(
             dump_path,
 #ifdef XP_MACOSX
             true,
 #endif
             PairedDumpCallback, static_cast<void*>(&incomingDump)
 #ifdef XP_WIN
                                     ,
             GetMinidumpType()
 #endif
                 )) {
       targetMinidump->Remove(false);
-      targetExtra->Remove(false);
       return false;
     }
   } else {
     incomingDump = aIncomingDumpToPair;
   }
 
   RenameAdditionalHangMinidump(incomingDump, targetMinidump, aIncomingPairName);
 
   if (ShouldReport()) {
-    MoveToPending(targetMinidump, targetExtra, nullptr);
+    MoveToPending(targetMinidump, nullptr, nullptr);
     MoveToPending(incomingDump, nullptr, nullptr);
   }
 #if defined(DEBUG) && defined(HAS_DLL_BLOCKLIST)
   DllBlocklist_Shutdown();
 #endif
 
+  {
+    MutexAutoLock lock(*crashReporterAPILock);
+    MergeContentCrashAnnotations(aTargetAnnotations,
+                                 crashReporterAPIData_Table);
+  }
+
+  AddCommonAnnotations(aTargetAnnotations);
+
   targetMinidump.forget(aMainDumpOut);
 
   return true;
 }
 
 bool CreateAdditionalChildMinidump(ProcessHandle childPid,
                                    ThreadId childBlamedThread,
                                    nsIFile* parentMinidump,
--- a/toolkit/crashreporter/nsExceptionHandler.h
+++ b/toolkit/crashreporter/nsExceptionHandler.h
@@ -114,24 +114,28 @@ nsresult RegisterAppMemory(void* ptr, si
 nsresult UnregisterAppMemory(void* ptr);
 
 // Include heap regions of the crash context.
 void SetIncludeContextHeap(bool aValue);
 
 // Functions for working with minidumps and .extras
 typedef mozilla::EnumeratedArray<Annotation, Annotation::Count, nsCString>
     AnnotationTable;
-
 void DeleteMinidumpFilesForID(const nsAString& id);
 bool GetMinidumpForID(const nsAString& id, nsIFile** minidump);
 bool GetIDFromMinidump(nsIFile* minidump, nsAString& id);
 bool GetExtraFileForID(const nsAString& id, nsIFile** extraFile);
 bool GetExtraFileForMinidump(nsIFile* minidump, nsIFile** extraFile);
-bool AppendExtraData(const nsAString& id, const AnnotationTable& data);
-bool AppendExtraData(nsIFile* extraFile, const AnnotationTable& data);
+bool WriteExtraFile(const nsAString& id, const AnnotationTable& annotations);
+
+/**
+ * Copies the non-empty annotations in the source table to the destination
+ * overwriting the corresponding entries.
+ */
+void MergeCrashAnnotations(AnnotationTable& aDst, const AnnotationTable& aSrc);
 
 #ifdef XP_WIN
 nsresult WriteMinidumpForException(EXCEPTION_POINTERS* aExceptionInfo);
 #endif
 #ifdef XP_LINUX
 bool WriteMinidumpForSigInfo(int signo, siginfo_t* info, void* uc);
 #endif
 #ifdef XP_MACOSX
@@ -156,19 +160,21 @@ void OOPInit();
  * @param aMoveToPending - if true move the report to the report
  *   pending directory.
  * @returns boolean indicating success or failure.
  */
 bool TakeMinidump(nsIFile** aResult, bool aMoveToPending = false);
 
 // Return true if a dump was found for |childPid|, and return the
 // path in |dump|.  The caller owns the last reference to |dump| if it
-// is non-nullptr. The sequence parameter will be filled with an ordinal
+// is non-nullptr. The annotations for the crash will be stored in
+// |aAnnotations|. The sequence parameter will be filled with an ordinal
 // indicating which remote process crashed first.
 bool TakeMinidumpForChild(uint32_t childPid, nsIFile** dump,
+                          AnnotationTable& aAnnotations,
                           uint32_t* aSequence = nullptr);
 
 #if defined(XP_WIN)
 typedef HANDLE ProcessHandle;
 typedef DWORD ProcessId;
 typedef DWORD ThreadId;
 typedef HANDLE FileHandle;
 #elif defined(XP_MACOSX)
@@ -211,22 +217,24 @@ ThreadId CurrentThreadId();
  * @param aTargetBlamedThread The target thread for the minidump.
  * @param aIncomingPairName The name to apply to the paired dump the caller
  *   passes in.
  * @param aIncomingDumpToPair Existing dump to pair with the new dump. if this
  *   is null, TakeMinidumpAndPair will take a new minidump of the calling
  *   process and thread and use it in aIncomingDumpToPairs place.
  * @param aTargetDumpOut The target minidump file paired up with
  *   aIncomingDumpToPair.
+ * @param aTargetAnnotations The crash annotations of the target process.
  * @return bool indicating success or failure
  */
 bool CreateMinidumpsAndPair(ProcessHandle aTargetPid,
                             ThreadId aTargetBlamedThread,
                             const nsACString& aIncomingPairName,
                             nsIFile* aIncomingDumpToPair,
+                            AnnotationTable& aTargetAnnotations,
                             nsIFile** aTargetDumpOut);
 
 // Create an additional minidump for a child of a process which already has
 // a minidump (|parentMinidump|).
 // The resulting dump will get the id of the parent and use the |name| as
 // an extension.
 bool CreateAdditionalChildMinidump(ProcessHandle childPid,
                                    ThreadId childBlamedThread,