Bug 1315850 - Add threadsafe ChromiumCDMProxy::GetCDMParent. r=gerald draft
authorChris Pearce <cpearce@mozilla.com>
Mon, 13 Mar 2017 13:47:20 +1300
changeset 504171 aa854e9d88965d7da60231d6f6a3912bf6ad2eeb
parent 504170 018abf2b8e1b351a29ba62275a2681fe9ea4fc24
child 504172 b6d499cafef2d6a6558b0db703b60320dea67803
push id50748
push userbmo:cpearce@mozilla.com
push dateFri, 24 Mar 2017 01:10:17 +0000
reviewersgerald
bugs1315850
milestone55.0a1
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_