Bug 1126465 - Make ThenValueBase inherit from a publicly-usable type, and refcount it. r=mattwoodrow, a=sledru
authorBobby Holley <bobbyholley@gmail.com>
Thu, 29 Jan 2015 22:11:11 -0800
changeset 243642 6e44bfd1e0f8
parent 243641 ebd1573c5911
child 243643 54d7f88c8b75
push id4421
push userryanvm@gmail.com
push date2015-02-02 19:52 +0000
treeherdermozilla-beta@08a02585bc60 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmattwoodrow, sledru
bugs1126465
milestone36.0
Bug 1126465 - Make ThenValueBase inherit from a publicly-usable type, and refcount it. r=mattwoodrow, a=sledru
dom/media/MediaPromise.h
--- a/dom/media/MediaPromise.h
+++ b/dom/media/MediaPromise.h
@@ -77,25 +77,34 @@ public:
   CreateAndReject(RejectValueType aRejectValue, const char* aRejectSite)
   {
     nsRefPtr<MediaPromise<ResolveValueT, RejectValueT>> p =
       new MediaPromise<ResolveValueT, RejectValueT>(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) {}
@@ -104,24 +113,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)
@@ -131,38 +138,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
@@ -218,45 +217,57 @@ 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);
-    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)
@@ -323,17 +334,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;
 };
 
 /*
  * Class to encapsulate a promise for a particular role. Use this as the member
  * variable for a class whose method returns a promise.
  */
 template<typename PromiseType>