Bug 1469603 - Use a recursive lock in crash reporter callbacks that might be called synchronously. r=erahm, a=RyanVM
authorGabriele Svelto <gsvelto@mozilla.com>
Thu, 21 Jun 2018 09:56:26 +0200
changeset 473798 2826a36d480d
parent 473797 1d6943a03afb
child 473799 d7133ef39f36
push id1739
push userryanvm@gmail.com
push dateMon, 02 Jul 2018 20:22:04 +0000
treeherdermozilla-release@d7133ef39f36 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerserahm, RyanVM
bugs1469603
milestone61.0.1
Bug 1469603 - Use a recursive lock in crash reporter callbacks that might be called synchronously. r=erahm, a=RyanVM
dom/plugins/ipc/PluginModuleParent.cpp
dom/plugins/ipc/PluginModuleParent.h
--- a/dom/plugins/ipc/PluginModuleParent.cpp
+++ b/dom/plugins/ipc/PluginModuleParent.cpp
@@ -574,17 +574,17 @@ PluginModuleChromeParent::InitCrashRepor
     }
 
     NativeThreadId threadId;
     if (!CallInitCrashReporter(shmem, &threadId)) {
         return false;
     }
 
     {
-      mozilla::MutexAutoLock lock(mCrashReporterMutex);
+      mozilla::RecursiveMutexAutoLock lock(mCrashReporterMutex);
       mCrashReporter = MakeUnique<ipc::CrashReporterHost>(
         GeckoProcessType_Plugin,
         shmem,
         threadId);
     }
 
     return true;
 }
