Bug 1789421 - Generate crash reports for content process shutdown hangs only when we can confirm that the process did not shut down in time r=jstutte

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

dom/ipc/ContentParent.cpp

@@ -4289,7 +4289,8 @@ void ContentParent::GeneratePairedMinidump(const char* aReason) {
 
     // Generate the report and insert into the queue for submittal.
     if (mCrashReporter->GenerateMinidumpAndPair(this, "browser"_ns)) {
-      mCreatedPairedMinidumps = mCrashReporter->FinalizeCrashReport();
+      mCrashReporter->FinalizeCrashReport();
+      mCreatedPairedMinidumps = true;
     }
   }
 }
@@ -4335,6 +4336,7 @@ void ContentParent::KillHard(const char* aReason) {
   }
 
   if (!KillProcess(otherProcessHandle, base::PROCESS_END_KILLED_BY_USER)) {
+    mCrashReporter->DeleteCrashReport();
     NS_WARNING("failed to kill subprocess!");
   }
 

ipc/glue/CrashReporterHost.cpp

@@ -28,7 +28,10 @@ bool CrashReporterHost::GenerateCrashReport(base::ProcessId aPid) {
   if (!TakeCrashedChildMinidump(aPid, nullptr)) {
     return false;
   }
-  return FinalizeCrashReport();
+
+  FinalizeCrashReport();
+  RecordCrash(mProcessType, nsICrashService::CRASH_TYPE_CRASH, mDumpID);
+  return true;
 }
 
 RefPtr<nsIFile> CrashReporterHost::TakeCrashedChildMinidump(
@@ -57,7 +60,7 @@ bool CrashReporterHost::AdoptMinidump(nsIFile* aFile,
   return true;
 }
 
-bool CrashReporterHost::FinalizeCrashReport() {
+void CrashReporterHost::FinalizeCrashReport() {
   MOZ_ASSERT(!mFinalized);
   MOZ_ASSERT(HasMinidump());
 
@@ -70,11 +73,13 @@ bool CrashReporterHost::FinalizeCrashReport() {
       nsDependentCString(startTime);
 
   CrashReporter::WriteExtraFile(mDumpID, mExtraAnnotations);
-
-  RecordCrash(mProcessType, nsICrashService::CRASH_TYPE_CRASH, mDumpID);
-
   mFinalized = true;
-  return true;
+}
+
+void CrashReporterHost::DeleteCrashReport() {
+  if (mFinalized && HasMinidump()) {
+    CrashReporter::DeleteMinidumpFilesForID(mDumpID, Some(u"browser"_ns));
+  }
 }
 
 /* static */

ipc/glue/CrashReporterHost.h

@@ -46,7 +46,10 @@ class CrashReporterHost {
   // If a minidump was already captured (e.g. via the hang reporter), this
   // finalizes the existing report by attaching metadata, writing out the
   // .extra file and notifying the crash service.
-  bool FinalizeCrashReport();
+  void FinalizeCrashReport();
+
+  // Delete any crash report we might have generated.
+  void DeleteCrashReport();
 
   // Generate a paired minidump. This does not take the crash report, as
   // GenerateCrashReport does. After this, FinalizeCrashReport may be called.

toolkit/crashreporter/nsExceptionHandler.cpp

@@ -3042,24 +3042,38 @@ static bool GetMinidumpLimboDir(nsIFile** dir) {
   }
 }
 
-void DeleteMinidumpFilesForID(const nsAString& id) {
+void DeleteMinidumpFilesForID(const nsAString& aId,
+                              const Maybe<nsString>& aAdditionalMinidump) {
   nsCOMPtr<nsIFile> minidumpFile;
-  if (GetMinidumpForID(id, getter_AddRefs(minidumpFile))) {
+  if (GetMinidumpForID(aId, getter_AddRefs(minidumpFile))) {
     minidumpFile->Remove(false);
   }
 
   nsCOMPtr<nsIFile> extraFile;
-  if (GetExtraFileForID(id, getter_AddRefs(extraFile))) {
+  if (GetExtraFileForID(aId, getter_AddRefs(extraFile))) {
     extraFile->Remove(false);
   }
+
+  if (aAdditionalMinidump && GetMinidumpForID(aId, getter_AddRefs(minidumpFile),
+                                              aAdditionalMinidump)) {
+    minidumpFile->Remove(false);
+  }
 }
 
-bool GetMinidumpForID(const nsAString& id, nsIFile** minidump) {
+bool GetMinidumpForID(const nsAString& id, nsIFile** minidump,
+                      const Maybe<nsString>& aAdditionalMinidump) {
   if (!GetMinidumpLimboDir(minidump)) {
     return false;
   }
 
-  (*minidump)->Append(id + u".dmp"_ns);
+  (*minidump)->Append(id);
+
+  if (aAdditionalMinidump) {
+    (*minidump)->Append(u"-"_ns);
+    (*minidump)->Append(*aAdditionalMinidump);
+  }
+
+  (*minidump)->Append(u".dmp"_ns);
 
   bool exists;
   if (NS_FAILED((*minidump)->Exists(&exists)) || !exists) {

toolkit/crashreporter/nsExceptionHandler.h

@@ -14,6 +14,7 @@
 
 #include "mozilla/Assertions.h"
 #include "mozilla/EnumeratedArray.h"
+#include "mozilla/Maybe.h"
 
 #include "CrashAnnotations.h"
 
@@ -43,6 +44,9 @@ class nsIFile;
 
 namespace CrashReporter {
 
+using mozilla::Maybe;
+using mozilla::Nothing;
+
 /**
  * Returns true if the crash reporter is using the dummy implementation.
  */
@@ -141,8 +145,11 @@ void GetAnnotation(uint32_t childPid, Annotation annotation,
 // 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);
+void DeleteMinidumpFilesForID(
+    const nsAString& aId,
+    const Maybe<nsString>& aAdditionalMinidump = Nothing());
+bool GetMinidumpForID(const nsAString& id, nsIFile** minidump,
+                      const Maybe<nsString>& aAdditionalMinidump = Nothing());
 bool GetIDFromMinidump(nsIFile* minidump, nsAString& id);
 bool GetExtraFileForID(const nsAString& id, nsIFile** extraFile);
 bool GetExtraFileForMinidump(nsIFile* minidump, nsIFile** extraFile);