Bug 1135424 - Allow MediaPromise dispatch to fail if the ThenValue has been disconnected. r=mattwoodrow
☠☠ backed out by 1e99f9913951 ☠ ☠
authorBobby Holley <bobbyholley@gmail.com>
Wed, 11 Mar 2015 11:29:50 -0700
changeset 263771 e52401d30a67e97c77ab891565446053b0b01533
parent 263770 05848c0df2f87e975bb53d39c44fab02876a22a0
child 263772 d86806ea63f405368cdd9e5dc071e2ceb9c74df5
push id830
push userraliiev@mozilla.com
push dateFri, 19 Jun 2015 19:24:37 +0000
treeherdermozilla-release@932614382a68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmattwoodrow
bugs1135424
milestone39.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 1135424 - Allow MediaPromise dispatch to fail if the ThenValue has been disconnected. r=mattwoodrow The original idea behind the current model was that we wanted ironclad guarantees that consumers would always get a callback on their promise. But we now have use cases where the consumer wants to forget about a promise (using the new Disconnect()) feature, and in some cases wants to shut down the task queue that the response is going to be dispatched on. In the case of this bug, we want to avoid waiting for the longest outstanding timer promise to be resolved before shutting down the MDSM. So this patch fixes up the pieces needed to make this work: * Loosening our invariants to allow dispatch targets to be released on any thread, since MediaTaskQueue and nsIEventTarget both have thread-safe refcounting. * Releasing mThisVal in Disconnect, so that we no longer depend on successful dispatch to release it on the correct (dispatch) thread. * Fiddling with various assertions. We also make some assertions fatal in nightly/aurora builds while we're at it.
dom/media/MediaPromise.h
--- a/dom/media/MediaPromise.h
+++ b/dom/media/MediaPromise.h
@@ -11,16 +11,17 @@
 
 #include "nsTArray.h"
 #include "nsThreadUtils.h"
 
 #include "mozilla/DebugOnly.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/Mutex.h"
 #include "mozilla/Monitor.h"
+#include "mozilla/unused.h"
 
 /* Polyfill __func__ on MSVC for consumers to pass to the MediaPromise API. */
 #ifdef _MSC_VER
 #define __func__ __FUNCTION__
 #endif
 
 class nsIEventTarget;
 namespace mozilla {
@@ -103,28 +104,21 @@ public:
     return Move(p);
   }
 
   class Consumer
   {
   public:
     NS_INLINE_DECL_THREADSAFE_REFCOUNTING(Consumer)
 
-    void Disconnect()
-    {
-      AssertOnDispatchThread();
-      MOZ_DIAGNOSTIC_ASSERT(!mComplete);
-      mDisconnected = true;
-    }
+    virtual void Disconnect() = 0;
 
-#ifdef DEBUG
-    virtual void AssertOnDispatchThread() = 0;
-#else
-    void AssertOnDispatchThread() {}
-#endif
+    // MSVC complains when an inner class (ThenValueBase::{Resolve,Reject}Runnable)
+    // tries to access an inherited protected member.
+    bool IsDisconnected() const { return mDisconnected; }
 
   protected:
     Consumer() : mComplete(false), mDisconnected(false) {}
     virtual ~Consumer() {}
 
     bool mComplete;
     bool mDisconnected;
   };
