Backed out changeset c1dd7376263e (bug 1262852) which caused crashdumps from plugin hangs to be without a signature.
authorGabriele Svelto <gsvelto@mozilla.com>
Sun, 12 Jun 2016 00:25:25 +0200
changeset 377789 60b340e55efe365fb45afd8f5ca7f4b53a73b2dd
parent 377788 a8dac921f07452ec836233b7ccc1f0b194b3d854
child 377790 c0fc5f0d1173eed6936196d3e2202d160fb98f02
child 377801 016e0f47e8ad66ba6eb11fe28958e3a69ef9e53d
push id20857
push userbmo:james@hoppipolla.co.uk
push dateSun, 12 Jun 2016 16:59:39 +0000
bugs1262852
milestone50.0a1
backs outc1dd7376263e4b031a77a7ad3f1cedd0a160c8a6
Backed out changeset c1dd7376263e (bug 1262852) which caused crashdumps from plugin hangs to be without a signature.
dom/ipc/CrashReporterParent.cpp
dom/ipc/CrashReporterParent.h
dom/ipc/PProcessHangMonitor.ipdl
dom/ipc/ProcessHangMonitor.cpp
dom/plugins/ipc/PluginBridge.h
dom/plugins/ipc/PluginModuleParent.cpp
--- a/dom/ipc/CrashReporterParent.cpp
+++ b/dom/ipc/CrashReporterParent.cpp
@@ -107,22 +107,16 @@ CrashReporterParent::GenerateCrashReport
   }
 
   bool result = GenerateChildData(processNotes);
   FinalizeChildData();
   return result;
 }
 
 bool
