Bug 1289378 - Fix inter-locking in PDMFactory+GMPDecoderModule - r=jya
authorGerald Squelart <gsquelart@mozilla.com>
Wed, 27 Jul 2016 12:43:33 +1000
changeset 331823 79f30219fae5aa4d321d17f28fe050442c7d51cf
parent 331822 2f3e55cbfea90538a32081f11f527a7f6b24dc56
child 331824 e11d665e4b6d5d192f348db65f54cc2ede073bf5
push id9858
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 14:37:10 +0000
treeherdermozilla-aurora@203106ef6cb6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjya
bugs1289378
milestone50.0a1
Bug 1289378 - Fix inter-locking in PDMFactory+GMPDecoderModule - r=jya Say PDMFactory::EnsureInit() runs on the main thread (effectively locking it) and then tries to lock sMonitor; if another thread was also running EnsureInit() and locked sMonitor, it will dead-lock when trying to sync- dispatch the GMPDecoderModule creation to the main thread. This can be fixed by ensuring that the actual PDMFactory instance creation is always done on the main thread (and that we don't hold sMonitor before dispatching to the main thread.) Note that we can now simplify GMPDecoderModule::Init and assert we are already on the main thread. MozReview-Commit-ID: 8xHpoymw6po
dom/media/platforms/PDMFactory.cpp
dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp
--- a/dom/media/platforms/PDMFactory.cpp
+++ b/dom/media/platforms/PDMFactory.cpp
@@ -24,16 +24,17 @@
 #ifdef MOZ_WIDGET_ANDROID
 #include "AndroidDecoderModule.h"
 #endif
 #include "GMPDecoderModule.h"
 
 #include "mozilla/ClearOnShutdown.h"
 #include "mozilla/SharedThreadPool.h"
 #include "mozilla/StaticPtr.h"
+#include "mozilla/SyncRunnable.h"
 #include "mozilla/TaskQueue.h"
 
 #include "MediaInfo.h"
 #include "MediaPrefs.h"
 #include "FuzzingWrapper.h"
 #include "H264Converter.h"
 
 #include "AgnosticDecoderModule.h"
@@ -82,27 +83,41 @@ PDMFactory::PDMFactory()
 
 PDMFactory::~PDMFactory()
 {
 }
 
 void
 PDMFactory::EnsureInit() const
 {
-  StaticMutexAutoLock mon(sMonitor);
-  if (!sInstance) {
-    sInstance = new PDMFactoryImpl();
+  {
+    StaticMutexAutoLock mon(sMonitor);
+    if (sInstance) {
+      // Quick exit if we already have an instance.
+      return;
+    }
     if (NS_IsMainThread()) {
+      // On the main thread and holding the lock -> Create instance.
+      sInstance = new PDMFactoryImpl();
       ClearOnShutdown(&sInstance);
-    } else {
-      nsCOMPtr<nsIRunnable> runnable =
-        NS_NewRunnableFunction([]() { ClearOnShutdown(&sInstance); });
-      NS_DispatchToMainThread(runnable);
+      return;
     }
   }
+
+  // Not on the main thread -> Sync-dispatch creation to main thread.
+  nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
+  nsCOMPtr<nsIRunnable> runnable =
+    NS_NewRunnableFunction([]() {
+      StaticMutexAutoLock mon(sMonitor);
+      if (!sInstance) {
+        sInstance = new PDMFactoryImpl();
+        ClearOnShutdown(&sInstance);
+      }
+    });
+  SyncRunnable::DispatchToThread(mainThread, runnable);
 }
 
 already_AddRefed<MediaDataDecoder>
 PDMFactory::CreateDecoder(const CreateDecoderParams& aParams)
 {
   const TrackInfo& config = aParams.mConfig;
   bool isEncrypted = mEMEPDM && config.mCrypto.mValid;
 
--- a/dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp
+++ b/dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp
@@ -8,17 +8,16 @@
 #include "DecoderDoctorDiagnostics.h"
 #include "GMPAudioDecoder.h"
 #include "GMPVideoDecoder.h"
 #include "MediaDataDecoderProxy.h"
 #include "MediaPrefs.h"
 #include "mozIGeckoMediaPluginService.h"
 #include "nsServiceManagerUtils.h"
 #include "mozilla/StaticMutex.h"
-#include "mozilla/SyncRunnable.h"
 #include "gmp-audio-decode.h"
 #include "gmp-video-decode.h"
 #ifdef XP_WIN
 #include "WMFDecoderModule.h"
 #endif
 
 namespace mozilla {
 
@@ -164,23 +163,17 @@ GMPDecoderModule::UpdateUsableCodecs()
                              nsDependentCString(gmp.mKeySystem));
   }
 }
 
 /* static */
 void
 GMPDecoderModule::Init()
 {
-  if (!NS_IsMainThread()) {
-    nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
-    nsCOMPtr<nsIRunnable> runnable =
-      NS_NewRunnableFunction([]() { Init(); });
-    SyncRunnable::DispatchToThread(mainThread, runnable);
-    return;
-  }
+  MOZ_ASSERT(NS_IsMainThread());
   // GMPService::HasPluginForAPI is main thread only, so to implement
   // SupportsMimeType() we build a table of the codecs which each whitelisted
   // GMP has and update it when any GMPs are removed or added at runtime.
   UpdateUsableCodecs();
 }
 
 /* static */
 const Maybe<nsCString>