Bug 1159558 - Redesign watching to use a manager. r=jww

dom/html/HTMLMediaElement.cpp

@@ -946,7 +946,7 @@ void HTMLMediaElement::NotifyMediaStreamTracksAvailable(DOMMediaStream* aStream)
     NotifyOwnerDocumentActivityChanged();
   }
 
-  mReadyStateUpdater->Notify();
+  mWatchManager.ManualNotify(&HTMLMediaElement::UpdateReadyStateInternal);
 }
 
 void HTMLMediaElement::LoadFromSourceChildren()
@@ -2042,10 +2042,10 @@ HTMLMediaElement::LookupMediaElementURITable(nsIURI* aURI)
 
 HTMLMediaElement::HTMLMediaElement(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo)
   : nsGenericHTMLElement(aNodeInfo),
+    mWatchManager(this),
     mCurrentLoadID(0),
     mNetworkState(nsIDOMHTMLMediaElement::NETWORK_EMPTY),
     mReadyState(nsIDOMHTMLMediaElement::HAVE_NOTHING, "HTMLMediaElement::mReadyState"),
-    mReadyStateUpdater("HTMLMediaElement::mReadyStateUpdater"),
     mLoadWaitStatus(NOT_WAITING),
     mVolume(1.0),
     mPreloadAction(PRELOAD_UNDEFINED),
@@ -2110,11 +2110,10 @@ HTMLMediaElement::HTMLMediaElement(already_AddRefed<mozilla::dom::NodeInfo>& aNo
   NotifyOwnerDocumentActivityChanged();
 
   MOZ_ASSERT(NS_IsMainThread());
-  mReadyStateUpdater->AddWeakCallback(this, &HTMLMediaElement::UpdateReadyStateInternal);
+  mWatchManager.Watch(mDownloadSuspendedByCache, &HTMLMediaElement::UpdateReadyStateInternal);
-  mReadyStateUpdater->Watch(mDownloadSuspendedByCache);
   // Paradoxically, there is a self-edge whereby UpdateReadyStateInternal refuses
   // to run until mReadyState reaches at least HAVE_METADATA by some other means.
-  mReadyStateUpdater->Watch(mReadyState);
+  mWatchManager.Watch(mReadyState, &HTMLMediaElement::UpdateReadyStateInternal);
 }
 
 HTMLMediaElement::~HTMLMediaElement()
@@ -3073,7 +3072,7 @@ void HTMLMediaElement::SetupSrcMediaStreamPlayback(DOMMediaStream* aStream)
   // playing a stream, we'll need to add a CombineWithPrincipal call here.
   mMediaStreamListener = new StreamListener(this, "HTMLMediaElement::mMediaStreamListener");
   mMediaStreamSizeListener = new StreamSizeListener(this);
-  mReadyStateUpdater->Watch(*mMediaStreamListener);
+  mWatchManager.Watch(*mMediaStreamListener, &HTMLMediaElement::UpdateReadyStateInternal);
 
   GetSrcMediaStream()->AddListener(mMediaStreamListener);
   // Listen for an initial image size on mSrcStream so we can get results even
@@ -3123,7 +3122,7 @@ void HTMLMediaElement::EndSrcMediaStreamPlayback()
   }
 
   // Kill its reference to this element
-  mReadyStateUpdater->Unwatch(*mMediaStreamListener);
+  mWatchManager.Unwatch(*mMediaStreamListener, &HTMLMediaElement::UpdateReadyStateInternal);
   mMediaStreamListener->Forget();
   mMediaStreamListener = nullptr;
   mMediaStreamSizeListener->Forget();
@@ -3226,7 +3225,7 @@ void HTMLMediaElement::MetadataLoaded(const MediaInfo* aInfo,
   if (!aInfo->HasVideo()) {
     ResetState();
   } else {
-    mReadyStateUpdater->Notify();
+    mWatchManager.ManualNotify(&HTMLMediaElement::UpdateReadyStateInternal);
   }
 
   if (IsVideo() && aInfo->HasVideo()) {
@@ -3887,7 +3886,7 @@ void HTMLMediaElement::UpdateMediaSize(const nsIntSize& aSize)
   }
 
   mMediaInfo.mVideo.mDisplay = aSize;
-  mReadyStateUpdater->Notify();
+  mWatchManager.ManualNotify(&HTMLMediaElement::UpdateReadyStateInternal);
 }
 
 void HTMLMediaElement::UpdateInitialMediaSize(const nsIntSize& aSize)

