Bug 1315850 - Add threadsafe ChromiumCDMProxy::GetCDMParent. r=gerald
authorChris Pearce <cpearce@mozilla.com>
Mon, 13 Mar 2017 13:47:20 +1300
changeset 349438 f50cc93d6ec78a2b41b70657ffdb1380a22b3a1f
parent 349437 fe49faf3ab88bc3eb39806fd12d49f17cd40c184
child 349439 832022d6eaf8f8836a344f069eb811fcee564fda
push id31551
push usercbook@mozilla.com
push dateFri, 24 Mar 2017 13:24:56 +0000
treeherdermozilla-central@4c987b7ed54a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgerald
bugs1315850
milestone55.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 1315850 - Add threadsafe ChromiumCDMProxy::GetCDMParent. r=gerald The MediaKeys accesses the ChromiumCDMProxy on the main thread. But the ChromiumCDMVideoDecoder will need to access the ChromiumCDMProxy on the decode task queue in order to get a reference to the ChromiumCDMParent so that it can talk to the CDM (on the GMP thread). Additionally we'll need to shutdown the ChromiumCDMProxy, and if we do that on the main threrad while the ChromiumCDMVideoDecoder is trying to get the ChromiumCDMParent reference, we could hit thread safety issues. So we need to hold a lock while reading or writing from the ChromiumCDMProxy's reference to the ChromiumCDMParent. So add a GetCDMParent() function to the ChromiumCDMProxy which takes the lock while reading or writing the reference. This means that the caller will always get a valid reference. There is no guarantee that the ChromiumCDMParent isn't shutdown after the reference is taken; if that happens, the ChromiumCDMParent returned will fail on all operations. In a later patch in this series, the ChromiumCDMProxy will anull its reference to the ChromiumCDMParent on shutdown, and cause GetCDMParent to return null. So callers need to null check the return value of GetCDMParent. MozReview-Commit-ID: 4xL41YbwkxL
dom/media/gmp/ChromiumCDMProxy.cpp
dom/media/gmp/ChromiumCDMProxy.h
--- a/dom/media/gmp/ChromiumCDMProxy.cpp
+++ b/dom/media/gmp/ChromiumCDMProxy.cpp
@@ -19,17 +19,17 @@ ChromiumCDMProxy::ChromiumCDMProxy(dom::
                                    bool aPersistentStateRequired,
                                    nsIEventTarget* aMainThread)
   : CDMProxy(aKeys,
              aKeySystem,
              aDistinctiveIdentifierRequired,
              aPersistentStateRequired,
              aMainThread)
   , mCrashHelper(aCrashHelper)
-  , mCDM(nullptr)
+  , mCDMMutex("ChromiumCDMProxy")
   , mGMPThread(GetGMPAbstractThread())
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_COUNT_CTOR(ChromiumCDMProxy);
 }
 
 ChromiumCDMProxy::~ChromiumCDMProxy()
 {
@@ -96,17 +96,20 @@ ChromiumCDMProxy::Init(PromiseId aPromis
           if (!cdm->Init(self,
                          self->mDistinctiveIdentifierRequired,
                          self->mPersistentStateRequired)) {
             self->RejectPromise(aPromiseId,
                                 NS_ERROR_FAILURE,
                                 NS_LITERAL_CSTRING("GetCDM failed."));
             return;
           }
-          self->mCDM = cdm;
+          {
+            MutexAutoLock lock(self->mCDMMutex);
+            self->mCDM = cdm;
+          }
           self->OnCDMCreated(aPromiseId);
         },
         [self, aPromiseId](nsresult rv) {
           self->RejectPromise(
             aPromiseId, NS_ERROR_FAILURE, NS_LITERAL_CSTRING("GetCDM failed."));
         });
     }));
 
@@ -122,23 +125,31 @@ ChromiumCDMProxy::OnCDMCreated(uint32_t 
           this);
 
   if (!NS_IsMainThread()) {
     mMainThread->Dispatch(NewRunnableMethod<PromiseId>(
                             this, &ChromiumCDMProxy::OnCDMCreated, aPromiseId),
                           NS_DISPATCH_NORMAL);
     return;
   }
