Fix bug in ObserverListThreadsafe::RemoveObserver

Like ObserverList, ObserverListThreadSafe::RemoveObserver should do
nothing if the observer isn't in the list.  Instead, it causes weird
bugs if the list has 0 or 1 existing elements.

Add comments clarifying semantics of AddObserver/RemoveObserver.

Change AddObserver to do nothing in release mode if it tries to add an existing observer.

BUG=84922
TEST=


Review URL: http://codereview.chromium.org/7024037

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

3 files changed, 55 insertions, 15 deletions

diff --git a/base/observer_list.h b/base/observer_list.h
index ca4b3fd..b8c2ae4 100644
--- a/base/observer_list.h
+++ b/base/observer_list.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -111,14 +111,18 @@ class ObserverListBase {
   explicit ObserverListBase(NotificationType type)
       : notify_depth_(0), type_(type) {}
 
-  // Add an observer to the list.
+  // Add an observer to the list.  An observer should not be added to
+  // the same list more than once.
   void AddObserver(ObserverType* obs) {
-    DCHECK(find(observers_.begin(), observers_.end(), obs) == observers_.end())
-        << "Observers can only be added once!";
+    if (std::find(observers_.begin(), observers_.end(), obs)
+        != observers_.end()) {
+      NOTREACHED() << "Observers can only be added once!";
+      return;
+    }
     observers_.push_back(obs);
   }
 
-  // Remove an observer from the list.
+  // Remove an observer from the list if it is in the list.
   void RemoveObserver(ObserverType* obs) {
     typename ListType::iterator it =
       std::find(observers_.begin(), observers_.end(), obs);
diff --git a/base/observer_list_threadsafe.h b/base/observer_list_threadsafe.h
index 31e0cae..883e285 100644
--- a/base/observer_list_threadsafe.h
+++ b/base/observer_list_threadsafe.h
@@ -83,7 +83,8 @@ class ObserverListThreadSafe
       : type_(ObserverListBase<ObserverType>::NOTIFY_ALL) {}
   explicit ObserverListThreadSafe(NotificationType type) : type_(type) {}
 
-  // Add an observer to the list.
+  // Add an observer to the list.  An observer should not be added to
+  // the same list more than once.
   void AddObserver(ObserverType* obs) {
     ObserverList<ObserverType>* list = NULL;
     MessageLoop* loop = MessageLoop::current();
@@ -101,11 +102,11 @@ class ObserverListThreadSafe
     list->AddObserver(obs);
   }
 
-  // Remove an observer from the list.
+  // Remove an observer from the list if it is in the list.
   // If there are pending notifications in-transit to the observer, they will
   // be aborted.
-  // RemoveObserver MUST be called from the same thread which called
-  // AddObserver.
+  // If the observer to be removed is in the list, RemoveObserver MUST
+  // be called from the same thread which called AddObserver.
   void RemoveObserver(ObserverType* obs) {
     ObserverList<ObserverType>* list = NULL;
     MessageLoop* loop = MessageLoop::current();
@@ -113,16 +114,18 @@ class ObserverListThreadSafe
       return;  // On shutdown, it is possible that current() is already null.
     {
       base::AutoLock lock(list_lock_);
-      list = observer_lists_[loop];
-      if (!list) {
-        NOTREACHED() << "RemoveObserver called on for unknown thread";
+      typename ObserversListMap::iterator it = observer_lists_.find(loop);
+      if (it == observer_lists_.end()) {
+        // This may happen if we try to remove an observer on a thread
+        // we never added an observer for.
         return;
       }
+      list = it->second;
 
       // If we're about to remove the last observer from the list,
       // then we can remove this observer_list entirely.
-      if (list->size() == 1)
-        observer_lists_.erase(loop);
+      if (list->HasObserver(obs) && list->size() == 1)
+        observer_lists_.erase(it);
     }
     list->RemoveObserver(obs);
 
diff --git a/base/observer_list_unittest.cc b/base/observer_list_unittest.cc
index d313367..8315a2b 100644
--- a/base/observer_list_unittest.cc
+++ b/base/observer_list_unittest.cc
@@ -175,7 +175,7 @@ class AddRemoveThread : public PlatformThread::Delegate,
 
 TEST(ObserverListTest, BasicTest) {
   ObserverList<Foo> observer_list;
-  Adder a(1), b(-1), c(1), d(-1);
+  Adder a(1), b(-1), c(1), d(-1), e(-1);
   Disrupter evil(&observer_list, &c);
 
   observer_list.AddObserver(&a);
@@ -187,12 +187,16 @@ TEST(ObserverListTest, BasicTest) {
   observer_list.AddObserver(&c);
   observer_list.AddObserver(&d);
 
+  // Removing an observer not in the list should do nothing.
+  observer_list.RemoveObserver(&e);
+
   FOR_EACH_OBSERVER(Foo, observer_list, Observe(10));
 
   EXPECT_EQ(a.total, 20);
   EXPECT_EQ(b.total, -20);
   EXPECT_EQ(c.total, 0);
   EXPECT_EQ(d.total, -10);
+  EXPECT_EQ(e.total, 0);
 }
 
 TEST(ObserverListThreadSafeTest, BasicTest) {
@@ -225,6 +229,35 @@ TEST(ObserverListThreadSafeTest, BasicTest) {
   EXPECT_EQ(d.total, -10);
 }
 
+TEST(ObserverListThreadSafeTest, RemoveObserver) {
+  MessageLoop loop;
+
+  scoped_refptr<ObserverListThreadSafe<Foo> > observer_list(
+      new ObserverListThreadSafe<Foo>);
+  Adder a(1), b(1);
+
+  // Should do nothing.
+  observer_list->RemoveObserver(&a);
+  observer_list->RemoveObserver(&b);
+
+  observer_list->Notify(&Foo::Observe, 10);
+  loop.RunAllPending();
+
+  EXPECT_EQ(a.total, 0);
+  EXPECT_EQ(b.total, 0);
+
+  observer_list->AddObserver(&a);
+
+  // Should also do nothing.
+  observer_list->RemoveObserver(&b);
+
+  observer_list->Notify(&Foo::Observe, 10);
+  loop.RunAllPending();
+
+  EXPECT_EQ(a.total, 10);
+  EXPECT_EQ(b.total, 0);
+}
+
 class FooRemover : public Foo {
  public:
   explicit FooRemover(ObserverListThreadSafe<Foo>* list) : list_(list) {}