dom/html/HTMLMediaElement.h

@@ -217,6 +217,9 @@ public:
   // Dispatch events
   virtual nsresult DispatchAsyncEvent(const nsAString& aName) final override;
 
+  // Triggers a recomputation of readyState.
+  void UpdateReadyState() override { UpdateReadyStateInternal(); }
+
   // Dispatch events that were raised while in the bfcache
   nsresult DispatchPendingMediaEvents();
 
@@ -642,17 +645,7 @@ protected:
   class StreamSizeListener;
 
   MediaDecoderOwner::NextFrameStatus NextFrameStatus();
-
+  void SetDecoder(MediaDecoder* aDecoder) { mDecoder = aDecoder; }
-  void SetDecoder(MediaDecoder* aDecoder)
-  {
-    if (mDecoder) {
-      mReadyStateUpdater->Unwatch(mDecoder->ReadyStateWatchTarget());
-    }
-    mDecoder = aDecoder;
-    if (mDecoder) {
-      mReadyStateUpdater->Watch(mDecoder->ReadyStateWatchTarget());
-    }
-  }
 
   virtual void GetItemValueText(DOMString& text) override;
   virtual void SetItemValueText(const nsAString& text) override;
@@ -1032,6 +1025,9 @@ protected:
   // At most one of mDecoder and mSrcStream can be non-null.
   nsRefPtr<MediaDecoder> mDecoder;
 
+  // State-watching manager.
+  WatchManager<HTMLMediaElement> mWatchManager;
+
   // A reference to the VideoFrameContainer which contains the current frame
   // of video to display.
   nsRefPtr<VideoFrameContainer> mVideoFrameContainer;
@@ -1102,8 +1098,6 @@ protected:
   nsMediaNetworkState mNetworkState;
   Watchable<nsMediaReadyState> mReadyState;
 
-  WatcherHolder mReadyStateUpdater;
-
   enum LoadAlgorithmState {
     // No load algorithm instance is waiting for a source to be added to the
     // media in order to continue loading.

dom/media/MediaDecoder.cpp

@@ -585,7 +585,7 @@ bool MediaDecoder::IsInfinite()
 }
 
 MediaDecoder::MediaDecoder() :
-  mReadyStateWatchTarget("MediaDecoder::mReadyStateWatchTarget"),
+  mWatchManager(this),
   mDecoderPosition(0),
   mPlaybackPosition(0),
   mCurrentTime(0.0),
@@ -636,8 +636,8 @@ MediaDecoder::MediaDecoder() :
   mAudioChannel = AudioChannelService::GetDefaultAudioChannel();
 
   // Initialize watchers.
-  mReadyStateWatchTarget->Watch(mPlayState);
+  mWatchManager.Watch(mPlayState, &MediaDecoder::UpdateReadyState);
-  mReadyStateWatchTarget->Watch(mNextFrameStatus);
+  mWatchManager.Watch(mNextFrameStatus, &MediaDecoder::UpdateReadyState);
 }
 
 bool MediaDecoder::Init(MediaDecoderOwner* aOwner)
@@ -1648,7 +1648,7 @@ void MediaDecoder::NotifyDataArrived(const char* aBuffer, uint32_t aLength, int6
 
   // ReadyState computation depends on MediaDecoder::CanPlayThrough, which
   // depends on the download rate.
-  mReadyStateWatchTarget->Notify();
+  UpdateReadyState();
 }
 
 // Provide access to the state machine object

dom/media/MediaDecoder.h

@@ -1028,7 +1028,12 @@ public:
     GetFrameStatistics().NotifyDecodedFrames(aParsed, aDecoded, aDropped);
   }
 