@@ -711,17 +711,17 @@ PluginModuleChromeParent::~PluginModuleC
 
     mozilla::HangMonitor::UnregisterAnnotator(*this);
 }
 
 void
 PluginModuleChromeParent::WriteExtraDataForMinidump()
 {
     // mCrashReporterMutex is already held by the caller
-    mCrashReporterMutex.AssertCurrentThreadOwns();
+    mCrashReporterMutex.AssertCurrentThreadIn();
 
     typedef nsDependentCString cstring;
 
     // Get the plugin filename, try to get just the file leafname
     const std::string& pluginFile = mSubprocess->GetPluginFilePath();
     size_t filePos = pluginFile.rfind(FILE_PATH_SEPARATOR);
     if (filePos == std::string::npos)
         filePos = 0;
@@ -1077,17 +1077,17 @@ PluginModuleContentParent::OnExitedSyncS
 }
 
 void
 PluginModuleChromeParent::TakeFullMinidump(base::ProcessId aContentPid,
                                            const nsAString& aBrowserDumpId,
                                            std::function<void(nsString)>&& aCallback,
                                            bool aAsync)
 {
-    mozilla::MutexAutoLock lock(mCrashReporterMutex);
+    mozilla::RecursiveMutexAutoLock lock(mCrashReporterMutex);
 
     if (!mCrashReporter || !mTakeFullMinidumpCallback.IsEmpty()) {
         aCallback(EmptyString());
         return;
     }
     mTakeFullMinidumpCallback.Init(Move(aCallback), aAsync);
 
     nsString browserDumpId{aBrowserDumpId};
@@ -1164,17 +1164,17 @@ PluginModuleChromeParent::ReleasePluginR
 }
 
 void
 PluginModuleChromeParent::TakeBrowserAndPluginMinidumps(bool aReportsReady,
                                                         base::ProcessId aContentPid,
                                                         const nsAString& aBrowserDumpId,
                                                         bool aAsync)
 {
-    mCrashReporterMutex.AssertCurrentThreadOwns();
+    mCrashReporterMutex.AssertCurrentThreadIn();
 
     // 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 (!aReportsReady) {
         mBrowserDumpFile = nullptr;
         CrashReporter::DeleteMinidumpFilesForID(aBrowserDumpId);
@@ -1199,17 +1199,17 @@ PluginModuleChromeParent::TakeBrowserAnd
     }
 }
 
 void
 PluginModuleChromeParent::OnTakeFullMinidumpComplete(bool aReportsReady,
                                                      base::ProcessId aContentPid,
                                                      const nsAString& aBrowserDumpId)
 {
-    mCrashReporterMutex.AssertCurrentThreadOwns();
+    mCrashReporterMutex.AssertCurrentThreadIn();
 
     if (aReportsReady) {
         nsString dumpId = mCrashReporter->MinidumpID();
         PLUGIN_LOG_DEBUG(
                          ("generated paired browser/plugin minidumps: %s)",
                           NS_ConvertUTF16toUTF8(dumpId).get()));
         nsAutoCString additionalDumps("browser");
         nsCOMPtr<nsIFile> pluginDumpFile;
@@ -1284,17 +1284,17 @@ PluginModuleChromeParent::TerminateChild
         TerminateChildProcessOnDumpComplete(aMsgLoop, aMonitorDescription);
     }
 }
 
 void
 PluginModuleChromeParent::TerminateChildProcessOnDumpComplete(MessageLoop* aMsgLoop,
                                                               const nsCString& aMonitorDescription)
 {
-    mCrashReporterMutex.AssertCurrentThreadOwns();
+    mCrashReporterMutex.AssertCurrentThreadIn();
 
     if (!mCrashReporter) {
         // If mCrashReporter is null then the hang has ended, the plugin module
         // is shutting down. There's nothing to do here.
         mTerminateChildProcessCallback.Invoke(true);
         return;
     }
     mCrashReporter->AddNote(NS_LITERAL_CSTRING("PluginHang"),
@@ -1501,17 +1501,17 @@ RemoveMinidump(nsIFile* minidump)
         extraFile->Remove(true);
     }
 }
 #endif // MOZ_CRASHREPORTER_INJECTOR
 
 void
 PluginModuleChromeParent::ProcessFirstMinidump()
 {
-    mozilla::MutexAutoLock lock(mCrashReporterMutex);
+    mozilla::RecursiveMutexAutoLock lock(mCrashReporterMutex);
 
     if (!mCrashReporter)
         return;
 
     WriteExtraDataForMinidump();
 
     if (mCrashReporter->HasMinidump()) {
         // A minidump may be set in TerminateChildProcess, which means the
--- a/dom/plugins/ipc/PluginModuleParent.h
+++ b/dom/plugins/ipc/PluginModuleParent.h
@@ -12,16 +12,17 @@
 #include "mozilla/HangAnnotations.h"
 #include "mozilla/PluginLibrary.h"
 #include "mozilla/ipc/CrashReporterHost.h"
 #include "mozilla/plugins/PluginProcessParent.h"
 #include "mozilla/plugins/PPluginModuleParent.h"
 #include "mozilla/plugins/PluginMessageUtils.h"
 #include "mozilla/plugins/PluginTypes.h"
 #include "mozilla/ipc/TaskFactory.h"
+#include "mozilla/RecursiveMutex.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/Unused.h"
 #include "npapi.h"
 #include "npfunctions.h"
 #include "nsExceptionHandler.h"
 #include "nsDataHashtable.h"
 #include "nsHashKeys.h"
 #include "nsIObserver.h"
@@ -312,19 +313,21 @@ protected:
 
     RefPtr<layers::TextureClientRecycleAllocator> mTextureAllocatorForDirectBitmap;
     RefPtr<layers::TextureClientRecycleAllocator> mTextureAllocatorForDXGISurface;
 
     /**
      * This mutex protects the crash reporter when the Plugin Hang UI event
      * handler is executing off main thread. It is intended to protect both
      * the mCrashReporter variable in addition to the CrashReporterHost object
-     * that mCrashReporter refers to.
+     * that mCrashReporter refers to. Sometimes asynchronous crash reporter
+     * callbacks are dispatched synchronously while the caller is still holding
+     * the mutex. This requires recursive locking support in the mutex.
      */
-    mozilla::Mutex mCrashReporterMutex;
+    mozilla::RecursiveMutex mCrashReporterMutex;
     UniquePtr<ipc::CrashReporterHost> mCrashReporter;
 };
 
 class PluginModuleContentParent : public PluginModuleParent
 {
   public:
     explicit PluginModuleContentParent();