-  // This should only be called once the CDM has been created.
-  MOZ_ASSERT(mCDM);
   MOZ_ASSERT(NS_IsMainThread());
   if (mKeys.IsNull()) {
     return;
   }
-  mKeys->OnCDMCreated(aPromiseId, mCDM->PluginId());
+  RefPtr<gmp::ChromiumCDMParent> cdm = GetCDMParent();
+  // This should only be called once the CDM has been created.
+  MOZ_ASSERT(cdm);
+  if (cdm) {
+    mKeys->OnCDMCreated(aPromiseId, cdm->PluginId());
+  } else {
+    // No CDM? Shouldn't be possible, but reject the promise anyway...
+    mKeys->RejectPromise(aPromiseId,
+                         NS_ERROR_DOM_INVALID_STATE_ERR,
+                         NS_LITERAL_CSTRING("Null CDM in OnCDMCreated()"));
+  }
 }
 
 #ifdef DEBUG
 bool
 ChromiumCDMProxy::IsOnOwnerThread()
 {
   return mGMPThread->IsCurrentThreadIn();
 }
@@ -185,22 +196,23 @@ ChromiumCDMProxy::CreateSession(uint32_t
           aCreateSessionToken,
           (int)aSessionType,
           aPromiseId,
           aInitData.Length());
 
   uint32_t sessionType = ToCDMSessionType(aSessionType);
   uint32_t initDataType = ToCDMInitDataType(aInitDataType);
 
+  RefPtr<gmp::ChromiumCDMParent> cdm = GetCDMParent();
   mGMPThread->Dispatch(
     NewRunnableMethod<uint32_t,
                       uint32_t,
                       uint32_t,
                       uint32_t,
-                      nsTArray<uint8_t>>(mCDM,
+                      nsTArray<uint8_t>>(cdm,
                                          &gmp::ChromiumCDMParent::CreateSession,
                                          aCreateSessionToken,
                                          sessionType,
                                          initDataType,
                                          aPromiseId,
                                          Move(aInitData)));
 }
 
