Bug 1126465 - Make ThenValueBase inherit from a publicly-usable type, and refcount it. r=mattwoodrow
☠☠ backed out by 3f102935baa3 ☠ ☠
authorBobby Holley <bobbyholley@gmail.com>
Wed, 28 Jan 2015 18:54:10 -0800
changeset 239794 22aff144837610bf7b1d4feed8019ad868ccd5ac
parent 239793 212c4e3377f84b1c3ebbd59cdd085241de92b0f7
child 239795 d8045d89bfda2261c70d9f5937b6959f1ea7f450
push id507
push usermconley@mozilla.com
push dateThu, 29 Jan 2015 19:16:52 +0000
reviewersmattwoodrow
bugs1126465
milestone38.0a1
Bug 1126465 - Make ThenValueBase inherit from a publicly-usable type, and refcount it. r=mattwoodrow
dom/media/MediaPromise.h
--- a/dom/media/MediaPromise.h
+++ b/dom/media/MediaPromise.h
@@ -76,25 +76,34 @@ public:
   static nsRefPtr<MediaPromise>
   CreateAndReject(RejectValueType aRejectValue, const char* aRejectSite)
   {
     nsRefPtr<MediaPromise> p = new MediaPromise(aRejectSite);
     p->Reject(aRejectValue, aRejectSite);
     return p;
   }
 
+  class Consumer
+  {
+  public:
+    NS_INLINE_DECL_THREADSAFE_REFCOUNTING(Consumer)
+  protected:
+    Consumer() {}
+    virtual ~Consumer() {}
+  };
+
 protected:
 
   /*
    * A ThenValue tracks a single consumer waiting on the promise. When a consumer
    * invokes promise->Then(...), a ThenValue is created. Once the Promise is
    * resolved or rejected, a {Resolve,Reject}Runnable is dispatched, which
    * invokes the resolve/reject method and then deletes the ThenValue.
    */
-  class ThenValueBase
+  class ThenValueBase : public Consumer
   {
   public:
     class ResolveRunnable : public nsRunnable
     {
     public:
       ResolveRunnable(ThenValueBase* aThenValue, ResolveValueType aResolveValue)
         : mThenValue(aThenValue)
         , mResolveValue(aResolveValue) {}
@@ -103,24 +112,22 @@ protected:
       {
         MOZ_ASSERT(!mThenValue);
       }
 
       NS_IMETHODIMP Run()
       {
         PROMISE_LOG("ResolveRunnable::Run() [this=%p]", this);
         mThenValue->DoResolve(mResolveValue);
-
-        delete mThenValue;
         mThenValue = nullptr;
         return NS_OK;
       }
 
     private:
-      ThenValueBase* mThenValue;
+      nsRefPtr<ThenValueBase> mThenValue;
       ResolveValueType mResolveValue;
     };
 
     class RejectRunnable : public nsRunnable
     {
     public:
       RejectRunnable(ThenValueBase* aThenValue, RejectValueType aRejectValue)
         : mThenValue(aThenValue)
@@ -130,38 +137,30 @@ protected:
       {
         MOZ_ASSERT(!mThenValue);
       }
 
       NS_IMETHODIMP Run()
       {
         PROMISE_LOG("RejectRunnable::Run() [this=%p]", this);
         mThenValue->DoReject(mRejectValue);
-
-        delete mThenValue;
         mThenValue = nullptr;
         return NS_OK;
       }
 
     private:
-      ThenValueBase* mThenValue;
+      nsRefPtr<ThenValueBase> mThenValue;
       RejectValueType mRejectValue;
     };
 
-    explicit ThenValueBase(const char* aCallSite) : mCallSite(aCallSite)
-    {
-      MOZ_COUNT_CTOR(ThenValueBase);
-    }
+    explicit ThenValueBase(const char* aCallSite) : mCallSite(aCallSite) {}
 
     virtual void Dispatch(MediaPromise *aPromise) = 0;
 
   protected:
-    // This may only be deleted by {Resolve,Reject}Runnable::Run.
-    virtual ~ThenValueBase() { MOZ_COUNT_DTOR(ThenValueBase); }
-
     virtual void DoResolve(ResolveValueType aResolveValue) = 0;
     virtual void DoReject(RejectValueType aRejectValue) = 0;
 
     const char* mCallSite;
   };
 
   /*
    * We create two overloads for invoking Resolve/Reject Methods so as to
@@ -217,47 +216,59 @@ protected:
       DebugOnly<nsresult> rv = detail::DispatchMediaPromiseRunnable(mResponseTarget, runnable);
       MOZ_ASSERT(NS_SUCCEEDED(rv));
     }
 
   protected:
     virtual void DoResolve(ResolveValueType aResolveValue) MOZ_OVERRIDE
     {
       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
+      // 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
     {
       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
+      // released on whatever thread last drops its reference to the ThenValue,
+      // which may or may not be ok.
+      mResponseTarget = nullptr;
+      mThisVal = nullptr;
     }
 
-    virtual ~ThenValue() {}
-
   private:
     nsRefPtr<TargetType> mResponseTarget;
     nsRefPtr<ThisType> mThisVal;
     ResolveMethodType mResolveMethod;
     RejectMethodType mRejectMethod;
   };
 public:
 
   template<typename TargetType, typename ThisType,
            typename ResolveMethodType, typename RejectMethodType>
   void Then(TargetType* aResponseTarget, const char* aCallSite, ThisType* aThisVal,
             ResolveMethodType aResolveMethod, RejectMethodType aRejectMethod)
   {
     MutexAutoLock lock(mMutex);
     MOZ_RELEASE_ASSERT(!IsExclusive || !mHaveConsumer);
     mHaveConsumer = true;
-    ThenValueBase* thenValue = new ThenValue<TargetType, ThisType, ResolveMethodType,
-                                             RejectMethodType>(aResponseTarget, aThisVal,
-                                                               aResolveMethod, aRejectMethod,
-                                                               aCallSite);
+    nsRefPtr<ThenValueBase> thenValue = new ThenValue<TargetType, ThisType, ResolveMethodType,
+                                                      RejectMethodType>(aResponseTarget, aThisVal,
+                                                                        aResolveMethod, aRejectMethod,
+                                                                        aCallSite);
     PROMISE_LOG("%s invoking Then() [this=%p, thenValue=%p, aThisVal=%p, isPending=%d]",
-                aCallSite, this, thenValue, aThisVal, (int) IsPending());
+                aCallSite, this, thenValue.get(), aThisVal, (int) IsPending());
     if (!IsPending()) {
       thenValue->Dispatch(this);
     } else {
       mThenValues.AppendElement(thenValue);
     }
   }
 
   void ChainTo(already_AddRefed<MediaPromise> aChainedPromise, const char* aCallSite)
@@ -326,17 +337,17 @@ protected:
     MOZ_ASSERT(mThenValues.IsEmpty());
     MOZ_ASSERT(mChainedPromises.IsEmpty());
   };
 
   const char* mCreationSite; // For logging
   Mutex mMutex;
   Maybe<ResolveValueType> mResolveValue;
   Maybe<RejectValueType> mRejectValue;
-  nsTArray<ThenValueBase*> mThenValues;
+  nsTArray<nsRefPtr<ThenValueBase>> mThenValues;
   nsTArray<nsRefPtr<MediaPromise>> mChainedPromises;
   bool mHaveConsumer;
 };
 
 /*
  * Class to encapsulate a promise for a particular role. Use this as the member
  * variable for a class whose method returns a promise.
  */