Bug 1227312 - Avoid calling FinalizeChildData twice in GenerateCompleteMinidump. r=ted

dom/ipc/CrashReporterParent.cpp

@@ -103,7 +103,9 @@ CrashReporterParent::GenerateCrashReportForMinidump(nsIFile* minidump,
 {
     if (!CrashReporter::GetIDFromMinidump(minidump, mChildDumpID))
         return false;
-    return GenerateChildData(processNotes);
+    bool result = GenerateChildData(processNotes);
+    FinalizeChildData();
+    return result;
 }
 
 bool
@@ -149,8 +151,6 @@ CrashReporterParent::GenerateChildData(const AnnotationTable* processNotes)
     if (!ret) {
         NS_WARNING("problem appending child data to .extra");
     }
-
-    FinalizeChildData();
     return ret;
 }
 

dom/ipc/CrashReporterParent.h

@@ -32,7 +32,8 @@ public:
 
   /*
    * Attempt to create a bare-bones crash report, along with extra process-
-   * specific annotations present in the given AnnotationTable.
+   * specific annotations present in the given AnnotationTable. Calls
+   * GenerateChildData and FinalizeChildData.
    *
    * @returns true if successful, false otherwise.
    */
@@ -46,7 +47,7 @@ public:
    * generate the minidumps. Crash reporter annotations set prior to this
    * call will be saved via PairedDumpCallbackExtra into an .extra file
    * under the proper crash id. AnnotateCrashReport annotations are not
-   * set in this call, use GenerateChildData.
+   * set in this call and the report is not finalized.
    *
    * @returns true if successful, false otherwise.
    */
@@ -107,8 +108,8 @@ public:
   GenerateCompleteMinidump(Toplevel* t);
 
   /**
-   * Submits a raw minidump handed in, calls GenerateChildData. Used
+   * Submits a raw minidump handed in, calls GenerateChildData and
-   * by plugins.
+   * FinalizeChildData. Used by content plugins and gmp.
    *
    * @returns true if successful, false otherwise.
    */
@@ -241,7 +242,9 @@ CrashReporterParent::GenerateCrashReport(Toplevel* t,
   nsCOMPtr<nsIFile> crashDump;
   if (t->TakeMinidump(getter_AddRefs(crashDump), nullptr) &&
       CrashReporter::GetIDFromMinidump(crashDump, mChildDumpID)) {
-    return GenerateChildData(processNotes);
+    bool result = GenerateChildData(processNotes);
+    FinalizeChildData();
+    return result;
   }
   return false;
 }
@@ -271,9 +274,9 @@ CrashReporterParent::GenerateCompleteMinidump(Toplevel* t)
                                             nullptr, // pair with a dump of this process and thread
                                             getter_AddRefs(childDump)) &&
       CrashReporter::GetIDFromMinidump(childDump, mChildDumpID)) {
-    GenerateChildData(nullptr);
+    bool result = GenerateChildData(nullptr);
     FinalizeChildData();
-    return true;
+    return result;
   }
   return false;
 }

dom/plugins/ipc/PluginModuleParent.cpp

@@ -1291,6 +1291,8 @@ PluginModuleChromeParent::TerminateChildProcess(MessageLoop* aMsgLoop,
     }
 
     if (reportsReady) {
+        // Important to set this here, it tells the ActorDestroy handler
+        // that we have an existing crash report that needs to be finalized.
         mPluginDumpID = crashReporter->ChildDumpID();
         PLUGIN_LOG_DEBUG(
                 ("generated paired browser/plugin minidumps: %s)",
@@ -1537,7 +1539,13 @@ PluginModuleChromeParent::ProcessFirstMinidump()
     WriteExtraDataForMinidump(notes);
 
     if (!mPluginDumpID.IsEmpty()) {
+        // mPluginDumpID may be set in TerminateChildProcess, which means the
+        // 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.
         crashReporter->GenerateChildData(&notes);
+        crashReporter->FinalizeChildData();
         return;
     }