@@ -218,69 +230,74 @@ void
 ChromiumCDMProxy::SetServerCertificate(PromiseId aPromiseId,
                                        nsTArray<uint8_t>& aCert)
 {
   MOZ_ASSERT(NS_IsMainThread());
   EME_LOG("ChromiumCDMProxy::SetServerCertificate(pid=%u) certLen=%zu",
           aPromiseId,
           aCert.Length());
 
+  RefPtr<gmp::ChromiumCDMParent> cdm = GetCDMParent();
   mGMPThread->Dispatch(NewRunnableMethod<uint32_t, nsTArray<uint8_t>>(
-    mCDM,
+    cdm,
     &gmp::ChromiumCDMParent::SetServerCertificate,
     aPromiseId,
     Move(aCert)));
 }
 
 void
 ChromiumCDMProxy::UpdateSession(const nsAString& aSessionId,
                                 PromiseId aPromiseId,
                                 nsTArray<uint8_t>& aResponse)
 {
   MOZ_ASSERT(NS_IsMainThread());
   EME_LOG("ChromiumCDMProxy::UpdateSession(sid='%s', pid=%u) responseLen=%zu",
           NS_ConvertUTF16toUTF8(aSessionId).get(),
           aPromiseId,
           aResponse.Length());
 
-  mGMPThread->Dispatch(NewRunnableMethod<nsCString, uint32_t, nsTArray<uint8_t>>(
-      mCDM,
+  RefPtr<gmp::ChromiumCDMParent> cdm = GetCDMParent();
+  mGMPThread->Dispatch(
+    NewRunnableMethod<nsCString, uint32_t, nsTArray<uint8_t>>(
+      cdm,
       &gmp::ChromiumCDMParent::UpdateSession,
       NS_ConvertUTF16toUTF8(aSessionId),
       aPromiseId,
       Move(aResponse)));
 }
 
 void
 ChromiumCDMProxy::CloseSession(const nsAString& aSessionId,
                                PromiseId aPromiseId)
 {
   MOZ_ASSERT(NS_IsMainThread());
   EME_LOG("ChromiumCDMProxy::CloseSession(sid='%s', pid=%u)",
           NS_ConvertUTF16toUTF8(aSessionId).get(),
           aPromiseId);
 
+  RefPtr<gmp::ChromiumCDMParent> cdm = GetCDMParent();
   mGMPThread->Dispatch(NewRunnableMethod<nsCString, uint32_t>(
-    mCDM,
+    cdm,
     &gmp::ChromiumCDMParent::CloseSession,
     NS_ConvertUTF16toUTF8(aSessionId),
     aPromiseId));
 }
 
 void
 ChromiumCDMProxy::RemoveSession(const nsAString& aSessionId,
                                 PromiseId aPromiseId)
 {
   MOZ_ASSERT(NS_IsMainThread());
   EME_LOG("ChromiumCDMProxy::RemoveSession(sid='%s', pid=%u)",
           NS_ConvertUTF16toUTF8(aSessionId).get(),
           aPromiseId);
 
+  RefPtr<gmp::ChromiumCDMParent> cdm = GetCDMParent();
   mGMPThread->Dispatch(NewRunnableMethod<nsCString, uint32_t>(
-    mCDM,
+    cdm,
     &gmp::ChromiumCDMParent::RemoveSession,
     NS_ConvertUTF16toUTF8(aSessionId),
     aPromiseId));
 }
 
 void
 ChromiumCDMProxy::Shutdown()
 {
@@ -468,17 +485,17 @@ CDMCaps&
 ChromiumCDMProxy::Capabilites()
 {
   return mCapabilites;
 }
 
 RefPtr<DecryptPromise>
 ChromiumCDMProxy::Decrypt(MediaRawData* aSample)
 {
-  RefPtr<gmp::ChromiumCDMParent> cdm = mCDM;
+  RefPtr<gmp::ChromiumCDMParent> cdm = GetCDMParent();
   RefPtr<MediaRawData> sample = aSample;
   return InvokeAsync(
     mGMPThread, __func__, [cdm, sample]() { return cdm->Decrypt(sample); });
 }
 
 void
 ChromiumCDMProxy::GetSessionIdsForKeyId(const nsTArray<uint8_t>& aKeyId,
                                         nsTArray<nsCString>& aSessionIds)
@@ -490,9 +507,17 @@ ChromiumCDMProxy::GetSessionIdsForKeyId(
 void
 ChromiumCDMProxy::Terminated()
 {
   if (!mKeys.IsNull()) {
     mKeys->Terminated();
   }
 }
 
+already_AddRefed<gmp::ChromiumCDMParent>
+ChromiumCDMProxy::GetCDMParent()
+{
+  MutexAutoLock lock(mCDMMutex);
+  RefPtr<gmp::ChromiumCDMParent> cdm = mCDM;
+  return cdm.forget();
+}
+
 } // namespace mozilla
--- a/dom/media/gmp/ChromiumCDMProxy.h
+++ b/dom/media/gmp/ChromiumCDMProxy.h
@@ -104,22 +104,27 @@ public:
                              nsTArray<nsCString>& aSessionIds) override;
 
 #ifdef DEBUG
   bool IsOnOwnerThread() override;
 #endif
 
   ChromiumCDMProxy* AsChromiumCDMProxy() override { return this; }
 
+  // Threadsafe. Note this may return a reference to a shutdown
+  // CDM, which will fail on all operations.
+  already_AddRefed<gmp::ChromiumCDMParent> GetCDMParent();
+
 private:
   void OnCDMCreated(uint32_t aPromiseId);
 
   ~ChromiumCDMProxy();
 
   GMPCrashHelper* mCrashHelper;
 
+  Mutex mCDMMutex;
   RefPtr<gmp::ChromiumCDMParent> mCDM;
   RefPtr<AbstractThread> mGMPThread;
 };
 
 } // namespace mozilla
 
 #endif // GMPCDMProxy_h_