Bug 1175975 - Null crash fix in ProcessHangMonitor (r=jimm)
authorBill McCloskey <billm@mozilla.com>
Thu, 18 Jun 2015 13:10:46 -0700
changeset 280628 06bac3891a828a9a2f81f964e7ca73eef46b9861
parent 280627 5b56c3d0e3796fcd86cb1fd48d95208abba93353
child 280629 81c5975ba2b429785a72b2df9e0c0c74e9b84fa7
push id4932
push userjlund@mozilla.com
push dateMon, 10 Aug 2015 18:23:06 +0000
treeherdermozilla-beta@6dd5a4f5f745 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjimm
bugs1175975
milestone41.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1175975 - Null crash fix in ProcessHangMonitor (r=jimm)
dom/ipc/ProcessHangMonitor.cpp
dom/plugins/ipc/PluginHangUIParent.cpp
dom/plugins/ipc/PluginModuleParent.cpp
dom/plugins/ipc/PluginModuleParent.h
--- a/dom/ipc/ProcessHangMonitor.cpp
+++ b/dom/ipc/ProcessHangMonitor.cpp
@@ -151,17 +151,17 @@ public:
   NS_IMETHOD BeginStartingDebugger() override;
   NS_IMETHOD EndStartingDebugger() override;
   NS_IMETHOD TerminatePlugin() override;
   NS_IMETHOD TerminateProcess() override;
   NS_IMETHOD UserCanceled() override;
 
   NS_IMETHOD IsReportForBrowser(nsIFrameLoader* aFrameLoader, bool* aResult) override;
 
