Bug 1268434 - Mutex-protect and check GMP abstract thread before uses - c?cpearce draft
authorGerald Squelart <gsquelart@mozilla.com>
Fri, 06 May 2016 21:36:22 +1000
changeset 364353 8bd14506709a8b377d8809d623f545324012f145
parent 364352 19a1743ceb2e035e571012e88d25275ce627b925
child 520247 993cd86ddcc6f91d37cc21f1a87a6caad910b09f
push id17417
push usergsquelart@mozilla.com
push dateFri, 06 May 2016 13:23:44 +0000
bugs1268434
milestone49.0a1
Bug 1268434 - Mutex-protect and check GMP abstract thread before uses - c?cpearce GeckoMediaPluginService::mAbstractThread was not reset as expected from ShutdownGMPThread, meaning it would retain a reference to the GMP thread, and it would allow dispatch attempts to the GMP thread after shutdown. Also mAbstractThread was not protected by a mutex (as mGMPThread was), which is definitely needed now that it can be reset at shutdown time. As its prefix implies, GetAbstractThread could return a nullptr, so it should be checked before every use. Note that this GetAbstractThread call (and its check) has been moved closer to the start of functions using it, to avoid unnecessary and potentially invariant- breaking partial work to take place when we can know in advance that it won't fully succeed because the GMP thread is not available. MozReview-Commit-ID: B1drOeM65hr
dom/media/gmp/GMPService.cpp
dom/media/gmp/GMPService.h
dom/media/gmp/GMPServiceChild.h
dom/media/gmp/GMPServiceParent.cpp
dom/media/gmp/GMPServiceParent.h
dom/media/gtest/TestGMPRemoveAndDelete.cpp
--- a/dom/media/gmp/GMPService.cpp
+++ b/dom/media/gmp/GMPService.cpp
@@ -315,16 +315,17 @@ void
 GeckoMediaPluginService::ShutdownGMPThread()
 {
   LOGD(("%s::%s", __CLASS__, __FUNCTION__));
   nsCOMPtr<nsIThread> gmpThread;
   {
     MutexAutoLock lock(mMutex);
     mGMPThreadShutdown = true;
     mGMPThread.swap(gmpThread);
+    mAbstractGMPThread = nullptr;
   }
 
   if (gmpThread) {
     gmpThread->Shutdown();
   }
 }
 
 nsresult
