Bug 1281090. Part 5 - don't lock while sending synchronous notification to avoid deadlock. r=gerald. draft
authorJW Wang <jwwang@mozilla.com>
Tue, 21 Jun 2016 16:32:13 +0800
changeset 380743 74b84944360afcd4ea50390153d071c8e63a74b4
parent 380742 05c147239fa97d754439ce1154d34d29c22542df
child 523805 1d03241bcf1a3a6fc8bb1953215828d6c6873b8f
push id21310
push userjwwang@mozilla.com
push dateThu, 23 Jun 2016 02:27:34 +0000
reviewersgerald
bugs1281090
milestone50.0a1
Bug 1281090. Part 5 - don't lock while sending synchronous notification to avoid deadlock. r=gerald. MozReview-Commit-ID: hOasuXLf5a
dom/media/MediaEventSource.h
--- a/dom/media/MediaEventSource.h
+++ b/dom/media/MediaEventSource.h
@@ -491,32 +491,61 @@ public:
   MediaEventListener
   Connect(nsIEventTarget* aTarget, This* aThis, Method aMethod) {
     return ConnectInternal(aTarget, aThis, aMethod);
   }
 
 protected:
   MediaEventSourceImpl() : mMutex("MediaEventSourceImpl::mMutex") {}
 
-  template <typename... Ts>
-  void NotifyInternal(Ts&&... aEvents) {
+  template <DispatchPolicy P, typename... Ts>
+  typename EnableIf<P == DispatchPolicy::Async, void>::Type
+  NotifyInternal(IntegralConstant<DispatchPolicy, P>, Ts&&... aEvents) {
     MutexAutoLock lock(mMutex);
     int32_t last = static_cast<int32_t>(mListeners.Length()) - 1;
     for (int32_t i = last; i >= 0; --i) {
       auto&& l = mListeners[i];
       // Remove disconnected listeners.
       // It is not optimal but is simple and works well.
       if (l->Token()->IsRevoked()) {
         mListeners.RemoveElementAt(i);
         continue;
       }
       l->Dispatch(Forward<Ts>(aEvents)...);
     }
   }
 
+  template <DispatchPolicy P, typename... Ts>
+  typename EnableIf<P == DispatchPolicy::Sync, void>::Type
+  NotifyInternal(IntegralConstant<DispatchPolicy, P>, Ts&&... aEvents) {
+    // Move |mListeners| to a new container before iteration to prevent
+    // |mListeners| from being disrupted if the listener calls Connect() to
+    // modify |mListeners| in the callback function.
+    nsTArray<UniquePtr<Listener>> listeners;
+    listeners.SwapElements(mListeners);
+    for (auto&& l : listeners) {
+      l->Dispatch(Forward<Ts>(aEvents)...);
+    }
+    PruneListeners();
+    // Move remaining listeners back to |mListeners|.
+    for (auto&& l : listeners) {
+      if (!l->Token()->IsRevoked()) {
+        mListeners.AppendElement(Move(l));
+      }
+    }
+    // Perform sanity checks.
+    MOZ_ASSERT(Lp == ListenerPolicy::NonExclusive || mListeners.Length() <= 1);
+  }
+
+  template <typename... Ts>
+  void Notify(Ts&&... aEvents) {
+    NotifyInternal(IntegralConstant<DispatchPolicy, Dp>(),
+                   Forward<Ts>(aEvents)...);
+  }
+
 private:
   Mutex mMutex;
   nsTArray<UniquePtr<Listener>> mListeners;
 };
 
 template <typename... Es>
 using MediaEventSource =
   MediaEventSourceImpl<DispatchPolicy::Async,
@@ -529,71 +558,61 @@ using MediaEventSourceExc =
 /**
  * A class to separate the interface of event subject (MediaEventSource)
  * and event publisher. Mostly used as a member variable to publish events
  * to the listeners.
  */
 template <typename... Es>
 class MediaEventProducer : public MediaEventSource<Es...> {
 public:
-  template <typename... Ts>
-  void Notify(Ts&&... aEvents) {
-    this->NotifyInternal(Forward<Ts>(aEvents)...);
-  }
+  using MediaEventSource<Es...>::Notify;
 };
 
 /**
  * Specialization for void type. A dummy bool is passed to NotifyInternal
  * since there is no way to pass a void value.
  */
 template <>
 class MediaEventProducer<void> : public MediaEventSource<void> {
 public:
   void Notify() {
-    this->NotifyInternal(true /* dummy */);
+    MediaEventSource<void>::Notify(false /* dummy */);
   }
 };
 
 /**
  * A producer allowing at most one listener.
  */
 template <typename... Es>
 class MediaEventProducerExc : public MediaEventSourceExc<Es...> {
 public:
-  template <typename... Ts>
-  void Notify(Ts&&... aEvents) {
-    this->NotifyInternal(Forward<Ts>(aEvents)...);
-  }
+  using MediaEventSourceExc<Es...>::Notify;
 };
 
 /**
  * Events are passed directly to the callback function of the listeners without
  * dispatching. Note this class is not thread-safe. Both Connect() and Notify()
  * must be called on the same thread.
  */
 template <typename... Es>
 class MediaCallback
   : public MediaEventSourceImpl<DispatchPolicy::Sync,
                                 ListenerPolicy::NonExclusive, Es...> {
 public:
-  template <typename... Ts>
-  void Notify(Ts&&... aEvents) {
-    this->NotifyInternal(Forward<Ts>(aEvents)...);
-  }
+  using MediaEventSourceImpl<DispatchPolicy::Sync,
+                             ListenerPolicy::NonExclusive, Es...>::Notify;
 };
 
 /**
  * A special version of MediaCallback which allows at most one listener.
  */
 template <typename... Es>
 class MediaCallbackExc
   : public MediaEventSourceImpl<DispatchPolicy::Sync,
                                 ListenerPolicy::Exclusive, Es...> {
 public:
-  template <typename... Ts>
-  void Notify(Ts&&... aEvents) {
-    this->NotifyInternal(Forward<Ts>(aEvents)...);
-  }
+  using MediaEventSourceImpl<DispatchPolicy::Sync,
+                             ListenerPolicy::Exclusive, Es...>::Notify;
 };
 
 } // namespace mozilla
 
 #endif //MediaEventSource_h_