Bug 1268434 - Mutex-protect and check GMP abstract thread before uses - r=cpearce a=ritu
authorGerald Squelart <gsquelart@mozilla.com>
Fri, 06 May 2016 21:36:22 +1000
changeset 332816 261915e8601067a49679353ccb9cd601bd8a619c
parent 332815 ba62a7d1c20dbf3d06ead5912b0d0f689bb79000
child 332817 1bb8e2207e8eeced7cdae427f7766831c3141795
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscpearce, ritu
bugs1268434
milestone48.0a2
Bug 1268434 - Mutex-protect and check GMP abstract thread before uses - r=cpearce a=ritu 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
@@ -357,28 +358,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,27 +78,27 @@ 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);
   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
@@ -536,18 +536,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);
@@ -563,28 +567,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;
@@ -766,16 +772,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)))) {
@@ -793,17 +803,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 nsRunnable {
 public:
   explicit NotifyObserversTask(const char* aTopic, nsString aData = EmptyString())
     : mTopic(aTopic)
     , mData(aData)
@@ -841,16 +850,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
@@ -1074,33 +1087,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();