Fix removal of multiple observers in ObserverListThreadSafe.

BUG=55725
TEST=ObserverListThreadSafeTest.RemoveMultipleObservers

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

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

2 files changed, 49 insertions, 5 deletions

diff --git a/base/observer_list_threadsafe.h b/base/observer_list_threadsafe.h
index 94c55a6..d034e6c 100644
--- a/base/observer_list_threadsafe.h
+++ b/base/observer_list_threadsafe.h
@@ -182,15 +182,16 @@ class ObserverListThreadSafe
 
     // If there are no more observers on the list, we can now delete it.
     if (list->size() == 0) {
-#ifndef NDEBUG
       {
         AutoLock lock(list_lock_);
-        // Verify this list is no longer registered.
+        // Remove |list| if it's not already removed.
+        // 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());
-        DCHECK(it == observer_lists_.end() || it->second != list);
+        if (it != observer_lists_.end() && it->second == list)
+          observer_lists_.erase(it);
       }
-#endif
       delete list;
     }
   }
diff --git a/base/observer_list_unittest.cc b/base/observer_list_unittest.cc
index 6982d3d..71c97d3 100644
--- a/base/observer_list_unittest.cc
+++ b/base/observer_list_unittest.cc
@@ -2,9 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/message_loop.h"
 #include "base/observer_list.h"
 #include "base/observer_list_threadsafe.h"
+
+#include <vector>
+
+#include "base/message_loop.h"
 #include "base/platform_thread.h"
 #include "base/ref_counted.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -224,6 +227,46 @@ TEST(ObserverListThreadSafeTest, BasicTest) {
   EXPECT_EQ(d.total, -10);
 }
 
+class FooRemover : public Foo {
+ public:
+  explicit FooRemover(ObserverListThreadSafe<Foo>* list) : list_(list) {}
+  virtual ~FooRemover() {}
+
+  void AddFooToRemove(Foo* foo) {
+    foos_.push_back(foo);
+  }
+
+  virtual void Observe(int x) {
+    std::vector<Foo*> tmp;
+    tmp.swap(foos_);
+    for (std::vector<Foo*>::iterator it = tmp.begin();
+         it != tmp.end(); ++it) {
+      list_->RemoveObserver(*it);
+    }
+  }
+
+ private:
+  const scoped_refptr<ObserverListThreadSafe<Foo> > list_;
+  std::vector<Foo*> foos_;
+};
+
+TEST(ObserverListThreadSafeTest, RemoveMultipleObservers) {
+  MessageLoop loop;
+  scoped_refptr<ObserverListThreadSafe<Foo> > observer_list(
+      new ObserverListThreadSafe<Foo>);
+
+  FooRemover a(observer_list);
+  Adder b(1);
+
+  observer_list->AddObserver(&a);
+  observer_list->AddObserver(&b);
+
+  a.AddFooToRemove(&a);
+  a.AddFooToRemove(&b);
+
+  observer_list->Notify(&Foo::Observe, 1);
+  loop.RunAllPending();
+}
 
 // A test driver for a multi-threaded notification loop.  Runs a number
 // of observer threads, each of which constantly adds/removes itself