Bug 1316215 - Block GMPContentParent close while a GMPService::GetContentParent is being processed. r=gerald draft
authorChris Pearce <cpearce@mozilla.com>
Wed, 16 Nov 2016 10:59:08 +1300
changeset 443255 59aa7bcfe8b8f44274d136d6147a946542a64fff
parent 443254 bc61628e97243db55b68d1efc93e7b55f32628e5
child 443256 8cc04d57ea1ef4e48c7ff088dbb12eabe4b3b223
push id36941
push usercpearce@mozilla.com
push dateThu, 24 Nov 2016 04:18:29 +0000
reviewersgerald
bugs1316215
milestone53.0a1
Bug 1316215 - Block GMPContentParent close while a GMPService::GetContentParent is being processed. r=gerald When GMPService::GetContentParent returns a MozPromise, we end up failing in test_peerConnection_scaleResolution.html with e10s enabled because we Close() the GMPContentParent twice. The test causes two GMPVideoEncoderParents to be created. When the number of IPDL actors on the GMPContentParent reach 0, we close the IPC connection. With GetContentParent() returning a MozPromise, it's more async, and so we can end up requesting the content parent in order to create the second GMPVideoEncoderParent, but while we're waiting for the promise to resolve the previous GMPVideoEncoderParent is destroyed and the GMPContentParent closes its IPC connection. Then the GetContentParent promise resolves, and that fails to operate correctly since it's closed its IPC connection. My solution here is to add a "blocker" that prevents the GMPContentParent from being shutdown while we're waiting for the GetContentParent promise to resolve. MozReview-Commit-ID: HxBkFkmv0tV
dom/media/gmp/GMPContentParent.cpp
dom/media/gmp/GMPContentParent.h
dom/media/gmp/GMPParent.cpp
dom/media/gmp/GMPService.cpp
dom/media/gmp/GMPService.h
dom/media/gmp/GMPServiceChild.cpp
--- a/dom/media/gmp/GMPContentParent.cpp
+++ b/dom/media/gmp/GMPContentParent.cpp
@@ -113,22 +113,38 @@ GMPContentParent::DecryptorDestroyed(GMP
 {
   MOZ_ASSERT(GMPThread() == NS_GetCurrentThread());
 
   MOZ_ALWAYS_TRUE(mDecryptors.RemoveElement(aSession));
   CloseIfUnused();
 }
 
 void
+GMPContentParent::AddCloseBlocker()
+{
+  MOZ_ASSERT(GMPThread() == NS_GetCurrentThread());
+  ++mCloseBlockerCount;
+}
+
+void
+GMPContentParent::RemoveCloseBlocker()
+{
+  MOZ_ASSERT(GMPThread() == NS_GetCurrentThread());
+  --mCloseBlockerCount;
+  CloseIfUnused();
+}
+
+void
 GMPContentParent::CloseIfUnused()
 {
   if (mAudioDecoders.IsEmpty() &&
       mDecryptors.IsEmpty() &&
       mVideoDecoders.IsEmpty() &&
-      mVideoEncoders.IsEmpty()) {
+      mVideoEncoders.IsEmpty() &&
+      mCloseBlockerCount == 0) {
     RefPtr<GMPContentParent> toClose;
     if (mParent) {
       toClose = mParent->ForgetGMPContentParent();
     } else {
       toClose = this;
       RefPtr<GeckoMediaPluginServiceChild> gmp(
         GeckoMediaPluginServiceChild::GetSingleton());
       gmp->RemoveGMPContentParent(toClose);
--- a/dom/media/gmp/GMPContentParent.h
+++ b/dom/media/gmp/GMPContentParent.h
@@ -57,17 +57,37 @@ public:
   {
     mPluginId = aPluginId;
   }
   uint32_t GetPluginId() const
   {
     return mPluginId;
   }
 
+  class CloseBlocker {
+  public:
+    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CloseBlocker)
+
+    explicit CloseBlocker(GMPContentParent* aParent)
+      : mParent(aParent)
+    {
+      mParent->AddCloseBlocker();
+    }
+    RefPtr<GMPContentParent> mParent;
+  private:
+    ~CloseBlocker() {
+      mParent->RemoveCloseBlocker();
+    }
+  };
+
 private:
+
+  void AddCloseBlocker();
+  void RemoveCloseBlocker();
+
   ~GMPContentParent();
 
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
   PGMPVideoDecoderParent* AllocPGMPVideoDecoderParent(const uint32_t& aDecryptorId) override;
   bool DeallocPGMPVideoDecoderParent(PGMPVideoDecoderParent* aActor) override;
 
   PGMPVideoEncoderParent* AllocPGMPVideoEncoderParent() override;
@@ -90,14 +110,15 @@ private:
   nsTArray<RefPtr<GMPVideoDecoderParent>> mVideoDecoders;
   nsTArray<RefPtr<GMPVideoEncoderParent>> mVideoEncoders;
   nsTArray<RefPtr<GMPDecryptorParent>> mDecryptors;
   nsTArray<RefPtr<GMPAudioDecoderParent>> mAudioDecoders;
   nsCOMPtr<nsIThread> mGMPThread;
   RefPtr<GMPParent> mParent;
   nsCString mDisplayName;
   uint32_t mPluginId;
+  uint32_t mCloseBlockerCount = 0;
 };
 
 } // namespace gmp
 } // namespace mozilla
 
 #endif // GMPParent_h_