-CrashReporterParent::UseMinidump(nsIFile* aMinidump)
-{
-  return CrashReporter::GetIDFromMinidump(aMinidump, mChildDumpID);
-}
-
-bool
 CrashReporterParent::GenerateChildData(const AnnotationTable* processNotes)
 {
   MOZ_ASSERT(mInitialized);
 
   if (mChildDumpID.IsEmpty()) {
     NS_WARNING("problem with GenerateChildData: no child dump id yet!");
     return false;
   }
--- a/dom/ipc/CrashReporterParent.h
+++ b/dom/ipc/CrashReporterParent.h
@@ -64,25 +64,16 @@ public:
    * @returns true if successful, false otherwise.
    */
   template<class Toplevel>
   bool
   GenerateMinidumpAndPair(Toplevel* aTopLevel, nsIFile* aMinidump,
                           const nsACString& aPairName);
 
   /**
-   * Uses the specified minidump instead of taking a new one.
-   *
-   * @param aMinidump - the minidump to use for this crashreport.
-   * @returns true if successful, false otherwise.
-   */
-  bool
-  UseMinidump(nsIFile* aMinidump);
-
-  /**
    * Apply child process annotations to an existing paired mindump generated
    * with GeneratePairedMinidump.
    *
    * Be careful about calling generate apis immediately after this call,
    * see FinalizeChildData.
    *
    * @param processNotes (optional) - Additional notes to append. Annotations
    *   stored in mNotes will also be applied. processNotes can be null.
--- a/dom/ipc/PProcessHangMonitor.ipdl
+++ b/dom/ipc/PProcessHangMonitor.ipdl
@@ -16,17 +16,16 @@ struct SlowScriptData
   nsCString filename;
   uint32_t lineno;
 };
 
 struct PluginHangData
 {
   uint32_t pluginId;
   ProcessId contentProcessId;
-  ProcessId pluginProcessId;
 };
 
 union HangData
 {
   SlowScriptData;
   PluginHangData;
 };
 
--- a/dom/ipc/ProcessHangMonitor.cpp
+++ b/dom/ipc/ProcessHangMonitor.cpp
@@ -82,17 +82,17 @@ class HangMonitorChild
                                     unsigned aLineNo);
   void NotifySlowScriptAsync(TabId aTabId,
                              const nsCString& aFileName,
                              unsigned aLineNo);
 
   bool IsDebuggerStartupComplete();
 
   void NotifyPluginHang(uint32_t aPluginId);
-  void NotifyPluginHangAsync(uint32_t aPluginId, ProcessId aPid);
+  void NotifyPluginHangAsync(uint32_t aPluginId);
 
   void ClearHang();
   void ClearHangAsync();
 
   virtual bool RecvTerminateScript() override;
   virtual bool RecvBeginStartingDebugger() override;
   virtual bool RecvEndStartingDebugger() override;
 
@@ -204,18 +204,16 @@ public:
   void BeginStartingDebugger();
   void EndStartingDebugger();
   void CleanupPluginHang(uint32_t aPluginId, bool aRemoveFiles);
 
   MessageLoop* MonitorLoop() { return mHangMonitor->MonitorLoop(); }
 
  private:
   void ShutdownOnThread();
-  void GenerateMinidumps(uint32_t aPluginId, ProcessId aPluginPid,
-                         ProcessId aContentPid, nsString& aCrashId);
 
   const RefPtr<ProcessHangMonitor> mHangMonitor;
 
   // This field is read-only after construction.
   bool mReportHangs;
 
   // This field is only accessed on the hang thread.
   bool mIPCOpen;
@@ -400,34 +398,31 @@ HangMonitorChild::IsDebuggerStartupCompl
 void
 HangMonitorChild::NotifyPluginHang(uint32_t aPluginId)
 {
   // main thread in the child
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
   mSentReport = true;
 
-  base::ProcessId pluginPid = plugins::PluginProcessId(aPluginId);
   // bounce to background thread
-  MonitorLoop()->PostTask(
-    NewNonOwningRunnableMethod<uint32_t, uint32_t>(this,
-                                                   &HangMonitorChild::NotifyPluginHangAsync,
-                                                   aPluginId, pluginPid));
+  MonitorLoop()->PostTask(NewNonOwningRunnableMethod<uint32_t>(this,
+                                                               &HangMonitorChild::NotifyPluginHangAsync,
+                                                               aPluginId));
 }
 
 void
-HangMonitorChild::NotifyPluginHangAsync(uint32_t aPluginId,
-                                        base::ProcessId aPid)
+HangMonitorChild::NotifyPluginHangAsync(uint32_t aPluginId)
 {
   MOZ_RELEASE_ASSERT(MessageLoop::current() == MonitorLoop());
 
   // bounce back to parent on background thread
   if (mIPCOpen) {
     Unused << SendHangEvidence(PluginHangData(aPluginId,
-                                              base::GetCurrentProcId(), aPid));
+                                              base::GetCurrentProcId()));
   }
 }
 
 void
 HangMonitorChild::ClearHang()
 {
   MOZ_ASSERT(NS_IsMainThread());
 
@@ -566,64 +561,16 @@ public:
   }
 
 private:
   RefPtr<HangMonitoredProcess> mProcess;
   HangData mHangData;
   nsAutoString mBrowserDumpId;
 };
 
-void
-HangMonitorParent::GenerateMinidumps(uint32_t aPluginId, ProcessId aPluginPid,
-                                     ProcessId aContentPid, nsString& aCrashId)
-{
-#ifdef MOZ_CRASHREPORTER
-  if (mBrowserCrashDumpIds.Get(aPluginId, &aCrashId)) {
-    return; // We already have a dump for this hang
-  }
-
-  nsCOMPtr<nsIFile> browserDump;
-  if (!CrashReporter::TakeMinidump(getter_AddRefs(browserDump), true)) {
-    NS_WARNING("Failed to generate a minidump for the browser process");
-    return;
-  }
-
-  nsCOMPtr<nsIFile> pluginDump;
-  mozilla::ipc::ScopedProcessHandle pluginHandle;
-  if (!base::OpenPrivilegedProcessHandle(aPluginPid, &pluginHandle.rwget()) ||
-      !CrashReporter::CreateMinidumpsAndPair(pluginHandle, 0,
-                                             NS_LITERAL_CSTRING("browser"),
-                                             browserDump,
-                                             getter_AddRefs(pluginDump))) {
-    browserDump->Remove(false);
-    NS_WARNING("Failed to generate a minidump for the plugin process");
-    return;
-  }
-
-  if (!CrashReporter::GetIDFromMinidump(pluginDump, aCrashId) ||
-      aCrashId.IsEmpty()) {
-    pluginDump->Remove(false);
-    return;
-  }
-
-  mBrowserCrashDumpIds.Put(aPluginId, aCrashId);
-
-  mozilla::ipc::ScopedProcessHandle contentHandle;
-  if (!base::OpenPrivilegedProcessHandle(aContentPid, &contentHandle.rwget()) ||
-      !CrashReporter::CreateAdditionalChildMinidump(contentHandle,
-                                                    0, pluginDump,
-                                                    NS_LITERAL_CSTRING("content"))) {
-    NS_WARNING("Failed to generate a minidump for the content process");
-    return;
-  }
-
-  return;
-#endif
-}
-
 bool
 HangMonitorParent::RecvHangEvidence(const HangData& aHangData)
 {
   // chrome process, background thread
   MOZ_RELEASE_ASSERT(MessageLoop::current() == MonitorLoop());
 
   if (!mReportHangs) {
     return true;
@@ -639,18 +586,27 @@ HangMonitorParent::RecvHangEvidence(cons
 
   // Before we wake up the browser main thread we want to take a
   // browser minidump.
   nsAutoString crashId;
 #ifdef MOZ_CRASHREPORTER
   if (aHangData.type() == HangData::TPluginHangData) {
     MutexAutoLock lock(mBrowserCrashDumpHashLock);
     const PluginHangData& phd = aHangData.get_PluginHangData();
-    GenerateMinidumps(phd.pluginId(), phd.pluginProcessId(),
-                           phd.contentProcessId(), crashId);
+    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 =
--- a/dom/plugins/ipc/PluginBridge.h
+++ b/dom/plugins/ipc/PluginBridge.h
@@ -21,19 +21,16 @@ bool
 SetupBridge(uint32_t aPluginId, dom::ContentParent* aContentParent,
             bool aForceBridgeNow, nsresult* aResult, uint32_t* aRunID);
 
 nsresult
 FindPluginsForContent(uint32_t aPluginEpoch,
                       nsTArray<PluginTag>* aPlugins,
                       uint32_t* aNewPluginEpoch);
 
-base::ProcessId
-PluginProcessId(uint32_t aPluginId);
-
 void
 TerminatePlugin(uint32_t aPluginId,
                 base::ProcessId aContentProcessId,
                 const nsCString& aMonitorDescription,
                 const nsAString& aBrowserDumpId);
 
 } // namespace plugins
 } // namespace mozilla
--- a/dom/plugins/ipc/PluginModuleParent.cpp
+++ b/dom/plugins/ipc/PluginModuleParent.cpp
@@ -348,39 +348,16 @@ private:
 
 PRCList PluginModuleMapping::sModuleListHead =
     PR_INIT_STATIC_CLIST(&PluginModuleMapping::sModuleListHead);
 
 bool PluginModuleMapping::sIsLoadModuleOnStack = false;
 
 } // namespace
 
-base::ProcessId
-mozilla::plugins::PluginProcessId(uint32_t aPluginId)
-{
-  RefPtr<nsPluginHost> host = nsPluginHost::GetInst();
-  if (!host) {
-    return mozilla::ipc::kInvalidProcessId;
-  }
-
-  nsPluginTag* pluginTag = host->PluginWithId(aPluginId);
-  if (!pluginTag || !pluginTag->mPlugin) {
-      return mozilla::ipc::kInvalidProcessId;
-  }
-
-  RefPtr<nsNPAPIPlugin> plugin = pluginTag->mPlugin;
-  PluginModuleChromeParent* chromeParent =
-      static_cast<PluginModuleChromeParent*>(plugin->GetLibrary());
-  if (!chromeParent) {
-    return mozilla::ipc::kInvalidProcessId;
-  }
-
-  return chromeParent->OtherPid();
-}
-
 void
 mozilla::plugins::TerminatePlugin(uint32_t aPluginId,
                                   base::ProcessId aContentProcessId,
                                   const nsCString& aMonitorDescription,
                                   const nsAString& aBrowserDumpId)
 {
     MOZ_ASSERT(XRE_IsParentProcess());
 
@@ -1253,26 +1230,33 @@ PluginModuleChromeParent::TerminateChild
         }
     }
 #endif // XP_WIN
 
     bool reportsReady = false;
 
     // Check to see if we already have a browser dump id - with e10s plugin
     // hangs we take this earlier (see ProcessHangMonitor) from a background
-    // thread. It includes a content and plugin dump too.
+    // thread. We do this before we message the main thread about the hang
+    // since the posted message will trash our browser stack state.
     bool exists;
     nsCOMPtr<nsIFile> browserDumpFile;
     if (!aBrowserDumpId.IsEmpty() &&
         CrashReporter::GetMinidumpForID(aBrowserDumpId, getter_AddRefs(browserDumpFile)) &&
         browserDumpFile &&
         NS_SUCCEEDED(browserDumpFile->Exists(&exists)) && exists)
     {
-        crashReporter->UseMinidump(browserDumpFile);
-        reportsReady = true;
+        // We have a single browser report, generate a new plugin process parent
+        // report and pair it up with the browser report handed in.
+        reportsReady = crashReporter->GenerateMinidumpAndPair(this, browserDumpFile,
+                                                              NS_LITERAL_CSTRING("browser"));
+        if (!reportsReady) {
+          browserDumpFile = nullptr;
+          CrashReporter::DeleteMinidumpFilesForID(aBrowserDumpId);
+        }
     }
 
     // Generate crash report including plugin and browser process minidumps.
     // 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);
@@ -1297,20 +1281,18 @@ PluginModuleChromeParent::TerminateChild
                 additionalDumps.AppendLiteral(",flash1");
             }
             if (CreatePluginMinidump(mFlashProcess2, 0, pluginDumpFile,
                                      NS_LITERAL_CSTRING("flash2"))) {
                 additionalDumps.AppendLiteral(",flash2");
             }
 #endif
             if (aContentPid != mozilla::ipc::kInvalidProcessId) {
-                // Include the content process minidump only if we don't have
-                // it already.
-                if (exists ||
-                    CreatePluginMinidump(aContentPid, 0,
+                // Include the content process minidump
+                if (CreatePluginMinidump(aContentPid, 0,
                                          pluginDumpFile,
                                          NS_LITERAL_CSTRING("content"))) {
                     additionalDumps.AppendLiteral(",content");
                 }
             }
         }
         crashReporter->AnnotateCrashReport(
             NS_LITERAL_CSTRING("additional_minidumps"),