Bug 1522547 - Have MediaKeys observe if the owning document becomes inactive so it can shutdown. r=cpearce
authorBryce Van Dyk <bvandyk@mozilla.com>
Tue, 05 Mar 2019 16:21:40 +0000
changeset 520333 062f54735111d741f9189f2c15e2b4da740783fa
parent 520332 153245ec86c26f88beba43d7858cf5c517e22d84
child 520334 e39ea97ade1e49c0cbd78eff03161020b15a02ac
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscpearce
bugs1522547
milestone67.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 1522547 - Have MediaKeys observe if the owning document becomes inactive so it can shutdown. r=cpearce MediaKeys objects are typically created and associated with an HTMLMediaElement, however it is possible to create a MediaKeys object and not associate it with an HTMLMediaElement. This resulted in an issue where these MediaKeys would keep alive other components that would assert during bowrser shutdown (see bug 1522547). We anticipated that MediaKeys associated with an HTMLMediaElement would need to be shutdown if their owning document became inactive, but were not handling the case where the keys never became associated with an element. This patch has the MediaKeys listen directly to their owning document for activity change. The MediaKeys will shutdown if their document becomes inactive. This avoids MediaKeys not associated with HTMLMediaElements keeping other objects alive during browser shutdown. Differential Revision: https://phabricator.services.mozilla.com/D21983
dom/html/HTMLMediaElement.cpp
dom/media/eme/MediaKeys.cpp
dom/media/eme/MediaKeys.h
--- a/dom/html/HTMLMediaElement.cpp
+++ b/dom/html/HTMLMediaElement.cpp
@@ -6022,17 +6022,18 @@ void HTMLMediaElement::NotifyOwnerDocume
     NotifyDecoderActivityChanges();
   }
 
   bool pauseElement = ShouldElementBePaused();
   SuspendOrResumeElement(pauseElement, !IsActive());
 
   // If the owning document has become inactive we should shutdown the CDM.
   if (!OwnerDoc()->IsCurrentActiveDocument() && mMediaKeys) {
-    mMediaKeys->Shutdown();
+    // We don't shutdown MediaKeys here because it also listens for document
+    // activity and will take care of shutting down itself.
     DDUNLINKCHILD(mMediaKeys.get());
     mMediaKeys = nullptr;
     if (mDecoder) {
       ShutdownDecoder();
     }
   }
 
   AddRemoveSelfReference();
--- a/dom/media/eme/MediaKeys.cpp
+++ b/dom/media/eme/MediaKeys.cpp
@@ -30,41 +30,92 @@
 #include "mozilla/dom/MediaKeySystemAccess.h"
 #include "nsPrintfCString.h"
 #include "ChromiumCDMProxy.h"
 
 namespace mozilla {
 
 namespace dom {
 
-NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(MediaKeys, mElement, mParent,
-                                      mKeySessions, mPromises,
-                                      mPendingSessions);
+// We don't use NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE because we need to
+// unregister our MediaKeys from mDocument's activity listeners. If we don't do
+// this then cycle collection can null mDocument before our dtor runs and the
+// observer ptr held by mDocument will dangle.
+NS_IMPL_CYCLE_COLLECTION_CLASS(MediaKeys)
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(MediaKeys)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mElement)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mParent)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mKeySessions)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPromises)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPendingSessions)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDocument)
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
+NS_IMPL_CYCLE_COLLECTION_TRACE_WRAPPERCACHE(MediaKeys)
+
+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(MediaKeys)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mElement)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mParent)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mKeySessions)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mPromises)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mPendingSessions)
+  tmp->UnregisterActivityObserver();
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mDocument)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
+
 NS_IMPL_CYCLE_COLLECTING_ADDREF(MediaKeys)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(MediaKeys)
 NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(MediaKeys)
   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
