Make CallbackList::empty() and Notify() safe to use during iteration.

Also makes Subscription movable.

I'll need this to convert some existing code over to CallbackList.

Bug: none
Change-Id: Iad1c20f8d4facacea67cf4379e6bdb77e7984d1a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2343954
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796684}

base/callback_list.h

@@ -65,10 +65,9 @@
 //
 // UNSUPPORTED:
 //
-// * Calling Notify() reentrantly during callback notification.
 // * Destroying the CallbackList during callback notification.
 //
-// Both of these are possible to support, but not currently necessary.
+// This is possible to support, but not currently necessary.
 
 namespace base {
 
@@ -85,12 +84,13 @@ namespace internal {
 template <typename CallbackList>
 struct CallbackListTraits;
 
+// NOTE: It's important that Callbacks provide iterator stability when items are
+// added to the end, so e.g. a std::vector<> is not suitable here.
 template <typename Signature>
 struct CallbackListTraits<OnceCallbackList<Signature>> {
   using CallbackType = OnceCallback<Signature>;
   using Callbacks = std::list<CallbackType>;
 };
-
 template <typename Signature>
 struct CallbackListTraits<RepeatingCallbackList<Signature>> {
   using CallbackType = RepeatingCallback<Signature>;
@@ -112,8 +112,8 @@ class CallbackListBase {
     explicit Subscription(base::OnceClosure destruction_closure)
         : destruction_closure_(std::move(destruction_closure)) {}
 
-    Subscription(const Subscription&) = delete;
-    Subscription& operator=(const Subscription&) = delete;
+    Subscription(Subscription&&) = default;
+    Subscription& operator=(Subscription&&) = default;
 
     ~Subscription() { std::move(destruction_closure_).Run(); }
 
@@ -160,38 +160,59 @@ class CallbackListBase {
     removal_callback_ = removal_callback;
   }
 
-  // Returns whether the list of registered callbacks is empty. This may not be
-  // called while Notify() is traversing the list (since the results could be
-  // inaccurate).
+  // Returns whether the list of registered callbacks is empty (from an external
+  // perspective -- meaning no remaining callbacks are live).
   bool empty() const {
-    DCHECK(!iterating_);
-    return callbacks_.empty();
+    return std::all_of(callbacks_.cbegin(), callbacks_.cend(),
+                       [](const auto& callback) { return callback.is_null(); });
   }
 
   // Calls all registered callbacks that are not canceled beforehand. If any
   // callbacks are unregistered, notifies any registered removal callback at the
   // end.
+  //
+  // Arguments must be copyable, since they must be supplied to all callbacks.
+  // Move-only types would be destructively modified by passing them to the
+  // first callback and not reach subsequent callbacks as intended.
+  //
+  // Notify() may be called re-entrantly, in which case the nested call
+  // completes before the outer one continues. Callbacks are only ever added at
+  // the end and canceled callbacks are not pruned from the list until the
+  // outermost iteration completes, so existing iterators should never be
+  // invalidated. However, this does mean that a callback added during a nested
+  // call can be notified by outer calls -- meaning it will be notified about
+  // things that happened before it was added -- if its subscription outlives
+  // the reentrant Notify() call.
   template <typename... RunArgs>
   void Notify(RunArgs&&... args) {
-    // Calling Notify() reentrantly is currently unsupported.
-    DCHECK(!iterating_);
-
     if (empty())
       return;  // Nothing to do.
 
-    // Canceled callbacks should be removed from the list whenever notification
-    // isn't in progress, so right now all callbacks should be valid.
-    const auto callback_valid = [](const auto& cb) { return !cb.is_null(); };
-    DCHECK(std::all_of(callbacks_.cbegin(), callbacks_.cend(), callback_valid));
-
     {
       AutoReset<bool> iterating(&iterating_, true);
+
       // Skip any callbacks that are canceled during iteration.
-      for (auto it = callbacks_.begin(); it != callbacks_.end();
-           it = std::find_if(it, callbacks_.end(), callback_valid))
+      // NOTE: Since RunCallback() may call Add(), it's not safe to cache the
+      // value of callbacks_.end() across loop iterations.
+      const auto next_valid = [this](const auto it) {
+        return std::find_if_not(it, callbacks_.end(), [](const auto& callback) {
+          return callback.is_null();
+        });
+      };
+      for (auto it = next_valid(callbacks_.begin()); it != callbacks_.end();
+           it = next_valid(it))
+        // NOTE: Intentionally does not call std::forward<RunArgs>(args)...,
+        // since that would allow move-only arguments.
         static_cast<CallbackListImpl*>(this)->RunCallback(it++, args...);
     }
 
+    // Re-entrant invocations shouldn't prune anything from the list. This can
+    // invalidate iterators from underneath higher call frames. It's safe to
+    // simply do nothing, since the outermost frame will continue through here
+    // and prune all null callbacks below.
+    if (iterating_)
+      return;
+
     // Any null callbacks remaining in the list were canceled due to
     // Subscription destruction during iteration, and can safely be erased now.
     const size_t erased_callbacks =
@@ -259,6 +280,8 @@ class OnceCallbackList
     // splice() removes them from |callbacks_| without invalidating those.
     null_callbacks_.splice(null_callbacks_.end(), this->callbacks_, it);
 
+    // NOTE: Intentionally does not call std::forward<RunArgs>(args)...; see
+    // comments in Notify().
     std::move(*it).Run(args...);
   }
 
@@ -288,6 +311,8 @@ class RepeatingCallbackList
   // Runs the current callback, which may cancel it or any other callbacks.
   template <typename... RunArgs>
   void RunCallback(typename Traits::Callbacks::iterator it, RunArgs&&... args) {
+    // NOTE: Intentionally does not call std::forward<RunArgs>(args)...; see
+    // comments in Notify().
     it->Run(args...);
   }
 

base/callback_list_unittest.cc

@@ -393,6 +393,62 @@ TEST(CallbackListTest, EmptyList) {
   cb_reg.Notify();
 }
 
+// empty() should be callable during iteration, and return false if not all the
+// remaining callbacks in the list are null.
+TEST(CallbackListTest, NonEmptyListDuringIteration) {
+  // Declare items such that |cb_reg| is torn down before the subscriptions.
+  // This ensures the removal callback's invariant that the callback list is
+  // nonempty will always hold.
+  Remover<RepeatingClosureList> remover;
+  Listener listener;
+  std::unique_ptr<RepeatingClosureList::Subscription> remover_sub, listener_sub;
+  RepeatingClosureList cb_reg;
+  cb_reg.set_removal_callback(base::BindRepeating(
+      [](const RepeatingClosureList* callbacks) {
+        EXPECT_FALSE(callbacks->empty());
+      },
+      Unretained(&cb_reg)));
+
+  remover_sub = cb_reg.Add(
+      BindRepeating(&Remover<RepeatingClosureList>::IncrementTotalAndRemove,
+                    Unretained(&remover)));
+  listener_sub = cb_reg.Add(
+      BindRepeating(&Listener::IncrementTotal, Unretained(&listener)));
+
+  // |remover| will remove |listener|.
+  remover.SetSubscriptionToRemove(std::move(listener_sub));
+
+  cb_reg.Notify();
+
+  EXPECT_EQ(1, remover.total());
+  EXPECT_EQ(0, listener.total());
+}
+
+// empty() should be callable during iteration, and return true if all the
+// remaining callbacks in the list are null.
+TEST(CallbackListTest, EmptyListDuringIteration) {
+  OnceClosureList cb_reg;
+  cb_reg.set_removal_callback(base::BindRepeating(
+      [](const OnceClosureList* callbacks) { EXPECT_TRUE(callbacks->empty()); },
+      Unretained(&cb_reg)));
+
+  Remover<OnceClosureList> remover;
+  Listener listener;
+  std::unique_ptr<OnceClosureList::Subscription> remover_sub =
+      cb_reg.Add(BindOnce(&Remover<OnceClosureList>::IncrementTotalAndRemove,
+                          Unretained(&remover)));
+  std::unique_ptr<OnceClosureList::Subscription> listener_sub =
+      cb_reg.Add(BindOnce(&Listener::IncrementTotal, Unretained(&listener)));
+
+  // |remover| will remove |listener|.
+  remover.SetSubscriptionToRemove(std::move(listener_sub));
+
+  cb_reg.Notify();
+
+  EXPECT_EQ(1, remover.total());
+  EXPECT_EQ(0, listener.total());
+}
+
 TEST(CallbackListTest, RemovalCallback) {
   Counter remove_count;
   RepeatingClosureList cb_reg;
@@ -444,6 +500,25 @@ TEST(CallbackListTest, AbandonSubscriptions) {
   subscription.reset();
 }
 
+// Subscriptions should be movable.
+TEST(CallbackListTest, MoveSubscription) {
+  RepeatingClosureList cb_reg;
+  Listener listener;
+  std::unique_ptr<RepeatingClosureList::Subscription> subscription1 =
+      cb_reg.Add(
+          BindRepeating(&Listener::IncrementTotal, Unretained(&listener)));
+  cb_reg.Notify();
+  EXPECT_EQ(1, listener.total());
+
+  auto subscription2 = std::move(subscription1);
+  cb_reg.Notify();
+  EXPECT_EQ(2, listener.total());
+
+  subscription2.reset();
+  cb_reg.Notify();
+  EXPECT_EQ(2, listener.total());
+}
+
 TEST(CallbackListTest, CancelBeforeRunning) {
   OnceClosureList cb_reg;
   Listener a;
@@ -461,5 +536,71 @@ TEST(CallbackListTest, CancelBeforeRunning) {
   EXPECT_EQ(0, a.total());
 }
 
+// Verifies Notify() can be called reentrantly and what its expected effects
+// are.
+TEST(CallbackListTest, ReentrantNotify) {
+  RepeatingClosureList cb_reg;
+  Listener a, b, c, d;
+  std::unique_ptr<RepeatingClosureList::Subscription> a_subscription,
+      c_subscription;
+
+  // A callback to run for |a|.
+  const auto a_callback =
+      [](RepeatingClosureList* callbacks, Listener* a,
+         std::unique_ptr<RepeatingClosureList::Subscription>* a_subscription,
+         const Listener* b, Listener* c,
+         std::unique_ptr<RepeatingClosureList::Subscription>* c_subscription,
+         Listener* d) {
+        // This should be the first callback.
+        EXPECT_EQ(0, a->total());
+        EXPECT_EQ(0, b->total());
+        EXPECT_EQ(0, c->total());
+        EXPECT_EQ(0, d->total());
+
+        // Increment |a| once.
+        a->IncrementTotal();
+
+        // Prevent |a| from being incremented again during the reentrant
+        // Notify(). Since this is the first callback, this also verifies the
+        // inner Notify() doesn't assume the first callback (or all callbacks)
+        // are valid.
+        a_subscription->reset();
+
+        // Add |c| and |d| to be incremented by the reentrant Notify().
+        *c_subscription = callbacks->Add(
+            BindRepeating(&Listener::IncrementTotal, Unretained(c)));
+        std::unique_ptr<RepeatingClosureList::Subscription> d_subscription =
+            callbacks->Add(
+                BindRepeating(&Listener::IncrementTotal, Unretained(d)));
+
+        // Notify reentrantly.  This should not increment |a|, but all the
+        // others should be incremented.
+        callbacks->Notify();
+        EXPECT_EQ(1, b->total());
+        EXPECT_EQ(1, c->total());
+        EXPECT_EQ(1, d->total());
+
+        // Since |d_subscription| is locally scoped, it should be canceled
+        // before the outer Notify() increments |d|.  |c_subscription| already
+        // exists and thus |c| should get incremented again by the outer
+        // Notify() even though it wasn't subscribed when that was called.
+      };
+
+  // Add |a| and |b| to the list to be notified, and notify.
+  a_subscription = cb_reg.Add(
+      BindRepeating(a_callback, Unretained(&cb_reg), Unretained(&a),
+                    Unretained(&a_subscription), Unretained(&b), Unretained(&c),
+                    Unretained(&c_subscription), Unretained(&d)));
+  std::unique_ptr<RepeatingClosureList::Subscription> b_subscription =
+      cb_reg.Add(BindRepeating(&Listener::IncrementTotal, Unretained(&b)));
+
+  // Execute both notifications and check the cumulative effect.
+  cb_reg.Notify();
+  EXPECT_EQ(1, a.total());
+  EXPECT_EQ(2, b.total());
+  EXPECT_EQ(2, c.total());
+  EXPECT_EQ(1, d.total());
+}
+
 }  // namespace
 }  // namespace base