@@ -144,17 +138,17 @@ protected:
     {
     public:
       ResolveRunnable(ThenValueBase* aThenValue, ResolveValueType aResolveValue)
         : mThenValue(aThenValue)
         , mResolveValue(aResolveValue) {}
 
       ~ResolveRunnable()
       {
-        MOZ_ASSERT(!mThenValue);
+        MOZ_DIAGNOSTIC_ASSERT(!mThenValue || mThenValue->IsDisconnected());
       }
 
       NS_IMETHODIMP Run()
       {
         PROMISE_LOG("ResolveRunnable::Run() [this=%p]", this);
         mThenValue->DoResolve(mResolveValue);
         mThenValue = nullptr;
         return NS_OK;
@@ -169,17 +163,17 @@ protected:
     {
     public:
       RejectRunnable(ThenValueBase* aThenValue, RejectValueType aRejectValue)
         : mThenValue(aThenValue)
         , mRejectValue(aRejectValue) {}
 
       ~RejectRunnable()
       {
-        MOZ_ASSERT(!mThenValue);
+        MOZ_DIAGNOSTIC_ASSERT(!mThenValue || mThenValue->IsDisconnected());
       }
 
       NS_IMETHODIMP Run()
       {
         PROMISE_LOG("RejectRunnable::Run() [this=%p]", this);
         mThenValue->DoReject(mRejectValue);
         mThenValue = nullptr;
         return NS_OK;
@@ -247,71 +241,87 @@ protected:
       MOZ_ASSERT(!aPromise->IsPending());
       bool resolved = aPromise->mResolveValue.isSome();
       nsRefPtr<nsRunnable> runnable =
         resolved ? static_cast<nsRunnable*>(new (typename ThenValueBase::ResolveRunnable)(this, aPromise->mResolveValue.ref()))
                  : static_cast<nsRunnable*>(new (typename ThenValueBase::RejectRunnable)(this, aPromise->mRejectValue.ref()));
       PROMISE_LOG("%s Then() call made from %s [Runnable=%p, Promise=%p, ThenValue=%p]",
                   resolved ? "Resolving" : "Rejecting", ThenValueBase::mCallSite,
                   runnable.get(), aPromise, this);
-      DebugOnly<nsresult> rv = detail::DispatchMediaPromiseRunnable(mResponseTarget, runnable);
-      MOZ_ASSERT(NS_SUCCEEDED(rv));
+      nsresult rv = detail::DispatchMediaPromiseRunnable(mResponseTarget, runnable);
+      unused << rv;
+
+      // NB: mDisconnected is only supposed to be accessed on the dispatch
+      // thread. However, we require the consumer to have disconnected any
+      // oustanding promise requests _before_ initiating shutdown on the
+      // thread or task queue. So the only non-buggy scenario for dispatch
+      // failing involves the target thread being unable to manipulate the
+      // ThenValue (since it's been disconnected), so it's safe to read here.
+      MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv) || Consumer::mDisconnected);
     }
 
 #ifdef DEBUG
-  virtual void AssertOnDispatchThread() MOZ_OVERRIDE
+  void AssertOnDispatchThread()
   {
     detail::AssertOnThread(mResponseTarget);
   }
+#else
+  void AssertOnDispatchThread() {}
 #endif
 
+  virtual void Disconnect() MOZ_OVERRIDE
+  {
+    AssertOnDispatchThread();
+    MOZ_DIAGNOSTIC_ASSERT(!Consumer::mComplete);
+    Consumer::mDisconnected = true;
+
+    // If a Consumer has been disconnected, we don't guarantee that the
+    // resolve/reject runnable will be dispatched. Null out our refcounted
+    // this-value now so that it's released predictably on the dispatch thread.
+    mThisVal = nullptr;
+  }
+
   protected:
     virtual void DoResolve(ResolveValueType aResolveValue) MOZ_OVERRIDE
     {
       Consumer::mComplete = true;
       if (Consumer::mDisconnected) {
+        MOZ_ASSERT(!mThisVal);
         PROMISE_LOG("ThenValue::DoResolve disconnected - bailing out [this=%p]", this);
-        // Null these out for the same reasons described below.
-        mResponseTarget = nullptr;
-        mThisVal = nullptr;
         return;
       }
       InvokeCallbackMethod(mThisVal.get(), mResolveMethod, aResolveValue);
 
-      // Null these out after invoking the callback so that any references are
-      // released predictably on the target thread. Otherwise, they would be
+      // Null out mThisVal after invoking the callback so that any references are
+      // released predictably on the dispatch thread. Otherwise, it would be
       // released on whatever thread last drops its reference to the ThenValue,
       // which may or may not be ok.
-      mResponseTarget = nullptr;
       mThisVal = nullptr;
     }
 
     virtual void DoReject(RejectValueType aRejectValue) MOZ_OVERRIDE
     {
       Consumer::mComplete = true;
       if (Consumer::mDisconnected) {
+        MOZ_ASSERT(!mThisVal);
         PROMISE_LOG("ThenValue::DoReject disconnected - bailing out [this=%p]", this);
-        // Null these out for the same reasons described below.
-        mResponseTarget = nullptr;
-        mThisVal = nullptr;
         return;
       }
       InvokeCallbackMethod(mThisVal.get(), mRejectMethod, aRejectValue);
 
-      // Null these out after invoking the callback so that any references are
-      // released predictably on the target thread. Otherwise, they would be
+      // Null out mThisVal after invoking the callback so that any references are
+      // released predictably on the dispatch thread. Otherwise, it would be
       // released on whatever thread last drops its reference to the ThenValue,
       // which may or may not be ok.
-      mResponseTarget = nullptr;
       mThisVal = nullptr;
     }
 
   private:
-    nsRefPtr<TargetType> mResponseTarget;
-    nsRefPtr<ThisType> mThisVal;
+    nsRefPtr<TargetType> mResponseTarget; // May be released on any thread.
+    nsRefPtr<ThisType> mThisVal; // Only accessed and refcounted on dispatch thread.
     ResolveMethodType mResolveMethod;
     RejectMethodType mRejectMethod;
   };
 public:
 
   template<typename TargetType, typename ThisType,
            typename ResolveMethodType, typename RejectMethodType>
   already_AddRefed<Consumer> RefableThen(TargetType* aResponseTarget, const char* aCallSite, ThisType* aThisVal,
@@ -656,17 +666,17 @@ private:
 template<typename PromiseType, typename TargetType>
 static nsRefPtr<PromiseType>
 ProxyInternal(TargetType* aTarget, MethodCallBase<PromiseType>* aMethodCall, const char* aCallerName)
 {
   nsRefPtr<typename PromiseType::Private> p = new (typename PromiseType::Private)(aCallerName);
   nsRefPtr<ProxyRunnable<PromiseType>> r = new ProxyRunnable<PromiseType>(p, aMethodCall);
   nsresult rv = detail::DispatchMediaPromiseRunnable(aTarget, r);
   MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv));
-  (void) rv; // Avoid compilation failures in builds with MOZ_DIAGNOSTIC_ASSERT disabled.
+  unused << rv;
   return Move(p);
 }
 
 } // namespace detail
 
 template<typename PromiseType, typename TargetType, typename ThisType>
 static nsRefPtr<PromiseType>
 ProxyMediaCall(TargetType* aTarget, ThisType* aThisVal, const char* aCallerName,