Bug 1227312 - Avoid calling FinalizeChildData twice in GenerateCompleteMinidump. r=ted, a=sledru
authorJim Mathies <jmathies@mozilla.com>
Thu, 03 Dec 2015 10:25:10 -0600
changeset 310786 60681827670839a14e3700482e7275d87bc3ccb1
parent 310785 044d5043b11dcdd19049b9f9a378b37f1dd6e140
child 310787 e4e34060f38271a3c47d5b49b29ac437fb8e0ad6
push id5513
push userraliiev@mozilla.com
push dateMon, 25 Jan 2016 13:55:34 +0000
treeherdermozilla-beta@5ee97dd05b5c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersted, sledru
bugs1227312
milestone45.0a2
Bug 1227312 - Avoid calling FinalizeChildData twice in GenerateCompleteMinidump. r=ted, a=sledru
dom/ipc/CrashReporterParent.cpp
dom/ipc/CrashReporterParent.h
dom/plugins/ipc/PluginModuleParent.cpp
--- a/dom/ipc/CrashReporterParent.cpp
+++ b/dom/ipc/CrashReporterParent.cpp
@@ -98,17 +98,19 @@ CrashReporterParent::SetChildData(const 
 
 #ifdef MOZ_CRASHREPORTER
 bool
 CrashReporterParent::GenerateCrashReportForMinidump(nsIFile* minidump,
     const AnnotationTable* processNotes)
 {
     if (!CrashReporter::GetIDFromMinidump(minidump, mChildDumpID))
         return false;
-    return GenerateChildData(processNotes);
+    bool result = GenerateChildData(processNotes);
+    FinalizeChildData();
+    return result;
 }
 
 bool
 CrashReporterParent::GenerateChildData(const AnnotationTable* processNotes)
 {
     MOZ_ASSERT(mInitialized);
 
     if (mChildDumpID.IsEmpty()) {
@@ -144,18 +146,16 @@ CrashReporterParent::GenerateChildData(c
     bool ret = CrashReporter::AppendExtraData(mChildDumpID, mNotes);
     if (ret && processNotes) {
         ret = CrashReporter::AppendExtraData(mChildDumpID, *processNotes);
     }
 
     if (!ret) {
         NS_WARNING("problem appending child data to .extra");
     }
-
-    FinalizeChildData();
     return ret;
 }
 
 void
 CrashReporterParent::FinalizeChildData()
 {
     MOZ_ASSERT(mInitialized);
 
--- a/dom/ipc/CrashReporterParent.h
+++ b/dom/ipc/CrashReporterParent.h
@@ -27,31 +27,32 @@ class CrashReporterParent :
 public:
   CrashReporterParent();
   virtual ~CrashReporterParent();
 
 #ifdef MOZ_CRASHREPORTER
 
   /*
    * 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.
    */
   template<class Toplevel>
   bool
   GenerateCrashReport(Toplevel* t, const AnnotationTable* processNotes);
 
   /*
    * Attempt to generate a parent/child pair of minidumps from the given
    * toplevel actor. This calls CrashReporter::CreateMinidumpsAndPair to
    * 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.
    */
   template<class Toplevel>
   bool
   GeneratePairedMinidump(Toplevel* t);
 
   /*
@@ -102,18 +103,18 @@ public:
    *
    * @returns true if successful, false otherwise.
    */
   template<class Toplevel>
   bool
   GenerateCompleteMinidump(Toplevel* t);
 
   /**
-   * Submits a raw minidump handed in, calls GenerateChildData. Used
-   * by plugins.
+   * Submits a raw minidump handed in, calls GenerateChildData and
+   * FinalizeChildData. Used by content plugins and gmp.
    *
    * @returns true if successful, false otherwise.
    */
   bool
   GenerateCrashReportForMinidump(nsIFile* minidump,
                                  const AnnotationTable* processNotes);
 
   /*
@@ -236,17 +237,19 @@ CrashReporterParent::GenerateMinidumpAnd
 template<class Toplevel>
 inline bool
 CrashReporterParent::GenerateCrashReport(Toplevel* t,
                                          const AnnotationTable* processNotes)
 {
   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;
 }
 
 template<class Toplevel>
 inline bool
 CrashReporterParent::GenerateCompleteMinidump(Toplevel* t)
 {
@@ -266,19 +269,19 @@ CrashReporterParent::GenerateCompleteMin
 #endif
   nsCOMPtr<nsIFile> childDump;
   if (CrashReporter::CreateMinidumpsAndPair(child,
                                             mMainThread,
                                             NS_LITERAL_CSTRING("browser"),
                                             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;
 }
 
 template<class Toplevel>
 /* static */ bool
 CrashReporterParent::CreateCrashReporter(Toplevel* actor)
 {
--- a/dom/plugins/ipc/PluginModuleParent.cpp
+++ b/dom/plugins/ipc/PluginModuleParent.cpp
@@ -1286,16 +1286,18 @@ PluginModuleChromeParent::TerminateChild
     // The plugin process is the parent report with additional dumps including
     // the browser process, content process when running under e10s, and
     // various flash subprocesses if we're the flash module.
     if (!reportsReady) {
         reportsReady = crashReporter->GeneratePairedMinidump(this);
     }
 
     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)",
                  NS_ConvertUTF16toUTF8(mPluginDumpID).get()));
         nsAutoCString additionalDumps("browser");
         nsCOMPtr<nsIFile> pluginDumpFile;
         if (GetMinidumpForID(mPluginDumpID, getter_AddRefs(pluginDumpFile)) &&
             pluginDumpFile) {
@@ -1532,17 +1534,23 @@ PluginModuleChromeParent::ProcessFirstMi
     CrashReporterParent* crashReporter = CrashReporter();
     if (!crashReporter)
         return;
 
     AnnotationTable notes(4);
     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;
     }
 
     uint32_t sequence = UINT32_MAX;
     nsCOMPtr<nsIFile> dumpFile;
     nsAutoCString flashProcessType;
     TakeMinidump(getter_AddRefs(dumpFile), &sequence);