Bug 1469603 - Use a recursive lock in crash reporter callbacks that might be called synchronously r=erahm
☠☠ backed out by 5dbc311fb93c ☠ ☠
authorGabriele Svelto <gsvelto@mozilla.com>
Wed, 20 Jun 2018 20:49:35 +0000
changeset 477455 f966dedaa07cc97f0d2f28bd6414577e1767300d
parent 477454 f918ad3d1296b31808da0b810757df4f0889414c
child 477456 5dbc311fb93c0f6d4c82eb14fa9062984251ccfe
push id9385
push userdluca@mozilla.com
push dateFri, 22 Jun 2018 15:47:18 +0000
treeherdermozilla-beta@82a9a1027e2b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerserahm
bugs1469603
milestone62.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 1469603 - Use a recursive lock in crash reporter callbacks that might be called synchronously r=erahm Differential Revision: https://phabricator.services.mozilla.com/D1724
dom/plugins/ipc/PluginModuleParent.cpp
dom/plugins/ipc/PluginModuleParent.h
--- a/dom/plugins/ipc/PluginModuleParent.cpp
+++ b/dom/plugins/ipc/PluginModuleParent.cpp
@@ -575,17 +575,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;
 }
@@ -715,17 +715,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;
@@ -1081,17 +1081,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(std::move(aCallback), aAsync);
 
     nsString browserDumpId{aBrowserDumpId};
@@ -1168,17 +1168,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);
@@ -1203,17 +1203,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;
@@ -1288,17 +1288,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"),
@@ -1505,17 +1505,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
@@ -312,19 +312,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();