events: Fix event dispatch when the dispatcher list changes during dispatch.

Avoid crashing if the dispatcher list changes during dispatching an event. This
is done by using an ObserverList<> instead of a std::vector<> for the dispatcher
list. Update the doc for Add/RemovePlatformEventDispatcher() to clarify the
expected behaviour when dispatchers are added/removed during dispatching an
event.

BUG=360477
R=sky@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@265159 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 195 insertions, 13 deletions

diff --git a/ui/events/platform/platform_event_source.cc b/ui/events/platform/platform_event_source.cc
index a1a79ce..4abb4ba 100644
--- a/ui/events/platform/platform_event_source.cc
+++ b/ui/events/platform/platform_event_source.cc
@@ -34,17 +34,12 @@ PlatformEventSource* PlatformEventSource::GetInstance() { return instance_; }
 void PlatformEventSource::AddPlatformEventDispatcher(
     PlatformEventDispatcher* dispatcher) {
   CHECK(dispatcher);
-  DCHECK(std::find(dispatchers_.begin(), dispatchers_.end(), dispatcher) ==
-         dispatchers_.end());
-  dispatchers_.push_back(dispatcher);
+  dispatchers_.AddObserver(dispatcher);
 }
 
 void PlatformEventSource::RemovePlatformEventDispatcher(
     PlatformEventDispatcher* dispatcher) {
-  PlatformEventDispatcherList::iterator remove =
-      std::remove(dispatchers_.begin(), dispatchers_.end(), dispatcher);
-  if (remove != dispatchers_.end())
-    dispatchers_.erase(remove);
+  dispatchers_.RemoveObserver(dispatcher);
 }
 
 scoped_ptr<ScopedEventDispatcher> PlatformEventSource::OverrideDispatcher(
@@ -77,11 +72,10 @@ uint32_t PlatformEventSource::DispatchEvent(PlatformEvent platform_event) {
     action = overridden_dispatcher_->DispatchEvent(platform_event);
   should_quit = !!(action & POST_DISPATCH_QUIT_LOOP);
 
-  if (action & POST_DISPATCH_PERFORM_DEFAULT) {
-    for (PlatformEventDispatcherList::iterator i = dispatchers_.begin();
-         i != dispatchers_.end();
-         ++i) {
-      PlatformEventDispatcher* dispatcher = *(i);
+  if ((action & POST_DISPATCH_PERFORM_DEFAULT) &&
+      dispatchers_.might_have_observers()) {
+    ObserverList<PlatformEventDispatcher>::Iterator iter(dispatchers_);
+    while (PlatformEventDispatcher* dispatcher = iter.GetNext()) {
       if (dispatcher->CanDispatchEvent(platform_event))
         action = dispatcher->DispatchEvent(platform_event);
       if (action & POST_DISPATCH_QUIT_LOOP)
diff --git a/ui/events/platform/platform_event_source.h b/ui/events/platform/platform_event_source.h
index 407085c..d5867cd 100644
--- a/ui/events/platform/platform_event_source.h
+++ b/ui/events/platform/platform_event_source.h
@@ -30,7 +30,14 @@ class EVENTS_EXPORT PlatformEventSource {
 
   static PlatformEventSource* GetInstance();
 
+  // Adds a dispatcher to the dispatcher list. If a dispatcher is added during
+  // dispatching an event, then the newly added dispatcher also receives that
+  // event.
   void AddPlatformEventDispatcher(PlatformEventDispatcher* dispatcher);
+
+  // Removes a dispatcher from the dispatcher list. Dispatchers can safely be
+  // removed from the dispatcher list during an event is being dispatched,
+  // without affecting the dispatch of the event to other existing dispatchers.
   void RemovePlatformEventDispatcher(PlatformEventDispatcher* dispatcher);
 
   // Installs a PlatformEventDispatcher that receives all the events. The
@@ -65,7 +72,10 @@ class EVENTS_EXPORT PlatformEventSource {
 
   void OnOverriddenDispatcherRestored();
 
-  typedef std::vector<PlatformEventDispatcher*> PlatformEventDispatcherList;
+  // Use an ObserverList<> instead of an std::vector<> to store the list of
+  // dispatchers, so that adding/removing dispatchers during an event dispatch
+  // is well-defined.
+  typedef ObserverList<PlatformEventDispatcher> PlatformEventDispatcherList;
   PlatformEventDispatcherList dispatchers_;
   PlatformEventDispatcher* overridden_dispatcher_;
 
diff --git a/ui/events/platform/platform_event_source_unittest.cc b/ui/events/platform/platform_event_source_unittest.cc
index 78b7cc4..fa5e9ef 100644
--- a/ui/events/platform/platform_event_source_unittest.cc
+++ b/ui/events/platform/platform_event_source_unittest.cc
@@ -27,6 +27,20 @@ scoped_ptr<PlatformEvent> CreatePlatformEvent() {
 template <typename T>
 void DestroyScopedPtr(scoped_ptr<T> object) {}
 
+void RemoveDispatcher(PlatformEventDispatcher* dispatcher) {
+  PlatformEventSource::GetInstance()->RemovePlatformEventDispatcher(dispatcher);
+}
+
+void RemoveDispatchers(PlatformEventDispatcher* first,
+                       PlatformEventDispatcher* second) {
+  PlatformEventSource::GetInstance()->RemovePlatformEventDispatcher(first);
+  PlatformEventSource::GetInstance()->RemovePlatformEventDispatcher(second);
+}
+
+void AddDispatcher(PlatformEventDispatcher* dispatcher) {
+  PlatformEventSource::GetInstance()->AddPlatformEventDispatcher(dispatcher);
+}
+
 }  // namespace
 
 class TestPlatformEventSource : public PlatformEventSource {
@@ -302,6 +316,170 @@ TEST_F(PlatformEventTest, OverriddenDispatcherInvokeDefaultDispatcher) {
   EXPECT_EQ(10, list[2]);
 }
 
+// Runs a callback during an event dispatch.
+class RunCallbackDuringDispatch : public TestPlatformEventDispatcher {
+ public:
+  RunCallbackDuringDispatch(int id, std::vector<int>* list)
+      : TestPlatformEventDispatcher(id, list) {}
+  virtual ~RunCallbackDuringDispatch() {}
+
+  void set_callback(const base::Closure& callback) {
+    callback_ = callback;
+  }
+
+ protected:
+  // PlatformEventDispatcher:
+  virtual uint32_t DispatchEvent(const PlatformEvent& event) OVERRIDE {
+    if (!callback_.is_null())
+      callback_.Run();
+    return TestPlatformEventDispatcher::DispatchEvent(event);
+  }
+
+ private:
+  base::Closure callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(RunCallbackDuringDispatch);
+};
+
+// Test that if a dispatcher removes another dispatcher that is later in the
+// dispatcher list during dispatching an event, then event dispatching still
+// continues correctly.
+TEST_F(PlatformEventTest, DispatcherRemovesNextDispatcherDuringDispatch) {
+  std::vector<int> list;
+  TestPlatformEventDispatcher first(10, &list);
+  RunCallbackDuringDispatch second(15, &list);
+  TestPlatformEventDispatcher third(20, &list);
+  TestPlatformEventDispatcher fourth(30, &list);
+
+  second.set_callback(base::Bind(&RemoveDispatcher, base::Unretained(&third)));
+
+  scoped_ptr<PlatformEvent> event(CreatePlatformEvent());
+  source()->Dispatch(*event);
+  // |second| removes |third| from the dispatcher list during dispatch. So the
+  // event should only reach |first|, |second|, and |fourth|.
+  ASSERT_EQ(3u, list.size());
+  EXPECT_EQ(10, list[0]);
+  EXPECT_EQ(15, list[1]);
+  EXPECT_EQ(30, list[2]);
+}
+
+// Tests that if a dispatcher removes itself from the dispatcher list during
+// dispatching an event, then event dispatching continues correctly.
+TEST_F(PlatformEventTest, DispatcherRemovesSelfDuringDispatch) {
+  std::vector<int> list;
+  TestPlatformEventDispatcher first(10, &list);
+  RunCallbackDuringDispatch second(15, &list);
+  TestPlatformEventDispatcher third(20, &list);
+
+  second.set_callback(base::Bind(&RemoveDispatcher, base::Unretained(&second)));
+
+  scoped_ptr<PlatformEvent> event(CreatePlatformEvent());
+  source()->Dispatch(*event);
+  // |second| removes itself from the dispatcher list during dispatch. So the
+  // event should reach all three dispatchers in the list.
+  ASSERT_EQ(3u, list.size());
+  EXPECT_EQ(10, list[0]);
+  EXPECT_EQ(15, list[1]);
+  EXPECT_EQ(20, list[2]);
+}
+
+// Tests that if a dispatcher removes itself from the dispatcher list during
+// dispatching an event, and this dispatcher is last in the dispatcher-list,
+// then event dispatching ends correctly.
+TEST_F(PlatformEventTest, DispatcherRemovesSelfDuringDispatchLast) {
+  std::vector<int> list;
+  TestPlatformEventDispatcher first(10, &list);
+  RunCallbackDuringDispatch second(15, &list);
+
+  second.set_callback(base::Bind(&RemoveDispatcher, base::Unretained(&second)));
+
+  scoped_ptr<PlatformEvent> event(CreatePlatformEvent());
+  source()->Dispatch(*event);
+  // |second| removes itself during dispatch. So both dispatchers will have
+  // received the event.
+  ASSERT_EQ(2u, list.size());
+  EXPECT_EQ(10, list[0]);
+  EXPECT_EQ(15, list[1]);
+}
+
+// Tests that if a dispatcher removes a single dispatcher that comes before it
+// in the dispatcher list, then dispatch continues correctly.
+TEST_F(PlatformEventTest, DispatcherRemovesPrevDispatcherDuringDispatch) {
+  std::vector<int> list;
+  TestPlatformEventDispatcher first(10, &list);
+  RunCallbackDuringDispatch second(15, &list);
+  TestPlatformEventDispatcher third(20, &list);
+
+  second.set_callback(base::Bind(&RemoveDispatcher, base::Unretained(&first)));
+
+  scoped_ptr<PlatformEvent> event(CreatePlatformEvent());
+  source()->Dispatch(*event);
+  // |second| removes |first| from the dispatcher list during dispatch. The
+  // event should reach all three dispatchers.
+  ASSERT_EQ(3u, list.size());
+  EXPECT_EQ(10, list[0]);
+  EXPECT_EQ(15, list[1]);
+  EXPECT_EQ(20, list[2]);
+}
+
+// Tests that if a dispatcher removes multiple dispatchers that comes before it
+// in the dispatcher list, then dispatch continues correctly.
+TEST_F(PlatformEventTest, DispatcherRemovesPrevDispatchersDuringDispatch) {
+  std::vector<int> list;
+  TestPlatformEventDispatcher first(10, &list);
+  TestPlatformEventDispatcher second(12, &list);
+  RunCallbackDuringDispatch third(15, &list);
+  TestPlatformEventDispatcher fourth(20, &list);
+
+  third.set_callback(base::Bind(&RemoveDispatchers,
+                                base::Unretained(&first),
+                                base::Unretained(&second)));
+
+  scoped_ptr<PlatformEvent> event(CreatePlatformEvent());
+  source()->Dispatch(*event);
+  // |third| removes |first| and |second| from the dispatcher list during
+  // dispatch. The event should reach all three dispatchers.
+  ASSERT_EQ(4u, list.size());
+  EXPECT_EQ(10, list[0]);
+  EXPECT_EQ(12, list[1]);
+  EXPECT_EQ(15, list[2]);
+  EXPECT_EQ(20, list[3]);
+}
+
+// Tests that adding a dispatcher during dispatching an event receives that
+// event.
+TEST_F(PlatformEventTest, DispatcherAddedDuringDispatchReceivesEvent) {
+  std::vector<int> list;
+  TestPlatformEventDispatcher first(10, &list);
+  RunCallbackDuringDispatch second(15, &list);
+  TestPlatformEventDispatcher third(20, &list);
+  TestPlatformEventDispatcher fourth(30, &list);
+  RemoveDispatchers(&third, &fourth);
+
+  scoped_ptr<PlatformEvent> event(CreatePlatformEvent());
+  source()->Dispatch(*event);
+  ASSERT_EQ(2u, list.size());
+  EXPECT_EQ(10, list[0]);
+  EXPECT_EQ(15, list[1]);
+
+  second.set_callback(base::Bind(&AddDispatcher, base::Unretained(&third)));
+  list.clear();
+  source()->Dispatch(*event);
+  ASSERT_EQ(3u, list.size());
+  EXPECT_EQ(10, list[0]);
+  EXPECT_EQ(15, list[1]);
+  EXPECT_EQ(20, list[2]);
+
+  second.set_callback(base::Bind(&AddDispatcher, base::Unretained(&fourth)));
+  list.clear();
+  source()->Dispatch(*event);
+  ASSERT_EQ(4u, list.size());
+  EXPECT_EQ(10, list[0]);
+  EXPECT_EQ(15, list[1]);
+  EXPECT_EQ(20, list[2]);
+  EXPECT_EQ(30, list[3]);
+}
+
 // Provides mechanism for running tests from inside an active message-loop.
 class PlatformEventTestWithMessageLoop : public PlatformEventTest {
  public: