Make ui::Compositor BeginFrame subscription robust

The current logic for removing a ui::Compositor's BeginFrame observer
while dispatching the BeginFrame signal is broken. Make it robust by
using an ObserverList.

BUG=394562

Review URL: https://codereview.chromium.org/1414533003

Cr-Commit-Position: refs/heads/master@{#355649}

3 files changed, 94 insertions, 29 deletions

diff --git a/ui/compositor/compositor.cc b/ui/compositor/compositor.cc
index 6d1bdace..ed80f62 100644
--- a/ui/compositor/compositor.cc
+++ b/ui/compositor/compositor.cc
@@ -194,8 +194,6 @@ Compositor::~Compositor() {
   FOR_EACH_OBSERVER(CompositorAnimationObserver, animation_observer_list_,
                     OnCompositingShuttingDown(this));
 
-  DCHECK(begin_frame_observer_list_.empty());
-
   if (root_layer_)
     root_layer_->ResetCompositor();
 
@@ -359,30 +357,21 @@ bool Compositor::HasAnimationObserver(
 }
 
 void Compositor::AddBeginFrameObserver(CompositorBeginFrameObserver* observer) {
-  DCHECK(std::find(begin_frame_observer_list_.begin(),
-                   begin_frame_observer_list_.end(), observer) ==
-         begin_frame_observer_list_.end());
-
-  if (begin_frame_observer_list_.empty())
+  if (!begin_frame_observer_list_.might_have_observers())
     host_->SetChildrenNeedBeginFrames(true);
 
+  begin_frame_observer_list_.AddObserver(observer);
+
   if (missed_begin_frame_args_.IsValid())
     observer->OnSendBeginFrame(missed_begin_frame_args_);
-
-  begin_frame_observer_list_.push_back(observer);
 }
 
 void Compositor::RemoveBeginFrameObserver(
     CompositorBeginFrameObserver* observer) {
-  auto it = std::find(begin_frame_observer_list_.begin(),
-                      begin_frame_observer_list_.end(), observer);
-  DCHECK(it != begin_frame_observer_list_.end());
-  begin_frame_observer_list_.erase(it);
+  begin_frame_observer_list_.RemoveObserver(observer);
 
-  if (begin_frame_observer_list_.empty()) {
-    host_->SetChildrenNeedBeginFrames(false);
-    missed_begin_frame_args_ = cc::BeginFrameArgs();
-  }
+  // As this call may take place while iterating over observers, unsubscription
+  // from |host_| is performed after iteration in |SendBeginFramesToChildren()|.
 }
 
 void Compositor::BeginMainFrame(const cc::BeginFrameArgs& args) {
@@ -452,8 +441,18 @@ void Compositor::DidAbortSwapBuffers() {
 }
 
 void Compositor::SendBeginFramesToChildren(const cc::BeginFrameArgs& args) {
-  for (auto observer : begin_frame_observer_list_)
-    observer->OnSendBeginFrame(args);
+  FOR_EACH_OBSERVER(CompositorBeginFrameObserver, begin_frame_observer_list_,
+                    OnSendBeginFrame(args));
+
+  // Unsubscription is performed here, after iteration, to handle the case where
+  // the last BeginFrame observer is removed while iterating over the observers.
+  if (!begin_frame_observer_list_.might_have_observers()) {
+    host_->SetChildrenNeedBeginFrames(false);
+    // Unsubscription should reset |missed_begin_frame_args_|, avoiding stale
+    // BeginFrame dispatch when the next BeginFrame observer is added.
+    missed_begin_frame_args_ = cc::BeginFrameArgs();
+    return;
+  }
 
   missed_begin_frame_args_ = args;
   missed_begin_frame_args_.type = cc::BeginFrameArgs::MISSED;
diff --git a/ui/compositor/compositor.h b/ui/compositor/compositor.h
index 0ba4aea..544a584 100644
--- a/ui/compositor/compositor.h
+++ b/ui/compositor/compositor.h
@@ -5,7 +5,6 @@
 #ifndef UI_COMPOSITOR_COMPOSITOR_H_
 #define UI_COMPOSITOR_COMPOSITOR_H_
 
-#include <list>
 #include <string>
 
 #include "base/containers/hash_tables.h"
@@ -344,7 +343,8 @@ class COMPOSITOR_EXPORT Compositor
 
   base::ObserverList<CompositorObserver, true> observer_list_;
   base::ObserverList<CompositorAnimationObserver> animation_observer_list_;
-  std::list<CompositorBeginFrameObserver*> begin_frame_observer_list_;
+  base::ObserverList<CompositorBeginFrameObserver, true>
+      begin_frame_observer_list_;
 
   gfx::AcceleratedWidget widget_;
   bool widget_valid_;
diff --git a/ui/compositor/compositor_unittest.cc b/ui/compositor/compositor_unittest.cc
index a53bdc2..667845a 100644
--- a/ui/compositor/compositor_unittest.cc
+++ b/ui/compositor/compositor_unittest.cc
@@ -19,6 +19,10 @@ using testing::_;
 namespace ui {
 namespace {
 
+ACTION_P2(RemoveObserver, compositor, observer) {
+  compositor->RemoveBeginFrameObserver(observer);
+}
+
 class MockCompositorBeginFrameObserver : public CompositorBeginFrameObserver {
  public:
   MOCK_METHOD1(OnSendBeginFrame, void(const cc::BeginFrameArgs&));
@@ -91,27 +95,32 @@ TEST_F(CompositorTest, AddAndRemoveBeginFrameObserver) {
     cc::CreateBeginFrameArgsForTesting(BEGINFRAME_FROM_HERE,
                                        base::TimeTicks::FromInternalValue(33));
 
-  // Simulate to trigger new BeginFrame by using |args|.
-  compositor()->SendBeginFramesToChildren(args);
+  MockCompositorBeginFrameObserver test_observer;
+  MockCompositorBeginFrameObserver test_observer2;
+
+  // Add a single observer.
+  compositor()->AddBeginFrameObserver(&test_observer);
+  Mock::VerifyAndClearExpectations(&test_observer);
 
   // When |missed_begin_frame_args_| is sent, its type is set to MISSED.
   cc::BeginFrameArgs expected_args(args);
-  expected_args.type = cc::BeginFrameArgs::MISSED;
+  cc::BeginFrameArgs expected_missed_args(args);
+  expected_missed_args.type = cc::BeginFrameArgs::MISSED;
 
-  MockCompositorBeginFrameObserver test_observer;
-  MockCompositorBeginFrameObserver test_observer2;
+  // Simulate to trigger new BeginFrame by using |args|.
   EXPECT_CALL(test_observer, OnSendBeginFrame(expected_args));
-  EXPECT_CALL(test_observer2, OnSendBeginFrame(expected_args));
+  compositor()->SendBeginFramesToChildren(args);
+  Mock::VerifyAndClearExpectations(&test_observer);
 
   // When new observer is added, Compositor immediately calls OnSendBeginFrame
   // with |missed_begin_frame_args_|.
-  compositor()->AddBeginFrameObserver(&test_observer);
+  EXPECT_CALL(test_observer2, OnSendBeginFrame(expected_missed_args));
   compositor()->AddBeginFrameObserver(&test_observer2);
   Mock::VerifyAndClearExpectations(&test_observer);
   Mock::VerifyAndClearExpectations(&test_observer2);
 
   // When |test_observer2| is removed and added again, it will be called again.
-  EXPECT_CALL(test_observer2, OnSendBeginFrame(expected_args));
+  EXPECT_CALL(test_observer2, OnSendBeginFrame(expected_missed_args));
   compositor()->RemoveBeginFrameObserver(&test_observer2);
   compositor()->AddBeginFrameObserver(&test_observer2);
   Mock::VerifyAndClearExpectations(&test_observer2);
@@ -121,12 +130,69 @@ TEST_F(CompositorTest, AddAndRemoveBeginFrameObserver) {
   EXPECT_CALL(test_observer2, OnSendBeginFrame(_)).Times(0);
   compositor()->RemoveBeginFrameObserver(&test_observer);
   compositor()->RemoveBeginFrameObserver(&test_observer2);
+  compositor()->SendBeginFramesToChildren(args);
   compositor()->AddBeginFrameObserver(&test_observer2);
   Mock::VerifyAndClearExpectations(&test_observer2);
 
   compositor()->RemoveBeginFrameObserver(&test_observer2);
 }
 
+TEST_F(CompositorTest, RemoveBeginFrameObserverWhileSendingBeginFrame) {
+  cc::BeginFrameArgs args = cc::CreateBeginFrameArgsForTesting(
+      BEGINFRAME_FROM_HERE, base::TimeTicks::FromInternalValue(33));
+
+  cc::BeginFrameArgs expected_args(args);
+  cc::BeginFrameArgs expected_missed_args(args);
+  expected_missed_args.type = cc::BeginFrameArgs::MISSED;
+
+  // Add both observers, and simulate removal of |test_observer2| during
+  // BeginFrame dispatch (implicitly triggered when the observer is added).
+  MockCompositorBeginFrameObserver test_observer;
+  MockCompositorBeginFrameObserver test_observer2;
+  EXPECT_CALL(test_observer, OnSendBeginFrame(expected_args));
+  EXPECT_CALL(test_observer2, OnSendBeginFrame(expected_missed_args))
+      .WillOnce(RemoveObserver(compositor(), &test_observer2));
+
+  // When a new observer is added, Compositor immediately calls OnSendBeginFrame
+  // with |missed_begin_frame_args_|.
+  compositor()->AddBeginFrameObserver(&test_observer);
+  compositor()->SendBeginFramesToChildren(args);
+  compositor()->AddBeginFrameObserver(&test_observer2);
+  Mock::VerifyAndClearExpectations(&test_observer);
+  Mock::VerifyAndClearExpectations(&test_observer2);
+
+  // |test_observer2| was removed during the previous implicit BeginFrame
+  // dispatch, and should not get the new frame.
+  expected_args.type = cc::BeginFrameArgs::NORMAL;
+  EXPECT_CALL(test_observer, OnSendBeginFrame(expected_args));
+  EXPECT_CALL(test_observer2, OnSendBeginFrame(_)).Times(0);
+  compositor()->SendBeginFramesToChildren(args);
+  Mock::VerifyAndClearExpectations(&test_observer);
+  Mock::VerifyAndClearExpectations(&test_observer2);
+
+  // Now remove |test_observer| during explicit BeginFrame dispatch.
+  EXPECT_CALL(test_observer, OnSendBeginFrame(expected_args))
+      .WillOnce(RemoveObserver(compositor(), &test_observer));
+  EXPECT_CALL(test_observer2, OnSendBeginFrame(_)).Times(0);
+  compositor()->SendBeginFramesToChildren(args);
+  Mock::VerifyAndClearExpectations(&test_observer);
+  Mock::VerifyAndClearExpectations(&test_observer2);
+
+  // No observers should get the new frame.
+  EXPECT_CALL(test_observer, OnSendBeginFrame(_)).Times(0);
+  EXPECT_CALL(test_observer2, OnSendBeginFrame(_)).Times(0);
+  compositor()->SendBeginFramesToChildren(args);
+  Mock::VerifyAndClearExpectations(&test_observer);
+  Mock::VerifyAndClearExpectations(&test_observer2);
+
+  // Adding a new observer should not trigger a missed frame, as the
+  // previous frame had no observers.
+  EXPECT_CALL(test_observer, OnSendBeginFrame(_)).Times(0);
+  compositor()->AddBeginFrameObserver(&test_observer);
+  compositor()->RemoveBeginFrameObserver(&test_observer);
+  Mock::VerifyAndClearExpectations(&test_observer);
+}
+
 TEST_F(CompositorTest, ReleaseWidgetWithOutputSurfaceNeverCreated) {
   compositor()->SetVisible(false);
   EXPECT_EQ(gfx::kNullAcceleratedWidget,