-  WatchTarget& ReadyStateWatchTarget() { return *mReadyStateWatchTarget; }
+  void UpdateReadyState()
+  {
+    if (mOwner) {
+      mOwner->UpdateReadyState();
+    }
+  }
 
   virtual MediaDecoderOwner::NextFrameStatus NextFrameStatus() { return mNextFrameStatus; }
 
@@ -1047,7 +1052,8 @@ protected:
   // Return true if the decoder has reached the end of playback
   bool IsEnded() const;
 
-  WatcherHolder mReadyStateWatchTarget;
+  // State-watching manager.
+  WatchManager<MediaDecoder> mWatchManager;
 
   // NextFrameStatus, mirrored from the state machine.
   Mirror<MediaDecoderOwner::NextFrameStatus>::Holder mNextFrameStatus;

dom/media/MediaDecoderOwner.h

@@ -24,6 +24,9 @@ public:
   // Dispatch an asynchronous event to the decoder owner
   virtual nsresult DispatchAsyncEvent(const nsAString& aName) = 0;
 
+  // Triggers a recomputation of readyState.
+  virtual void UpdateReadyState() = 0;
+
   /**
    * Fires a timeupdate event. If aPeriodic is true, the event will only
    * be fired if we've not fired a timeupdate event (for any reason) in the

dom/media/MediaDecoderStateMachine.cpp

@@ -202,6 +202,7 @@ MediaDecoderStateMachine::MediaDecoderStateMachine(MediaDecoder* aDecoder,
                                                    MediaDecoderReader* aReader,
                                                    bool aRealTime) :
   mDecoder(aDecoder),
+  mWatchManager(this),
   mRealTime(aRealTime),
   mDispatchedStateMachine(false),
   mDelayedScheduler(this),
@@ -210,7 +211,6 @@ MediaDecoderStateMachine::MediaDecoderStateMachine(MediaDecoder* aDecoder,
   mStartTime(-1),
   mEndTime(-1),
   mDurationSet(false),
-  mNextFrameStatusUpdater("MediaDecoderStateMachine::mNextFrameStatusUpdater"),
   mFragmentEndTime(-1),
   mReader(aReader),
   mCurrentFrameTime(0),
@@ -265,11 +265,8 @@ MediaDecoderStateMachine::MediaDecoderStateMachine(MediaDecoder* aDecoder,
   mNextPlayState.Init(mTaskQueue, MediaDecoder::PLAY_STATE_PAUSED, "MediaDecoderStateMachine::mNextPlayState (Mirror)",
                   aDecoder->CanonicalNextPlayState());
 
-  // Skip the initial notification we get when we Watch the value, since we're
+  mWatchManager.Watch(mState, &MediaDecoderStateMachine::UpdateNextFrameStatus);
-  // not on the right thread yet.
+  mWatchManager.Watch(mAudioCompleted, &MediaDecoderStateMachine::UpdateNextFrameStatus);
-  mNextFrameStatusUpdater->Watch(mState);
-  mNextFrameStatusUpdater->Watch(mAudioCompleted);
-  mNextFrameStatusUpdater->AddWeakCallback(this, &MediaDecoderStateMachine::UpdateNextFrameStatus);
 
   static bool sPrefCacheInit = false;
   if (!sPrefCacheInit) {

dom/media/MediaDecoderStateMachine.h

@@ -775,6 +775,9 @@ public:
   // Task queue for running the state machine.
   nsRefPtr<MediaTaskQueue> mTaskQueue;
 
+  // State-watching manager.
+  WatchManager<MediaDecoderStateMachine> mWatchManager;
+
   // True is we are decoding a realtime stream, like a camera stream.
   bool mRealTime;
 
@@ -903,7 +906,6 @@ public:
 
   // The status of our next frame. Mirrored on the main thread and used to
   // compute ready state.
-  WatcherHolder mNextFrameStatusUpdater;
   Canonical<NextFrameStatus>::Holder mNextFrameStatus;
 public:
   AbstractCanonical<NextFrameStatus>* CanonicalNextFrameStatus() { return &mNextFrameStatus; }

dom/media/StateWatching.h

@@ -43,8 +43,8 @@
  * There are two basic pieces:
  *   (1) Objects that can be watched for updates. These inherit WatchTarget.
  *   (2) Objects that receive objects and trigger processing. These inherit
- *       AbstractWatcher. Note that some watchers may themselves be watched,
+ *       AbstractWatcher. In the current machinery, these exist only internally
- *       allowing watchers to be composed together.
+ *       within the WatchManager, though that could change.
  *
  * Note that none of this machinery is thread-safe - it must all happen on the
  * same owning thread. To solve multi-threaded use-cases, use state mirroring
@@ -76,18 +76,7 @@ public:
   virtual void Notify() = 0;
 
 protected:
-  virtual ~AbstractWatcher() {}
+  virtual ~AbstractWatcher() { MOZ_ASSERT(mDestroyed); }
-  virtual void CustomDestroy() {}
-
-private:
-  // Only the holder is allowed to invoke Destroy().
-  friend class WatcherHolder;
-  void Destroy()
-  {
-    mDestroyed = true;
-    CustomDestroy();
-  }
-
   bool mDestroyed;
 };
 
@@ -174,87 +163,128 @@ private:
   T mValue;
 };
 
-/*
+// Manager class for state-watching. Declare one of these in any class for which
- * Watcher is the concrete AbstractWatcher implementation. It registers itself with
+// you want to invoke method callbacks.
- * various WatchTargets, and accepts any number of callbacks that will be
+//
- * invoked whenever any WatchTarget notifies of a change. It may also be watched
+// Internally, WatchManager maintains one AbstractWatcher per callback method.
- * by other watchers.
+// Consumers invoke Watch/Unwatch on a particular (WatchTarget, Callback) tuple.
- *
+// This causes an AbstractWatcher for |Callback| to be instantiated if it doesn't
- * When a notification is received, a runnable is passed as "direct" to the
+// already exist, and registers it with |WatchTarget|.
- * thread's tail dispatcher, which run directly (rather than via dispatch) when
+//
- * the tail dispatcher fires. All subsequent notifications are ignored until the
+// Using Direct Tasks on the TailDispatcher, WatchManager ensures that we fire
- * runnable executes, triggering the updates and resetting the flags.
+// watch callbacks no more than once per task, once all other operations for that
- */
+// task have been completed.
-class Watcher : public AbstractWatcher, public WatchTarget
+//
+// WatchManager<OwnerType> is intended to be declared as a member of |OwnerType|
+// objects. Given that, it and its owned objects can't hold permanent strong refs to
+// the owner, since that would keep the owner alive indefinitely. Instead, it
+// _only_ holds strong refs while waiting for Direct Tasks to fire. This ensures
+// that everything is kept alive just long enough.
+template <typename OwnerType>
+class WatchManager
 {
 public:
-  explicit Watcher(const char* aName)
+  typedef void(OwnerType::*CallbackMethod)();
-    : WatchTarget(aName), mNotifying(false) {}
+  explicit WatchManager(OwnerType* aOwner)
+    : mOwner(aOwner) {}
 
-  void Notify() override
+  ~WatchManager()
   {
-    if (mNotifying) {
+    for (size_t i = 0; i < mWatchers.Length(); ++i) {
-      return;
+      mWatchers[i]->Destroy();
-    }
-    mNotifying = true;
-
-    // Queue up our notification jobs to run in a stable state.
-    nsCOMPtr<nsIRunnable> r = NS_NewRunnableMethod(this, &Watcher::DoNotify);
-    AbstractThread::GetCurrent()->TailDispatcher().AddDirectTask(r.forget());
-  }
-
-  void Watch(WatchTarget& aTarget) { aTarget.AddWatcher(this); }
-  void Unwatch(WatchTarget& aTarget) { aTarget.RemoveWatcher(this); }
-
-  template<typename ThisType>
-  void AddWeakCallback(ThisType* aThisVal, void(ThisType::*aMethod)())
-  {
-    mCallbacks.AppendElement(NS_NewNonOwningRunnableMethod(aThisVal, aMethod));
-  }
-
-protected:
-  void CustomDestroy() override { mCallbacks.Clear(); }
-
-  void DoNotify()
-  {
-    MOZ_ASSERT(mNotifying);
-    mNotifying = false;
-
-    // Notify dependent watchers.
-    NotifyWatchers();
-
-    for (size_t i = 0; i < mCallbacks.Length(); ++i) {
-      mCallbacks[i]->Run();
     }
   }
 
-private:
+  void Watch(WatchTarget& aTarget, CallbackMethod aMethod)
-  nsTArray<nsCOMPtr<nsIRunnable>> mCallbacks;
-
-  bool mNotifying;
-};
-
-/*
- * WatcherHolder encapsulates a Watcher and handles lifetime issues. Use it to
- * holder watcher members.
- */
-class WatcherHolder
-{
-public:
-  explicit WatcherHolder(const char* aName) { mWatcher = new Watcher(aName); }
-  operator Watcher*() { return mWatcher; }
-  Watcher* operator->() { return mWatcher; }
-
-  ~WatcherHolder()
   {
-    mWatcher->Destroy();
+    aTarget.AddWatcher(&EnsureWatcher(aMethod));
-    mWatcher = nullptr;
+  }
+
+  void Unwatch(WatchTarget& aTarget, CallbackMethod aMethod)
+  {
+    PerCallbackWatcher* watcher = GetWatcher(aMethod);
+    MOZ_ASSERT(watcher);
+    aTarget.RemoveWatcher(watcher);
+  }
+
+  void ManualNotify(CallbackMethod aMethod)
+  {
+    PerCallbackWatcher* watcher = GetWatcher(aMethod);
+    MOZ_ASSERT(watcher);
+    watcher->Notify();
   }
 
 private:
-  nsRefPtr<Watcher> mWatcher;
+  class PerCallbackWatcher : public AbstractWatcher
-};
+  {
+  public:
+    PerCallbackWatcher(OwnerType* aOwner, CallbackMethod aMethod)
+      : mOwner(aOwner), mCallbackMethod(aMethod) {}
 
+    void Destroy()
+    {
+      mDestroyed = true;
+      mOwner = nullptr;
+    }
+
+    void Notify() override
+    {
+      MOZ_DIAGNOSTIC_ASSERT(mOwner, "mOwner is only null after destruction, "
+                                    "at which point we shouldn't be notified");
+      if (mStrongRef) {
+        // We've already got a notification job in the pipe.
+        return;
+      }
+      mStrongRef = mOwner; // Hold the owner alive while notifying.
+
+      // Queue up our notification jobs to run in a stable state.
+      nsCOMPtr<nsIRunnable> r = NS_NewRunnableMethod(this, &PerCallbackWatcher::DoNotify);
+      AbstractThread::GetCurrent()->TailDispatcher().AddDirectTask(r.forget());
+    }
+
+    bool CallbackMethodIs(CallbackMethod aMethod) const
+    {
+      return mCallbackMethod == aMethod;
+    }
+
+  private:
+    ~PerCallbackWatcher() {}
+
+    void DoNotify()
+    {
+      MOZ_ASSERT(mStrongRef);
+      nsRefPtr<OwnerType> ref = mStrongRef.forget();
+      ((*ref).*mCallbackMethod)();
+    }
+
+    OwnerType* mOwner; // Never null.
+    nsRefPtr<OwnerType> mStrongRef; // Only non-null when notifying.
+    CallbackMethod mCallbackMethod;
+  };
+
+  PerCallbackWatcher* GetWatcher(CallbackMethod aMethod)
+  {
+    for (size_t i = 0; i < mWatchers.Length(); ++i) {
+      if (mWatchers[i]->CallbackMethodIs(aMethod)) {
+        return mWatchers[i];
+      }
+    }
+    return nullptr;
+  }
+
+  PerCallbackWatcher& EnsureWatcher(CallbackMethod aMethod)
+  {
+    PerCallbackWatcher* watcher = GetWatcher(aMethod);
+    if (watcher) {
+      return *watcher;
+    }
+    watcher = mWatchers.AppendElement(new PerCallbackWatcher(mOwner, aMethod))->get();
+    return *watcher;
+  }
+
+  nsTArray<nsRefPtr<PerCallbackWatcher>> mWatchers;
+  OwnerType* mOwner;
+};
 
 #undef WATCH_LOG