Bug 1284030 - Do not take multiple minidumps during a hang r=jimm

dom/ipc/ProcessHangMonitor.cpp

@@ -162,20 +162,17 @@ public:
     mActor = nullptr;
   }
 
-  /** Sets the information associated with this hang: this includes the ID of
+  /**
-   * the plugin which caused the hang as well as the content PID.
+   * Sets the information associated with this hang: this includes the ID of
+   * the plugin which caused the hang as well as the content PID. The ID of
+   * a minidump taken during the hang can also be provided.
    *
    * @param aHangData The hang information
+   * @param aDumpId The ID of a minidump taken when the hang occurred
    */
-  void SetHangData(const HangData& aHangData) { mHangData = aHangData; }
+  void SetHangData(const HangData& aHangData, const nsAString& aDumpId) {
-
+    mHangData = aHangData;
-  /** Sets the ID of the crash dump associated with this hang. When the ID has
+    mDumpId = aDumpId;
-   * been set then the corresponding crash dump will be used for reporting
-   * instead of generating a new one.
-   *
-   * @param aId The ID of the crash dump taken when the hang was detected. */
-  void SetDumpId(nsString& aId) {
-    mDumpId = aId;
   }
 
   void ClearHang() {
@@ -216,9 +213,17 @@ public:
   void EndStartingDebugger();
   void CleanupPluginHang(uint32_t aPluginId, bool aRemoveFiles);
 
+  /**
+   * Update the dump for the specified plugin. This method is thread-safe and
+   * is used to replace a browser minidump with a full minidump. If aDumpId is
+   * empty this is a no-op.
+   */
+  void UpdateMinidump(uint32_t aPluginId, const nsString& aDumpId);
+
   MessageLoop* MonitorLoop() { return mHangMonitor->MonitorLoop(); }
 
- private:
+private:
+  bool TakeBrowserMinidump(const PluginHangData& aPhd, nsString& aCrashId);
   void ShutdownOnThread();
 
   const RefPtr<ProcessHangMonitor> mHangMonitor;
@@ -550,11 +555,15 @@ class HangObserverNotifier final : public Runnable
 {
 public:
   HangObserverNotifier(HangMonitoredProcess* aProcess,
+                       HangMonitorParent *aParent,
                        const HangData& aHangData,
-                       const nsString& aBrowserDumpId)
+                       const nsString& aBrowserDumpId,
+                       bool aTakeMinidump)
     : mProcess(aProcess),
+      mParent(aParent),
       mHangData(aHangData),
-      mBrowserDumpId(aBrowserDumpId)
+      mBrowserDumpId(aBrowserDumpId),
+      mTakeMinidump(aTakeMinidump)
   {}
 
   NS_IMETHOD
@@ -564,14 +573,19 @@ public:
     MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
     nsString dumpId;
-    if (mHangData.type() == HangData::TPluginHangData) {
+    if ((mHangData.type() == HangData::TPluginHangData) && mTakeMinidump) {
+      // We've been handed a partial minidump; complete it with plugin and
+      // content process dumps.
       const PluginHangData& phd = mHangData.get_PluginHangData();
       plugins::TakeFullMinidump(phd.pluginId(), phd.contentProcessId(),
                                 mBrowserDumpId, dumpId);
+      mParent->UpdateMinidump(phd.pluginId(), dumpId);
+    } else {
+      // We already have a full minidump; go ahead and use it.
+      dumpId = mBrowserDumpId;
     }
 
-    mProcess->SetHangData(mHangData);
+    mProcess->SetHangData(mHangData, dumpId);
-    mProcess->SetDumpId(dumpId);
 
     nsCOMPtr<nsIObserverService> observerService =
       mozilla::services::GetObserverService();
@@ -581,10 +595,40 @@ public:
 
 private:
   RefPtr<HangMonitoredProcess> mProcess;
+  HangMonitorParent* mParent;
   HangData mHangData;
   nsAutoString mBrowserDumpId;
+  bool mTakeMinidump;
 };
 
+// Take a minidump of the browser process if one wasn't already taken for the
+// plugin that caused the hang. Return false if a dump was already available or
+// true if new one has been taken.
+bool
+HangMonitorParent::TakeBrowserMinidump(const PluginHangData& aPhd,
+                                       nsString& aCrashId)
+{
+#ifdef MOZ_CRASHREPORTER
+  MutexAutoLock lock(mBrowserCrashDumpHashLock);
+  if (!mBrowserCrashDumpIds.Get(aPhd.pluginId(), &aCrashId)) {
+    nsCOMPtr<nsIFile> browserDump;
+    if (CrashReporter::TakeMinidump(getter_AddRefs(browserDump), true)) {
+      if (!CrashReporter::GetIDFromMinidump(browserDump, aCrashId)
+          || aCrashId.IsEmpty()) {
+        browserDump->Remove(false);
+        NS_WARNING("Failed to generate timely browser stack, "
+                   "this is bad for plugin hang analysis!");
+      } else {
+        mBrowserCrashDumpIds.Put(aPhd.pluginId(), aCrashId);
+        return true;
+      }
+    }
+  }
+#endif // MOZ_CRASHREPORTER
+
+  return false;
+}
+
 bool
 HangMonitorParent::RecvHangEvidence(const HangData& aHangData)
 {
@@ -606,30 +650,17 @@ HangMonitorParent::RecvHangEvidence(const HangData& aHangData)
   // Before we wake up the browser main thread we want to take a
   // browser minidump.
   nsAutoString crashId;
-#ifdef MOZ_CRASHREPORTER
+  bool takeMinidump = false;
   if (aHangData.type() == HangData::TPluginHangData) {
-    MutexAutoLock lock(mBrowserCrashDumpHashLock);
+    takeMinidump = TakeBrowserMinidump(aHangData.get_PluginHangData(), crashId);
-    const PluginHangData& phd = aHangData.get_PluginHangData();
-    if (!mBrowserCrashDumpIds.Get(phd.pluginId(), &crashId)) {
-      nsCOMPtr<nsIFile> browserDump;
-      if (CrashReporter::TakeMinidump(getter_AddRefs(browserDump), true)) {
-        if (!CrashReporter::GetIDFromMinidump(browserDump, crashId) || crashId.IsEmpty()) {
-          browserDump->Remove(false);
-          NS_WARNING("Failed to generate timely browser stack, this is bad for plugin hang analysis!");
-        } else {
-          mBrowserCrashDumpIds.Put(phd.pluginId(), crashId);
-        }
-      }
-    }
   }
-#endif
 
   mHangMonitor->InitiateCPOWTimeout();
 
   MonitorAutoLock lock(mMonitor);
 
   nsCOMPtr<nsIRunnable> notifier =
-    new HangObserverNotifier(mProcess, aHangData, crashId);
+    new HangObserverNotifier(mProcess, this, aHangData, crashId, takeMinidump);
   NS_DispatchToMainThread(notifier);
 
   return true;
@@ -726,6 +757,17 @@ HangMonitorParent::CleanupPluginHang(uint32_t aPluginId, bool aRemoveFiles)
 #endif
 }
 
+void
+HangMonitorParent::UpdateMinidump(uint32_t aPluginId, const nsString& aDumpId)
+{
+  if (aDumpId.IsEmpty()) {
+    return;
+  }
+
+  MutexAutoLock lock(mBrowserCrashDumpHashLock);
+  mBrowserCrashDumpIds.Put(aPluginId, aDumpId);
+}
+
 /* HangMonitoredProcess implementation */
 
 NS_IMPL_ISUPPORTS(HangMonitoredProcess, nsIHangReport)