Bug 1507093 - P3. Ensure that self AddRef/DeRef occur sequentially r=pehrsons
authorJean-Yves Avenard <jyavenard@mozilla.com>
Thu, 15 Nov 2018 12:48:11 +0000
changeset 446569 b8066bd5bb6606bc343671c19bc8231492b277a8
parent 446568 aa7084b3c14b8500210e7065ffd172f1506f0cd4
child 446570 d1e2bb18d628e3b5cd6ed47e4a9adf55a8cdcfd1
push id35043
push userebalazs@mozilla.com
push dateThu, 15 Nov 2018 16:12:36 +0000
treeherdermozilla-central@59026ada59bd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspehrsons
bugs1507093
milestone65.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 1507093 - P3. Ensure that self AddRef/DeRef occur sequentially r=pehrsons It was possible for two sequential calls to HTMLMediaElement::AddRemoveSelfReference to leave the media element deregistered when it should have registered. And we ensure to ony ever self register once. Differential Revision: https://phabricator.services.mozilla.com/D11859
dom/html/HTMLMediaElement.cpp
dom/html/HTMLMediaElement.h
--- a/dom/html/HTMLMediaElement.cpp
+++ b/dom/html/HTMLMediaElement.cpp
@@ -3868,32 +3868,49 @@ public:
     mWeak = aPtr;
     nsContentUtils::RegisterShutdownObserver(this);
     mPhase = Phase::Subscribed;
   }
   void Unsubscribe()
   {
     MOZ_DIAGNOSTIC_ASSERT(mPhase == Phase::Subscribed);
     MOZ_DIAGNOSTIC_ASSERT(mWeak);
+    MOZ_DIAGNOSTIC_ASSERT(!mAddRefed,
+                          "ReleaseMediaElement should have been called first");
     mWeak = nullptr;
     nsContentUtils::UnregisterShutdownObserver(this);
     mPhase = Phase::Unsubscribed;
   }
-  void AddRefMediaElement() { mWeak->AddRef(); }
-  void ReleaseMediaElement() { mWeak->Release(); }
+  void AddRefMediaElement()
+  {
+    MOZ_DIAGNOSTIC_ASSERT(mWeak);
+    MOZ_DIAGNOSTIC_ASSERT(!mAddRefed, "Should only ever AddRef once");
+    mWeak->AddRef();
+    mAddRefed = true;
+  }
+  void ReleaseMediaElement()
+  {
+    MOZ_DIAGNOSTIC_ASSERT(mWeak);
+    MOZ_DIAGNOSTIC_ASSERT(mAddRefed, "Should only release after AddRef");
+    mWeak->Release();
+    mAddRefed = false;
+  }
 
 private:
   virtual ~ShutdownObserver()
   {
     MOZ_DIAGNOSTIC_ASSERT(mPhase == Phase::Unsubscribed);
     MOZ_DIAGNOSTIC_ASSERT(!mWeak);
+    MOZ_DIAGNOSTIC_ASSERT(!mAddRefed,
+                          "ReleaseMediaElement should have been called first");
   }
   // Guaranteed to be valid by HTMLMediaElement.
   HTMLMediaElement* mWeak = nullptr;
   Phase mPhase = Phase::Init;
+  bool mAddRefed = false;
 };
 
 NS_IMPL_ISUPPORTS(HTMLMediaElement::ShutdownObserver, nsIObserver)
 
 HTMLMediaElement::HTMLMediaElement(
   already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo)
   : nsGenericHTMLElement(std::move(aNodeInfo))
   , mWatchManager(this, OwnerDoc()->AbstractMainThreadFor(TaskCategory::Other))
@@ -6796,36 +6813,32 @@ HTMLMediaElement::AddRemoveSelfReference
     !mShuttingDown && ownerDoc->IsActive() &&
     (mDelayingLoadEvent || (!mPaused && mDecoder && !mDecoder->IsEnded()) ||
      (!mPaused && mSrcStream && !mSrcStream->IsFinished()) ||
      (mDecoder && mDecoder->IsSeeking()) || CanActivateAutoplay() ||
      (mMediaSource ? mProgressTimer : mNetworkState == NETWORK_LOADING));
 
   if (needSelfReference != mHasSelfReference) {
     mHasSelfReference = needSelfReference;
+    RefPtr<HTMLMediaElement> self = this;
     if (needSelfReference) {
       // The shutdown observer will hold a strong reference to us. This
       // will do to keep us alive. We need to know about shutdown so that
       // we can release our self-reference.
-      mShutdownObserver->AddRefMediaElement();
+      mMainThreadEventTarget->Dispatch(NS_NewRunnableFunction(
+        "dom::HTMLMediaElement::AddSelfReference",
+        [self]() { self->mShutdownObserver->AddRefMediaElement(); }));
     } else {
       // Dispatch Release asynchronously so that we don't destroy this object
       // inside a call stack of method calls on this object
-      mMainThreadEventTarget->Dispatch(
-        NewRunnableMethod("dom::HTMLMediaElement::DoRemoveSelfReference",
-                          this,
-                          &HTMLMediaElement::DoRemoveSelfReference));
-    }
-  }
-}
-
-void
-HTMLMediaElement::DoRemoveSelfReference()
-{
-  mShutdownObserver->ReleaseMediaElement();
+      mMainThreadEventTarget->Dispatch(NS_NewRunnableFunction(
+        "dom::HTMLMediaElement::AddSelfReference",
+        [self]() { self->mShutdownObserver->ReleaseMediaElement(); }));
+    }
+  }
 }
 
 void
 HTMLMediaElement::NotifyShutdownEvent()
 {
   mShuttingDown = true;
   // Since target thread had been shutdown, it's no chance to execute the Then()
   // afterward. Therefore, we should disconnect the request.
--- a/dom/html/HTMLMediaElement.h
+++ b/dom/html/HTMLMediaElement.h
@@ -1116,21 +1116,16 @@ protected:
                              uint32_t aFlags);
 
   /**
    * Call this to reevaluate whether we should be holding a self-reference.
    */
   void AddRemoveSelfReference();
 
   /**
-   * Called asynchronously to release a self-reference to this element.
-   */
-  void DoRemoveSelfReference();
-
-  /**
    * Called when "xpcom-shutdown" event is received.
    */
   void NotifyShutdownEvent();
 
   /**
    * Possible values of the 'preload' attribute.
    */
   enum PreloadAttrValue : uint8_t {