@@ -366,28 +367,29 @@ GeckoMediaPluginService::GetThread(nsITh
     nsresult rv = NS_NewNamedThread("GMPThread", getter_AddRefs(mGMPThread));
     if (NS_FAILED(rv)) {
       return rv;
     }
 
     mAbstractGMPThread = AbstractThread::CreateXPCOMThreadWrapper(mGMPThread, false);
 
     // Tell the thread to initialize plugins
-    InitializePlugins();
+    InitializePlugins(mAbstractGMPThread.get());
   }
 
   nsCOMPtr<nsIThread> copy = mGMPThread;
   copy.forget(aThread);
 
   return NS_OK;
 }
 
 RefPtr<AbstractThread>
 GeckoMediaPluginService::GetAbstractGMPThread()
 {
+  MutexAutoLock lock(mMutex);
   return mAbstractGMPThread;
 }
 
 class GetGMPContentParentForAudioDecoderDone : public GetGMPContentParentCallback
 {
 public:
   explicit GetGMPContentParentForAudioDecoderDone(UniquePtr<GetGMPAudioDecoderCallback>&& aCallback)
    : mCallback(Move(aCallback))
--- a/dom/media/gmp/GMPService.h
+++ b/dom/media/gmp/GMPService.h
@@ -78,28 +78,28 @@ public:
   RefPtr<AbstractThread> GetAbstractGMPThread();
 
 protected:
   GeckoMediaPluginService();
   virtual ~GeckoMediaPluginService();
 
   void RemoveObsoletePluginCrashCallbacks(); // Called from add/run.
 
-  virtual void InitializePlugins() = 0;
+  virtual void InitializePlugins(AbstractThread* aAbstractGMPThread) = 0;
   virtual bool GetContentParentFrom(const nsACString& aNodeId,
                                     const nsCString& aAPI,
                                     const nsTArray<nsCString>& aTags,
                                     UniquePtr<GetGMPContentParentCallback>&& aCallback) = 0;
 
   nsresult GMPDispatch(nsIRunnable* event, uint32_t flags = NS_DISPATCH_NORMAL);
   nsresult GMPDispatch(already_AddRefed<nsIRunnable> event, uint32_t flags = NS_DISPATCH_NORMAL);
   void ShutdownGMPThread();
 
-  Mutex mMutex; // Protects mGMPThread and mGMPThreadShutdown and some members
-                // in derived classes.
+  Mutex mMutex; // Protects mGMPThread, mAbstractGMPThread and
+                // mGMPThreadShutdown and some members in derived classes.
   nsCOMPtr<nsIThread> mGMPThread;
   RefPtr<AbstractThread> mAbstractGMPThread;
   bool mGMPThreadShutdown;
   bool mShuttingDownOnGMPThread;
 
   class GMPCrashCallback
   {
   public:
--- a/dom/media/gmp/GMPServiceChild.h
+++ b/dom/media/gmp/GMPServiceChild.h
@@ -53,17 +53,17 @@ public:
 
   NS_DECL_NSIOBSERVER
 
   void SetServiceChild(UniquePtr<GMPServiceChild>&& aServiceChild);
 
   void RemoveGMPContentParent(GMPContentParent* aGMPContentParent);
 
 protected:
-  void InitializePlugins() override
+  void InitializePlugins(AbstractThread*) override
   {
     // Nothing to do here.
   }
   bool GetContentParentFrom(const nsACString& aNodeId,
                             const nsCString& aAPI,
                             const nsTArray<nsCString>& aTags,
                             UniquePtr<GetGMPContentParentCallback>&& aCallback)
     override;
--- a/dom/media/gmp/GMPServiceParent.cpp
+++ b/dom/media/gmp/GMPServiceParent.cpp
@@ -523,18 +523,22 @@ GeckoMediaPluginServiceParent::EnsureIni
 }
 
 bool
 GeckoMediaPluginServiceParent::GetContentParentFrom(const nsACString& aNodeId,
                                                     const nsCString& aAPI,
                                                     const nsTArray<nsCString>& aTags,
                                                     UniquePtr<GetGMPContentParentCallback>&& aCallback)
 {
+  RefPtr<AbstractThread> thread(GetAbstractGMPThread());
+  if (!thread) {
+    return false;
+  }
+
   RefPtr<GeckoMediaPluginServiceParent> self(this);
-  RefPtr<AbstractThread> thread(GetAbstractGMPThread());
   nsCString nodeId(aNodeId);
   nsTArray<nsCString> tags(aTags);
   nsCString api(aAPI);
   GetGMPContentParentCallback* rawCallback = aCallback.release();
   EnsureInitialized()->Then(thread, __func__,
     [self, tags, api, nodeId, rawCallback]() -> void {
       UniquePtr<GetGMPContentParentCallback> callback(rawCallback);
       RefPtr<GMPParent> gmp = self->SelectPluginForAPI(nodeId, api, tags);
@@ -550,28 +554,30 @@ GeckoMediaPluginServiceParent::GetConten
       UniquePtr<GetGMPContentParentCallback> callback(rawCallback);
       NS_WARNING("GMPService::EnsureInitialized failed.");
       callback->Done(nullptr);
     });
   return true;
 }
 
 void
-GeckoMediaPluginServiceParent::InitializePlugins()
+GeckoMediaPluginServiceParent::InitializePlugins(
+  AbstractThread* aAbstractGMPThread)
 {
+  MOZ_ASSERT(aAbstractGMPThread);
   MonitorAutoLock lock(mInitPromiseMonitor);
   if (mLoadPluginsFromDiskComplete) {
     return;
   }
 
   RefPtr<GeckoMediaPluginServiceParent> self(this);
   RefPtr<GenericPromise> p = mInitPromise.Ensure(__func__);
-  RefPtr<AbstractThread> thread(GetAbstractGMPThread());
-  InvokeAsync(thread, this, __func__, &GeckoMediaPluginServiceParent::LoadFromEnvironment)
-    ->Then(thread, __func__,
+  InvokeAsync(aAbstractGMPThread, this, __func__,
+              &GeckoMediaPluginServiceParent::LoadFromEnvironment)
+    ->Then(aAbstractGMPThread, __func__,
       [self]() -> void {
         MonitorAutoLock lock(self->mInitPromiseMonitor);
         self->mLoadPluginsFromDiskComplete = true;
         self->mInitPromise.Resolve(true, __func__);
       },
       [self]() -> void {
         MonitorAutoLock lock(self->mInitPromiseMonitor);
         self->mLoadPluginsFromDiskComplete = true;
@@ -753,16 +759,20 @@ GeckoMediaPluginServiceParent::CrashPlug
     mPlugins[i]->Crash();
   }
 }
 
 RefPtr<GenericPromise::AllPromiseType>
 GeckoMediaPluginServiceParent::LoadFromEnvironment()
 {
   MOZ_ASSERT(NS_GetCurrentThread() == mGMPThread);
+  RefPtr<AbstractThread> thread(GetAbstractGMPThread());
+  if (!thread) {
+    return GenericPromise::AllPromiseType::CreateAndReject(NS_ERROR_FAILURE, __func__);
+  }
 
   const char* env = PR_GetEnv("MOZ_GMP_PATH");
   if (!env || !*env) {
     return GenericPromise::AllPromiseType::CreateAndResolve(true, __func__);
   }
 
   nsString allpaths;
   if (NS_WARN_IF(NS_FAILED(NS_CopyNativeToUnicode(nsDependentCString(env), allpaths)))) {
@@ -780,17 +790,16 @@ GeckoMediaPluginServiceParent::LoadFromE
       break;
     } else {
       promises.AppendElement(AddOnGMPThread(nsString(Substring(allpaths, pos, next - pos))));
       pos = next + 1;
     }
   }
 
   mScannedPluginOnDisk = true;
-  RefPtr<AbstractThread> thread(GetAbstractGMPThread());
   return GenericPromise::All(thread, promises);
 }
 
 class NotifyObserversTask final : public mozilla::Runnable {
 public:
   explicit NotifyObserversTask(const char* aTopic, nsString aData = EmptyString())
     : mTopic(aTopic)
     , mData(aData)
@@ -828,16 +837,20 @@ GeckoMediaPluginServiceParent::PathRunna
 #endif
   return NS_OK;
 }
 
 RefPtr<GenericPromise>
 GeckoMediaPluginServiceParent::AsyncAddPluginDirectory(const nsAString& aDirectory)
 {
   RefPtr<AbstractThread> thread(GetAbstractGMPThread());
+  if (!thread) {
+    return GenericPromise::CreateAndReject(NS_ERROR_FAILURE, __func__);
+  }
+
   nsString dir(aDirectory);
   return InvokeAsync(thread, this, __func__, &GeckoMediaPluginServiceParent::AddOnGMPThread, dir)
     ->Then(AbstractThread::MainThread(), __func__,
       [dir]() -> void {
         LOGD(("GeckoMediaPluginServiceParent::AsyncAddPluginDirectory %s succeeded",
               NS_ConvertUTF16toUTF8(dir).get()));
         MOZ_ASSERT(NS_IsMainThread());
         // For e10s, we must fire a notification so that all ContentParents notify
@@ -1056,33 +1069,37 @@ GeckoMediaPluginServiceParent::ClonePlug
 
   return gmp.forget();
 }
 
 RefPtr<GenericPromise>
 GeckoMediaPluginServiceParent::AddOnGMPThread(nsString aDirectory)
 {
   MOZ_ASSERT(NS_GetCurrentThread() == mGMPThread);
-  LOGD(("%s::%s: %s", __CLASS__, __FUNCTION__, NS_LossyConvertUTF16toASCII(aDirectory).get()));
+  nsCString dir = NS_ConvertUTF16toUTF8(aDirectory);
+  RefPtr<AbstractThread> thread(GetAbstractGMPThread());
+  if (!thread) {
+    LOGD(("%s::%s: %s No GMP Thread", __CLASS__, __FUNCTION__, dir.get()));
+    return GenericPromise::CreateAndReject(NS_ERROR_FAILURE, __func__);
+  }
+  LOGD(("%s::%s: %s", __CLASS__, __FUNCTION__, dir.get()));
 
   nsCOMPtr<nsIFile> directory;
   nsresult rv = NS_NewLocalFile(aDirectory, false, getter_AddRefs(directory));
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return GenericPromise::CreateAndReject(NS_ERROR_FAILURE, __func__);
   }
 
   RefPtr<GMPParent> gmp = CreateGMPParent();
   if (!gmp) {
     NS_WARNING("Can't Create GMPParent");
     return GenericPromise::CreateAndReject(NS_ERROR_FAILURE, __func__);
   }
 
   RefPtr<GeckoMediaPluginServiceParent> self(this);
-  RefPtr<AbstractThread> thread(GetAbstractGMPThread());
-  nsCString dir = NS_ConvertUTF16toUTF8(aDirectory);
   return gmp->Init(this, directory)->Then(thread, __func__,
     [gmp, self, dir]() -> void {
       LOGD(("%s::%s: %s Succeeded", __CLASS__, __FUNCTION__, dir.get()));
       {
         MutexAutoLock lock(self->mMutex);
         self->mPlugins.AppendElement(gmp);
       }
       NS_DispatchToMainThread(new NotifyObserversTask("gmp-path-added"), NS_DISPATCH_NORMAL);
--- a/dom/media/gmp/GMPServiceParent.h
+++ b/dom/media/gmp/GMPServiceParent.h
@@ -107,17 +107,17 @@ private:
                             DirectoryFilter& aFilter);
   void ForgetThisSiteOnGMPThread(const nsACString& aOrigin);
   void ClearRecentHistoryOnGMPThread(PRTime aSince);
 
 protected:
   friend class GMPParent;
   void ReAddOnGMPThread(const RefPtr<GMPParent>& aOld);
   void PluginTerminated(const RefPtr<GMPParent>& aOld);
-  void InitializePlugins() override;
+  void InitializePlugins(AbstractThread* aAbstractGMPThread) override;
   RefPtr<GenericPromise::AllPromiseType> LoadFromEnvironment();
   RefPtr<GenericPromise> AddOnGMPThread(nsString aDirectory);
   bool GetContentParentFrom(const nsACString& aNodeId,
                             const nsCString& aAPI,
                             const nsTArray<nsCString>& aTags,
                             UniquePtr<GetGMPContentParentCallback>&& aCallback)
     override;
 private:
--- a/dom/media/gtest/TestGMPRemoveAndDelete.cpp
+++ b/dom/media/gtest/TestGMPRemoveAndDelete.cpp
@@ -230,16 +230,17 @@ GMPRemoveTest::Setup()
   GeneratePlugin();
   GetService()->GetThread(getter_AddRefs(mGMPThread));
 
   // Spin the event loop until the GMP service has had a chance to complete
   // adding GMPs from MOZ_GMP_PATH. Otherwise, the RemovePluginDirectory()
   // below may complete before we're finished adding GMPs from MOZ_GMP_PATH,
   // and we'll end up not removing the GMP, and the test will fail.
   RefPtr<AbstractThread> thread(GetServiceParent()->GetAbstractGMPThread());
+  EXPECT_TRUE(thread);
   GMPTestMonitor* mon = &mTestMonitor;
   GetServiceParent()->EnsureInitialized()->Then(thread, __func__,
     [mon]() { mon->SetFinished(); },
     [mon]() { mon->SetFinished(); }
   );
   mTestMonitor.AwaitFinished();
 
   nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();