--- a/dom/media/gmp/GMPParent.cpp
+++ b/dom/media/gmp/GMPParent.cpp
@@ -1054,18 +1054,19 @@ GMPParent::RecvAsyncShutdownComplete()
 }
 
 void
 GMPParent::ResolveGetContentParentPromises()
 {
   nsTArray<UniquePtr<MozPromiseHolder<GetGMPContentParentPromise>>> promises;
   promises.SwapElements(mGetContentParentPromises);
   MOZ_ASSERT(mGetContentParentPromises.IsEmpty());
+  RefPtr<GMPContentParent::CloseBlocker> blocker(new GMPContentParent::CloseBlocker(mGMPContentParent));
   for (auto& holder : promises) {
-    holder->Resolve(mGMPContentParent, __func__);
+    holder->Resolve(blocker, __func__);
   }
 }
 
 PGMPContentParent*
 GMPParent::AllocPGMPContentParent(Transport* aTransport, ProcessId aOtherPid)
 {
   MOZ_ASSERT(GMPThread() == NS_GetCurrentThread());
   MOZ_ASSERT(!mGMPContentParent);
@@ -1092,17 +1093,18 @@ GMPParent::RejectGetContentParentPromise
 
 void
 GMPParent::GetGMPContentParent(UniquePtr<MozPromiseHolder<GetGMPContentParentPromise>>&& aPromiseHolder)
 {
   LOGD("%s %p", __FUNCTION__, this);
   MOZ_ASSERT(GMPThread() == NS_GetCurrentThread());
 
   if (mGMPContentParent) {
-    aPromiseHolder->Resolve(mGMPContentParent, __func__);
+    RefPtr<GMPContentParent::CloseBlocker> blocker(new GMPContentParent::CloseBlocker(mGMPContentParent));
+    aPromiseHolder->Resolve(blocker, __func__);
   } else {
     mGetContentParentPromises.AppendElement(Move(aPromiseHolder));
     // If we don't have a GMPContentParent and we try to get one for the first
     // time (mGetContentParentPromises.Length() == 1) then call PGMPContent::Open. If more
     // calls to GetGMPContentParent happen before mGMPContentParent has been
     // set then we should just store them, so that they get called when we set
     // mGMPContentParent as a result of the PGMPContent::Open call.
     if (mGetContentParentPromises.Length() == 1) {
--- a/dom/media/gmp/GMPService.cpp
+++ b/dom/media/gmp/GMPService.cpp
@@ -309,17 +309,18 @@ GeckoMediaPluginService::GetGMPAudioDeco
     return NS_ERROR_FAILURE;
   }
 
   GetGMPAudioDecoderCallback* rawCallback = aCallback.release();
   RefPtr<AbstractThread> thread(GetAbstractGMPThread());
   RefPtr<GMPCrashHelper> helper(aHelper);
   GetContentParent(aHelper, aNodeId, NS_LITERAL_CSTRING(GMP_API_AUDIO_DECODER), *aTags)
     ->Then(thread, __func__,
-      [rawCallback, helper](RefPtr<GMPContentParent> parent) {
+      [rawCallback, helper](RefPtr<GMPContentParent::CloseBlocker> wrapper) {
+        RefPtr<GMPContentParent> parent = wrapper->mParent;
         UniquePtr<GetGMPAudioDecoderCallback> callback(rawCallback);
         GMPAudioDecoderParent* actor = nullptr;
         if (parent && NS_SUCCEEDED(parent->GetGMPAudioDecoder(&actor))) {
           actor->SetCrashHelper(helper);
         }
         callback->Done(actor);
       },
       [rawCallback] {
@@ -345,17 +346,18 @@ GeckoMediaPluginService::GetDecryptingGM
     return NS_ERROR_FAILURE;
   }
 
   GetGMPVideoDecoderCallback* rawCallback = aCallback.release();
   RefPtr<AbstractThread> thread(GetAbstractGMPThread());
   RefPtr<GMPCrashHelper> helper(aHelper);
   GetContentParent(aHelper, aNodeId, NS_LITERAL_CSTRING(GMP_API_VIDEO_DECODER), *aTags)
     ->Then(thread, __func__,
-      [rawCallback, helper, aDecryptorId](RefPtr<GMPContentParent> parent) {
+      [rawCallback, helper, aDecryptorId](RefPtr<GMPContentParent::CloseBlocker> wrapper) {
+        RefPtr<GMPContentParent> parent = wrapper->mParent;
         UniquePtr<GetGMPVideoDecoderCallback> callback(rawCallback);
         GMPVideoDecoderParent* actor = nullptr;
         GMPVideoHostImpl* host = nullptr;
         if (parent && NS_SUCCEEDED(parent->GetGMPVideoDecoder(&actor, aDecryptorId))) {
           host = &(actor->Host());
           actor->SetCrashHelper(helper);
         }
         callback->Done(actor, host);
@@ -382,17 +384,18 @@ GeckoMediaPluginService::GetGMPVideoEnco
     return NS_ERROR_FAILURE;
   }
 
   GetGMPVideoEncoderCallback* rawCallback = aCallback.release();
   RefPtr<AbstractThread> thread(GetAbstractGMPThread());
   RefPtr<GMPCrashHelper> helper(aHelper);
   GetContentParent(aHelper, aNodeId, NS_LITERAL_CSTRING(GMP_API_VIDEO_ENCODER), *aTags)
     ->Then(thread, __func__,
-      [rawCallback, helper](RefPtr<GMPContentParent> parent) {
+      [rawCallback, helper](RefPtr<GMPContentParent::CloseBlocker> wrapper) {
+        RefPtr<GMPContentParent> parent = wrapper->mParent;
         UniquePtr<GetGMPVideoEncoderCallback> callback(rawCallback);
         GMPVideoEncoderParent* actor = nullptr;
         GMPVideoHostImpl* host = nullptr;
         if (parent && NS_SUCCEEDED(parent->GetGMPVideoEncoder(&actor))) {
           host = &(actor->Host());
           actor->SetCrashHelper(helper);
         }
         callback->Done(actor, host);
@@ -427,17 +430,18 @@ GeckoMediaPluginService::GetGMPDecryptor
     return NS_ERROR_FAILURE;
   }
 
   GetGMPDecryptorCallback* rawCallback = aCallback.release();
   RefPtr<AbstractThread> thread(GetAbstractGMPThread());
   RefPtr<GMPCrashHelper> helper(aHelper);
   GetContentParent(aHelper, aNodeId, NS_LITERAL_CSTRING(GMP_API_DECRYPTOR), *aTags)
     ->Then(thread, __func__,
-      [rawCallback, helper](RefPtr<GMPContentParent> parent) {
+      [rawCallback, helper](RefPtr<GMPContentParent::CloseBlocker> wrapper) {
+        RefPtr<GMPContentParent> parent = wrapper->mParent;
         UniquePtr<GetGMPDecryptorCallback> callback(rawCallback);
         GMPDecryptorParent* actor = nullptr;
         if (parent && NS_SUCCEEDED(parent->GetGMPDecryptor(&actor))) {
           actor->SetCrashHelper(helper);
         }
         callback->Done(actor);
       },
       [rawCallback] {
--- a/dom/media/gmp/GMPService.h
+++ b/dom/media/gmp/GMPService.h
@@ -17,29 +17,29 @@
 #include "nsIThread.h"
 #include "nsThreadUtils.h"
 #include "nsIDocument.h"
 #include "nsIWeakReference.h"
 #include "mozilla/AbstractThread.h"
 #include "nsClassHashtable.h"
 #include "nsISupportsImpl.h"
 #include "mozilla/MozPromise.h"
+#include "GMPContentParent.h"
 
 template <class> struct already_AddRefed;
 
 namespace mozilla {
 
 class GMPCrashHelper;
 
 extern LogModule* GetGMPLog();
 
 namespace gmp {
 
-class GMPContentParent;
-typedef MozPromise<RefPtr<GMPContentParent>, nsresult, /* IsExclusive = */ true> GetGMPContentParentPromise;
+typedef MozPromise<RefPtr<GMPContentParent::CloseBlocker>, nsresult, /* IsExclusive = */ true> GetGMPContentParentPromise;
 
 class GeckoMediaPluginService : public mozIGeckoMediaPluginService
                               , public nsIObserver
 {
 public:
   static already_AddRefed<GeckoMediaPluginService> GetGeckoMediaPluginService();
 
   virtual nsresult Init();
--- a/dom/media/gmp/GMPServiceChild.cpp
+++ b/dom/media/gmp/GMPServiceChild.cpp
@@ -100,17 +100,18 @@ GeckoMediaPluginServiceChild::GetContent
       }
 
       RefPtr<GMPContentParent> parent;
       child->GetBridgedGMPContentParent(otherProcess, getter_AddRefs(parent));
       if (!alreadyBridgedTo.Contains(otherProcess)) {
         parent->SetDisplayName(displayName);
         parent->SetPluginId(pluginId);
       }
-      holder->Resolve(parent, __func__);
+      RefPtr<GMPContentParent::CloseBlocker> blocker(new GMPContentParent::CloseBlocker(parent));
+      holder->Resolve(blocker, __func__);
     },
     [rawHolder](nsresult rv) {
       UniquePtr<MozPromiseHolder<GetGMPContentParentPromise>> holder(rawHolder);
       holder->Reject(rv, __func__);
     });
 
   return promise;
 }