diff options
| -rw-r--r-- | base/observer_list_threadsafe.h | 35 | ||||
| -rw-r--r-- | base/observer_list_unittest.cc | 51 |
2 files changed, 70 insertions, 16 deletions
diff --git a/base/observer_list_threadsafe.h b/base/observer_list_threadsafe.h index 6a22823..7a1d81b 100644 --- a/base/observer_list_threadsafe.h +++ b/base/observer_list_threadsafe.h @@ -19,6 +19,7 @@ #include "base/message_loop_proxy.h" #include "base/observer_list.h" #include "base/task.h" +#include "base/threading/platform_thread.h" /////////////////////////////////////////////////////////////////////////////// // @@ -89,18 +90,18 @@ class ObserverListThreadSafe // Add an observer to the list. An observer should not be added to // the same list more than once. void AddObserver(ObserverType* obs) { + // If there is not a current MessageLoop, it is impossible to notify on it, + // so do not add the observer. + if (!MessageLoop::current()) + return; + ObserverList<ObserverType>* list = NULL; - MessageLoop* loop = MessageLoop::current(); - // TODO(mbelshe): Get rid of this check. Its needed right now because - // Time currently triggers usage of the ObserverList. - // And unittests use time without a MessageLoop. - if (!loop) - return; // Some unittests may access this without a message loop. + base::PlatformThreadId thread_id = base::PlatformThread::CurrentId(); { base::AutoLock lock(list_lock_); - if (observer_lists_.find(loop) == observer_lists_.end()) - observer_lists_[loop] = new ObserverListContext(type_); - list = &(observer_lists_[loop]->list); + if (observer_lists_.find(thread_id) == observer_lists_.end()) + observer_lists_[thread_id] = new ObserverListContext(type_); + list = &(observer_lists_[thread_id]->list); } list->AddObserver(obs); } @@ -113,12 +114,10 @@ class ObserverListThreadSafe void RemoveObserver(ObserverType* obs) { ObserverListContext* context = NULL; ObserverList<ObserverType>* list = NULL; - MessageLoop* loop = MessageLoop::current(); - if (!loop) - return; // On shutdown, it is possible that current() is already null. + base::PlatformThreadId thread_id = base::PlatformThread::CurrentId(); { base::AutoLock lock(list_lock_); - typename ObserversListMap::iterator it = observer_lists_.find(loop); + typename ObserversListMap::iterator it = observer_lists_.find(thread_id); if (it == observer_lists_.end()) { // This will happen if we try to remove an observer on a thread // we never added an observer for. @@ -228,7 +227,7 @@ class ObserverListThreadSafe { base::AutoLock lock(list_lock_); typename ObserversListMap::iterator it = - observer_lists_.find(MessageLoop::current()); + observer_lists_.find(base::PlatformThread::CurrentId()); // The ObserverList could have been removed already. In fact, it could // have been removed and then re-added! If the master list's loop @@ -253,7 +252,7 @@ class ObserverListThreadSafe // This can happen if multiple observers got removed in a notification. // See http://crbug.com/55725. typename ObserversListMap::iterator it = - observer_lists_.find(MessageLoop::current()); + observer_lists_.find(base::PlatformThread::CurrentId()); if (it != observer_lists_.end() && it->second == context) observer_lists_.erase(it); } @@ -261,7 +260,11 @@ class ObserverListThreadSafe } } - typedef std::map<MessageLoop*, ObserverListContext*> ObserversListMap; + // Key by PlatformThreadId because in tests, clients can attempt to remove + // observers without a MessageLoop. If this were keyed by MessageLoop, that + // operation would be silently ignored, leaving garbage in the ObserverList. + typedef std::map<base::PlatformThreadId, ObserverListContext*> + ObserversListMap; base::Lock list_lock_; // Protects the observer_lists_. ObserversListMap observer_lists_; diff --git a/base/observer_list_unittest.cc b/base/observer_list_unittest.cc index 34cc485..f7fc1cb 100644 --- a/base/observer_list_unittest.cc +++ b/base/observer_list_unittest.cc @@ -258,6 +258,57 @@ TEST(ObserverListThreadSafeTest, RemoveObserver) { EXPECT_EQ(b.total, 0); } +TEST(ObserverListThreadSafeTest, WithoutMessageLoop) { + scoped_refptr<ObserverListThreadSafe<Foo> > observer_list( + new ObserverListThreadSafe<Foo>); + + Adder a(1), b(1), c(1); + + // No MessageLoop, so these should not be added. + observer_list->AddObserver(&a); + observer_list->AddObserver(&b); + + { + // Add c when there's a loop. + MessageLoop loop; + observer_list->AddObserver(&c); + + observer_list->Notify(&Foo::Observe, 10); + loop.RunAllPending(); + + EXPECT_EQ(0, a.total); + EXPECT_EQ(0, b.total); + EXPECT_EQ(10, c.total); + + // Now add a when there's a loop. + observer_list->AddObserver(&a); + + // Remove c when there's a loop. + observer_list->RemoveObserver(&c); + + // Notify again. + observer_list->Notify(&Foo::Observe, 20); + loop.RunAllPending(); + + EXPECT_EQ(20, a.total); + EXPECT_EQ(0, b.total); + EXPECT_EQ(10, c.total); + } + + // Removing should always succeed with or without a loop. + observer_list->RemoveObserver(&a); + + // Notifying should not fail but should also be a no-op. + MessageLoop loop; + observer_list->AddObserver(&b); + observer_list->Notify(&Foo::Observe, 30); + loop.RunAllPending(); + + EXPECT_EQ(20, a.total); + EXPECT_EQ(30, b.total); + EXPECT_EQ(10, c.total); +} + class FooRemover : public Foo { public: explicit FooRemover(ObserverListThreadSafe<Foo>* list) : list_(list) {} |