Bug 1315850 - Ask the GMPService for the GMP thread in GMPParent::ChildTerminated. r=gerald

When we shutdown the browser while the GMPService is active we can end up
leaking a GMPParent, GeckoMediaPluginServiceParent, and a Runnable. I tracked
this down to the runnable dispatched to the GMP thread in
GMPParent::ChildTerminated(). The dispatch of this runnable is failing as we
are dispatching the runnable to a reference of the GMP thread which we have
previously acquired, but that thread is now shutdown. So the dispatch fails,
and if you look in nsThread::DispatchInternal() you'll see that we deliberately
leak the runnable if dispatch fails! The runnable leaking means that the
references it holds to the GMPParent and the GMP service parent leak.

The solution in this patch is to not cache a reference to the GMP thread on the
GMPParent; instead we re-request the GMP thread from the GMPService when we
want it. This means that in the case where the browser is shutting down,
GMPParent::GMPThread() will return null, and we'll not leak the runnable. We'll
then follow the (hacky) shutdown path added in bug 1163239.

We also need to change GMPParent::GMPThread() and GMPContentParent::GMPThread()
to return a reference to the GMP thread with a refcount held on it, in order
to ensure we don't race with the GMP service shutting down the GMP thread
while we're trying to dispatch to in on shutdown.

MozReview-Commit-ID: CXv9VZqTRzY

--HG--
extra : rebase_source : e507e48ee633cad8911287fb7296bbb1679a7bcb

dom/media/gmp/GMPContentParent.cpp

@@ -167,7 +167,7 @@ GMPContentParent::GetGMPDecryptor(GMPDecryptorParent** aGMPDP)
   return NS_OK;
 }
 
-nsIThread*
+nsCOMPtr<nsIThread>
 GMPContentParent::GMPThread()
 {
   if (!mGMPThread) {

dom/media/gmp/GMPContentParent.h

@@ -40,7 +40,7 @@ public:
   already_AddRefed<ChromiumCDMParent> GetChromiumCDM();
   void ChromiumCDMDestroyed(ChromiumCDMParent* aCDM);
 
-  nsIThread* GMPThread();
+  nsCOMPtr<nsIThread> GMPThread();
 
   // GMPSharedMem
   void CheckThread() override;

dom/media/gmp/GMPDecryptorParent.h

@@ -119,7 +119,7 @@ private:
   uint32_t mPluginId;
   GMPDecryptorProxyCallback* mCallback;
 #ifdef DEBUG
-  nsIThread* const mGMPThread;
+  nsCOMPtr<nsIThread> const mGMPThread;
 #endif
 };
 

dom/media/gmp/GMPParent.cpp

@@ -324,7 +324,7 @@ void
 GMPParent::ChildTerminated()
 {
   RefPtr<GMPParent> self(this);
-  nsIThread* gmpThread = GMPThread();
+  nsCOMPtr<nsIThread> gmpThread = GMPThread();
 
   if (!gmpThread) {
     // Bug 1163239 - this can happen on shutdown.
@@ -373,26 +373,20 @@ GMPParent::State() const
   return mState;
 }
 
-// Not changing to use mService since we'll be removing it
-nsIThread*
+nsCOMPtr<nsIThread>
 GMPParent::GMPThread()
 {
-  if (!mGMPThread) {
-    nsCOMPtr<mozIGeckoMediaPluginService> mps = do_GetService("@mozilla.org/gecko-media-plugin-service;1");
-    MOZ_ASSERT(mps);
-    if (!mps) {
-      return nullptr;
-    }
-    // Not really safe if we just grab to the mGMPThread, as we don't know
-    // what thread we're running on and other threads may be trying to
-    // access this without locks!  However, debug only, and primary failure
-    // mode outside of compiler-helped TSAN is a leak.  But better would be
-    // to use swap() under a lock.
-    mps->GetThread(getter_AddRefs(mGMPThread));
-    MOZ_ASSERT(mGMPThread);
+  nsCOMPtr<mozIGeckoMediaPluginService> mps =
+    do_GetService("@mozilla.org/gecko-media-plugin-service;1");
+  MOZ_ASSERT(mps);
+  if (!mps) {
+    return nullptr;
   }
-
-  return mGMPThread;
+  // Note: GeckoMediaPluginService::GetThread() is threadsafe, and returns
+  // nullptr if the GeckoMediaPluginService has started shutdown.
+  nsCOMPtr<nsIThread> gmpThread;
+  mps->GetThread(getter_AddRefs(gmpThread));
+  return gmpThread;
 }
 
 /* static */
@@ -590,7 +584,8 @@ GMPParent::RecvPGMPTimerConstructor(PGMPTimerParent* actor)
 PGMPTimerParent*
 GMPParent::AllocPGMPTimerParent()
 {
-  GMPTimerParent* p = new GMPTimerParent(GMPThread());
+  nsCOMPtr<nsIThread> thread = GMPThread();
+  GMPTimerParent* p = new GMPTimerParent(thread);
   mTimers.AppendElement(p); // Released in DeallocPGMPTimerParent, or on shutdown.
   return p;
 }

dom/media/gmp/GMPParent.h

@@ -100,7 +100,7 @@ public:
   void DeleteProcess();
 
   GMPState State() const;
-  nsIThread* GMPThread();
+  nsCOMPtr<nsIThread> GMPThread();
 
   // A GMP can either be a single instance shared across all NodeIds (like
   // in the OpenH264 case), or we can require a new plugin instance for every
@@ -207,7 +207,6 @@ private:
 
   nsTArray<RefPtr<GMPTimerParent>> mTimers;
   nsTArray<RefPtr<GMPStorageParent>> mStorage;
-  nsCOMPtr<nsIThread> mGMPThread;
   // NodeId the plugin is assigned to, or empty if the the plugin is not
   // assigned to a NodeId.
   nsCString mNodeId;

dom/media/gmp/GMPVideoDecoderParent.cpp

@@ -209,7 +209,8 @@ GMPVideoDecoderParent::Reset()
     LogToBrowserConsole(NS_LITERAL_STRING("GMPVideoDecoderParent timed out waiting for ResetComplete()"));
   });
   CancelResetCompleteTimeout();
-  mResetCompleteTimeout = SimpleTimer::Create(task, 5000, mPlugin->GMPThread());
+  nsCOMPtr<nsIThread> thread = mPlugin->GMPThread();
+  mResetCompleteTimeout = SimpleTimer::Create(task, 5000, thread);
 
   // Async IPC, we don't have access to a return value.
   return NS_OK;