-  NS_INTERFACE_MAP_ENTRY(nsISupports)
+  NS_INTERFACE_MAP_ENTRY(nsIDocumentActivity)
 NS_INTERFACE_MAP_END
 
 MediaKeys::MediaKeys(nsPIDOMWindowInner* aParent, const nsAString& aKeySystem,
                      const MediaKeySystemConfiguration& aConfig)
     : mParent(aParent),
       mKeySystem(aKeySystem),
       mCreatePromiseId(0),
       mConfig(aConfig) {
   EME_LOG("MediaKeys[%p] constructed keySystem=%s", this,
           NS_ConvertUTF16toUTF8(mKeySystem).get());
 }
 
 MediaKeys::~MediaKeys() {
+  UnregisterActivityObserver();
+  mDocument = nullptr;
   Shutdown();
   EME_LOG("MediaKeys[%p] destroyed", this);
 }
 
+void MediaKeys::RegisterActivityObserver() {
+  MOZ_ASSERT(mDocument);
+  if (mDocument) {
+    mDocument->RegisterActivityObserver(this);
+  }
+}
+
+void MediaKeys::UnregisterActivityObserver() {
+  if (mDocument) {
+    mDocument->UnregisterActivityObserver(this);
+  }
+}
+
+// NS_DECL_NSIDOCUMENTACTIVITY
+void MediaKeys::NotifyOwnerDocumentActivityChanged() {
+  EME_LOG("MediaKeys[%p] NotifyOwnerDocumentActivityChanged()", this);
+  // If our owning document is no longer active we should shutdown.
+  if (!mDocument->IsCurrentActiveDocument()) {
+    EME_LOG(
+        "MediaKeys[%p] NotifyOwnerDocumentActivityChanged() owning document is "
+        "not active, shutting down!",
+        this);
+    Shutdown();
+  }
+}
+
 void MediaKeys::Terminated() {
   EME_LOG("MediaKeys[%p] CDM crashed unexpectedly", this);
 
   KeySessionHashMap keySessions;
   // Remove entries during iteration will screw it. Make a copy first.
   for (auto iter = mKeySessions.Iter(); !iter.Done(); iter.Next()) {
     RefPtr<MediaKeySession>& session = iter.Data();
     keySessions.Put(session->GetSessionId(), session);
@@ -373,17 +424,19 @@ already_AddRefed<DetailedPromise> MediaK
   nsCOMPtr<nsPIDOMWindowOuter> top = window->GetOuterWindow()->GetTop();
   if (!top || !top->GetExtantDoc()) {
     promise->MaybeReject(
         NS_ERROR_DOM_INVALID_STATE_ERR,
         NS_LITERAL_CSTRING("Couldn't get document in MediaKeys::Init"));
     return promise.forget();
   }
 
-  mTopLevelPrincipal = top->GetExtantDoc()->NodePrincipal();
+  mDocument = top->GetExtantDoc();
+
+  mTopLevelPrincipal = mDocument->NodePrincipal();
 
   if (!mPrincipal || !mTopLevelPrincipal) {
     NS_WARNING("Failed to get principals when creating MediaKeys");
     promise->MaybeReject(
         NS_ERROR_DOM_INVALID_STATE_ERR,
         NS_LITERAL_CSTRING("Couldn't get principal(s) in MediaKeys::Init"));
     return promise.forget();
   }
@@ -424,16 +477,18 @@ already_AddRefed<DetailedPromise> MediaK
   MOZ_ASSERT(!mCreatePromiseId, "Should only be created once!");
   mCreatePromiseId = StorePromise(promise);
   EME_LOG("MediaKeys[%p]::Init() calling AddRef()", this);
   AddRef();
   mProxy->Init(mCreatePromiseId, NS_ConvertUTF8toUTF16(origin),
                NS_ConvertUTF8toUTF16(topLevelOrigin),
                KeySystemToGMPName(mKeySystem));
 
+  RegisterActivityObserver();
+
   return promise.forget();
 }
 
 void MediaKeys::OnCDMCreated(PromiseId aId, const uint32_t aPluginId) {
   EME_LOG("MediaKeys[%p]::OnCDMCreated(aId=%" PRIu32 ", aPluginId=%" PRIu32 ")",
           this, aId, aPluginId);
   RefPtr<DetailedPromise> promise(RetrievePromise(aId));
   if (!promise) {
--- a/dom/media/eme/MediaKeys.h
+++ b/dom/media/eme/MediaKeys.h
@@ -9,16 +9,17 @@
 
 #include "DecoderDoctorLogger.h"
 #include "nsWrapperCache.h"
 #include "nsISupports.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/RefPtr.h"
 #include "nsCOMPtr.h"
 #include "nsCycleCollectionParticipant.h"
+#include "nsIDocumentActivity.h"
 #include "nsRefPtrHashtable.h"
 #include "mozilla/dom/Promise.h"
 #include "mozilla/dom/MediaKeysBinding.h"
 #include "mozilla/dom/MediaKeyStatusMapBinding.h"  // For MediaKeyStatus
 #include "mozilla/dom/MediaKeySystemAccessBinding.h"
 #include "mozIGeckoMediaPluginService.h"
 #include "mozilla/DetailedPromise.h"
 #include "mozilla/WeakPtr.h"
@@ -43,26 +44,29 @@ typedef nsRefPtrHashtable<nsStringHashKe
 typedef nsRefPtrHashtable<nsUint32HashKey, dom::DetailedPromise> PromiseHashMap;
 typedef nsRefPtrHashtable<nsUint32HashKey, MediaKeySession>
     PendingKeySessionsHashMap;
 typedef nsDataHashtable<nsUint32HashKey, uint32_t> PendingPromiseIdTokenHashMap;
 typedef uint32_t PromiseId;
 
 // This class is used on the main thread only.
 // Note: its addref/release is not (and can't be) thread safe!
-class MediaKeys final : public nsISupports,
+class MediaKeys final : public nsIDocumentActivity,
                         public nsWrapperCache,
                         public SupportsWeakPtr<MediaKeys>,
                         public DecoderDoctorLifeLogger<MediaKeys> {
   ~MediaKeys();
 
  public:
   NS_DECL_CYCLE_COLLECTING_ISUPPORTS
   NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(MediaKeys)
   MOZ_DECLARE_WEAKREFERENCE_TYPENAME(MediaKeys)
+  // We want to listen to the owning document so we can shutdown if it goes
+  // inactive.
+  NS_DECL_NSIDOCUMENTACTIVITY
 
   MediaKeys(nsPIDOMWindowInner* aParentWindow, const nsAString& aKeySystem,
             const MediaKeySystemConfiguration& aConfig);
 
   already_AddRefed<DetailedPromise> Init(ErrorResult& aRv);
 
   nsPIDOMWindowInner* GetParentObject() const;
 
@@ -151,21 +155,25 @@ class MediaKeys final : public nsISuppor
   // Instantiate CDMProxy instance.
   // It could be MediaDrmCDMProxy (Widevine on Fennec) or ChromiumCDMProxy (the
   // rest).
   already_AddRefed<CDMProxy> CreateCDMProxy(nsIEventTarget* aMainThread);
 
   // Removes promise from mPromises, and returns it.
   already_AddRefed<DetailedPromise> RetrievePromise(PromiseId aId);
 
+  void RegisterActivityObserver();
+  void UnregisterActivityObserver();
+
   // Owning ref to proxy. The proxy has a weak reference back to the MediaKeys,
   // and the MediaKeys destructor clears the proxy's reference to the MediaKeys.
   RefPtr<CDMProxy> mProxy;
 
   RefPtr<HTMLMediaElement> mElement;
+  RefPtr<Document> mDocument;
 
   nsCOMPtr<nsPIDOMWindowInner> mParent;
   const nsString mKeySystem;
   KeySessionHashMap mKeySessions;
   PromiseHashMap mPromises;
   PendingKeySessionsHashMap mPendingSessions;
   PromiseId mCreatePromiseId;