-  // Called on xpcom shutdown
+  // Called when a content process shuts down.
   void Clear() {
     mContentParent = nullptr;
     mActor = nullptr;
   }
 
   void SetHangData(const HangData& aHangData) { mHangData = aHangData; }
   void SetBrowserDumpId(nsAutoString& aId) {
     mBrowserDumpId = aId;
@@ -451,21 +451,35 @@ HangMonitorParent::HangMonitorParent(Pro
    mMonitor("HangMonitorParent lock"),
    mShutdownDone(false),
    mBrowserCrashDumpHashLock("mBrowserCrashDumpIds lock")
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
   mReportHangs = mozilla::Preferences::GetBool("dom.ipc.reportProcessHangs", false);
 }
 
+static PLDHashOperator
+DeleteMinidump(const uint32_t& aPluginId, nsString aCrashId, void* aUserData)
+{
+#ifdef MOZ_CRASHREPORTER
+  if (!aCrashId.IsEmpty()) {
+    CrashReporter::DeleteMinidumpFilesForID(aCrashId);
+  }
+#endif
+  return PL_DHASH_NEXT;
+}
+
 HangMonitorParent::~HangMonitorParent()
 {
   // For some reason IPDL doesn't autmatically delete the channel for a
   // bridged protocol (bug 1090570). So we have to do it ourselves.
   XRE_GetIOMessageLoop()->PostTask(FROM_HERE, new DeleteTask<Transport>(GetTransport()));
+
+  MutexAutoLock lock(mBrowserCrashDumpHashLock);
+  mBrowserCrashDumpIds.EnumerateRead(DeleteMinidump, nullptr);
 }
 
 void
 HangMonitorParent::Shutdown()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
   MonitorAutoLock lock(mMonitor);
@@ -800,30 +814,32 @@ HangMonitoredProcess::TerminatePlugin()
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
   if (mHangData.type() != HangData::TPluginHangData) {
     return NS_ERROR_UNEXPECTED;
   }
 
   uint32_t id = mHangData.get_PluginHangData().pluginId();
   plugins::TerminatePlugin(id, mBrowserDumpId);
 
-  mActor->CleanupPluginHang(id, false);
+  if (mActor) {
+    mActor->CleanupPluginHang(id, false);
+  }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 HangMonitoredProcess::TerminateProcess()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
 
   if (!mContentParent) {
     return NS_ERROR_UNEXPECTED;
   }
 
-  if (mHangData.type() == HangData::TPluginHangData) {
+  if (mActor && mHangData.type() == HangData::TPluginHangData) {
     uint32_t id = mHangData.get_PluginHangData().pluginId();
     mActor->CleanupPluginHang(id, true);
   }
 
   mContentParent->KillHard("HangMonitor");
   return NS_OK;
 }
 
@@ -850,18 +866,20 @@ HangMonitoredProcess::IsReportForBrowser
 NS_IMETHODIMP
 HangMonitoredProcess::UserCanceled()
 {
   MOZ_RELEASE_ASSERT(NS_IsMainThread());
   if (mHangData.type() != HangData::TPluginHangData) {
     return NS_OK;
   }
 
-  uint32_t id = mHangData.get_PluginHangData().pluginId();
-  mActor->CleanupPluginHang(id, true);
+  if (mActor) {
+    uint32_t id = mHangData.get_PluginHangData().pluginId();
+    mActor->CleanupPluginHang(id, true);
+  }
   return NS_OK;
 }
 
 ProcessHangMonitor* ProcessHangMonitor::sInstance;
 
 ProcessHangMonitor::ProcessHangMonitor()
  : mCPOWTimeout(false)
 {
--- a/dom/plugins/ipc/PluginHangUIParent.cpp
+++ b/dom/plugins/ipc/PluginHangUIParent.cpp
@@ -348,18 +348,17 @@ PluginHangUIParent::RecvUserResponse(con
   }
   mLastUserResponse = aResponse;
   mResponseTicks = ::GetTickCount();
   mIsShowing = false;
   // responseCode: 1 = Stop, 2 = Continue, 3 = Cancel
   int responseCode;
   if (aResponse & HANGUI_USER_RESPONSE_STOP) {
     // User clicked Stop
-    nsString dummy;
-    mModule->TerminateChildProcess(mMainThreadMessageLoop, &dummy);
+    mModule->TerminateChildProcess(mMainThreadMessageLoop, EmptyString());
     responseCode = 1;
   } else if(aResponse & HANGUI_USER_RESPONSE_CONTINUE) {
     mModule->OnHangUIContinue();
     // User clicked Continue
     responseCode = 2;
   } else {
     // Dialog was cancelled
     responseCode = 3;
--- a/dom/plugins/ipc/PluginModuleParent.cpp
+++ b/dom/plugins/ipc/PluginModuleParent.cpp
@@ -354,20 +354,19 @@ mozilla::plugins::TerminatePlugin(uint32
 {
     MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
 
     nsRefPtr<nsPluginHost> host = nsPluginHost::GetInst();
     nsPluginTag* pluginTag = host->PluginWithId(aPluginId);
     if (!pluginTag || !pluginTag->mPlugin) {
         return;
     }
-    nsAutoString dumpId(aBrowserDumpId);
     nsRefPtr<nsNPAPIPlugin> plugin = pluginTag->mPlugin;
     PluginModuleChromeParent* chromeParent = static_cast<PluginModuleChromeParent*>(plugin->GetLibrary());
-    chromeParent->TerminateChildProcess(MessageLoop::current(), &dumpId);
+    chromeParent->TerminateChildProcess(MessageLoop::current(), aBrowserDumpId);
 }
 
 /* static */ PluginLibrary*
 PluginModuleContentParent::LoadModule(uint32_t aPluginId)
 {
     PluginModuleMapping::NotifyLoadingModule loadingModule;
     nsAutoPtr<PluginModuleMapping> mapping(new PluginModuleMapping(aPluginId));
 
@@ -1149,18 +1148,17 @@ PluginModuleChromeParent::ShouldContinue
 #ifdef XP_WIN
     if (LaunchHangUI()) {
         return true;
     }
     // If LaunchHangUI returned false then we should proceed with the 
     // original plugin hang behaviour and kill the plugin container.
     FinishHangUI();
 #endif // XP_WIN
-    nsString dummy;
-    TerminateChildProcess(MessageLoop::current(), &dummy);
+    TerminateChildProcess(MessageLoop::current(), EmptyString());
     GetIPCChannel()->CloseWithTimeout();
     return false;
 }
 
 bool
 PluginModuleContentParent::ShouldContinueFromReplyTimeout()
 {
     nsRefPtr<ProcessHangMonitor> monitor = ProcessHangMonitor::Get();
@@ -1174,17 +1172,17 @@ PluginModuleContentParent::ShouldContinu
 void
 PluginModuleContentParent::OnExitedSyncSend()
 {
     ProcessHangMonitor::ClearHang();
 }
 
 void
 PluginModuleChromeParent::TerminateChildProcess(MessageLoop* aMsgLoop,
-                                                nsAString* aBrowserDumpId)
+                                                const nsAString& aBrowserDumpId)
 {
 #ifdef MOZ_CRASHREPORTER
 #ifdef XP_WIN
     mozilla::MutexAutoLock lock(mCrashReporterMutex);
     CrashReporterParent* crashReporter = mCrashReporter;
     if (!crashReporter) {
         // If mCrashReporter is null then the hang has ended, the plugin module
         // is shutting down. There's nothing to do here.
@@ -1210,28 +1208,28 @@ PluginModuleChromeParent::TerminateChild
     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. 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 && !aBrowserDumpId->IsEmpty() &&
-        CrashReporter::GetMinidumpForID(*aBrowserDumpId, getter_AddRefs(browserDumpFile)) &&
+    if (!aBrowserDumpId.IsEmpty() &&
+        CrashReporter::GetMinidumpForID(aBrowserDumpId, getter_AddRefs(browserDumpFile)) &&
         browserDumpFile &&
         NS_SUCCEEDED(browserDumpFile->Exists(&exists)) && exists)
     {
         // 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);
+          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) {
--- a/dom/plugins/ipc/PluginModuleParent.h
+++ b/dom/plugins/ipc/PluginModuleParent.h
@@ -382,17 +382,17 @@ class PluginModuleChromeParent
      *
      * @param aMsgLoop the main message pump associated with the module
      *   protocol.
      * @param aBrowserDumpId (optional) previously taken browser dump id. If
      *   provided TerminateChildProcess will use this browser dump file in
      *   generating a multi-process crash report. If not provided a browser
      *   dump will be taken at the time of this call.
      */
-    void TerminateChildProcess(MessageLoop* aMsgLoop, nsAString* aBrowserDumpId);
+    void TerminateChildProcess(MessageLoop* aMsgLoop, const nsAString& aBrowserDumpId);
 
 #ifdef XP_WIN
     /**
      * Called by Plugin Hang UI to notify that the user has clicked continue.
      * Used for chrome hang annotations.
      */
     void
     OnHangUIContinue();