Use MessageLoopProxy instead of MessageLoop to dispatch notifications in ObserverListThreadsafe.

This is nearly identical to the reverted r96013, with the modification that
std::map::insert isn't used.  Its invocation was complex (such that
it worked in some compilers but not in others) and the performance gain is
negligible in any use case I've seen in Chromium.

BUG=91589

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

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

base/observer_list_threadsafe.h

@@ -14,6 +14,7 @@
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop.h"
+#include "base/message_loop_proxy.h"
 #include "base/observer_list.h"
 #include "base/task.h"
 
@@ -96,8 +97,8 @@ class ObserverListThreadSafe
     {
       base::AutoLock lock(list_lock_);
       if (observer_lists_.find(loop) == observer_lists_.end())
-        observer_lists_[loop] = new ObserverList<ObserverType>(type_);
-      list = observer_lists_[loop];
+        observer_lists_[loop] = new ObserverListContext(type_);
+      list = &(observer_lists_[loop]->list);
     }
     list->AddObserver(obs);
   }
@@ -108,6 +109,7 @@ class ObserverListThreadSafe
   // 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) {
+    ObserverListContext* context = NULL;
     ObserverList<ObserverType>* list = NULL;
     MessageLoop* loop = MessageLoop::current();
     if (!loop)
@@ -116,11 +118,12 @@ class ObserverListThreadSafe
       base::AutoLock lock(list_lock_);
       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
+        // This will happen if we try to remove an observer on a thread
         // we never added an observer for.
         return;
       }
-      list = it->second;
+      context = it->second;
+      list = &context->list;
 
       // If we're about to remove the last observer from the list,
       // then we can remove this observer_list entirely.
@@ -133,7 +136,7 @@ class ObserverListThreadSafe
     // nonzero.  Instead of deleting here, the NotifyWrapper will delete
     // when it finishes iterating.
     if (list->size() == 0)
-      delete list;
+      delete context;
   }
 
   // Notify methods.
@@ -180,6 +183,18 @@ class ObserverListThreadSafe
   // See comment above ObserverListThreadSafeTraits' definition.
   friend struct ObserverListThreadSafeTraits<ObserverType>;
 
+  struct ObserverListContext {
+    explicit ObserverListContext(NotificationType type)
+        : loop(base::MessageLoopProxy::CreateForCurrentThread()),
+          list(type) {
+    }
+
+    scoped_refptr<base::MessageLoopProxy> loop;
+    ObserverList<ObserverType> list;
+
+    DISALLOW_COPY_AND_ASSIGN(ObserverListContext);
+  };
+
   ~ObserverListThreadSafe() {
     typename ObserversListMap::const_iterator it;
     for (it = observer_lists_.begin(); it != observer_lists_.end(); ++it)
@@ -192,13 +207,12 @@ class ObserverListThreadSafe
     base::AutoLock lock(list_lock_);
     typename ObserversListMap::iterator it;
     for (it = observer_lists_.begin(); it != observer_lists_.end(); ++it) {
-      MessageLoop* loop = (*it).first;
-      ObserverList<ObserverType>* list = (*it).second;
-      loop->PostTask(
+      ObserverListContext* context = (*it).second;
+      context->loop->PostTask(
           FROM_HERE,
           NewRunnableMethod(this,
               &ObserverListThreadSafe<ObserverType>::
-                 template NotifyWrapper<Method, Params>, list, method));
+                 template NotifyWrapper<Method, Params>, context, method));
     }
   }
 
@@ -206,7 +220,7 @@ class ObserverListThreadSafe
   // ObserverList.  This function MUST be called on the thread which owns
   // the unsafe ObserverList.
   template <class Method, class Params>
-  void NotifyWrapper(ObserverList<ObserverType>* list,
+  void NotifyWrapper(ObserverListContext* context,
       const UnboundMethod<ObserverType, Method, Params>& method) {
 
     // Check that this list still needs notifications.
@@ -219,19 +233,19 @@ class ObserverListThreadSafe
       // have been removed and then re-added!  If the master list's loop
       // does not match this one, then we do not need to finish this
       // notification.
-      if (it == observer_lists_.end() || it->second != list)
+      if (it == observer_lists_.end() || it->second != context)
         return;
     }
 
     {
-      typename ObserverList<ObserverType>::Iterator it(*list);
+      typename ObserverList<ObserverType>::Iterator it(context->list);
       ObserverType* obs;
       while ((obs = it.GetNext()) != NULL)
         method.Run(obs);
     }
 
     // If there are no more observers on the list, we can now delete it.
-    if (list->size() == 0) {
+    if (context->list.size() == 0) {
       {
         base::AutoLock lock(list_lock_);
         // Remove |list| if it's not already removed.
@@ -239,16 +253,15 @@ class ObserverListThreadSafe
         // See http://crbug.com/55725.
         typename ObserversListMap::iterator it =
             observer_lists_.find(MessageLoop::current());
-        if (it != observer_lists_.end() && it->second == list)
+        if (it != observer_lists_.end() && it->second == context)
           observer_lists_.erase(it);
       }
-      delete list;
+      delete context;
     }
   }
 
-  typedef std::map<MessageLoop*, ObserverList<ObserverType>*> ObserversListMap;
+  typedef std::map<MessageLoop*, ObserverListContext*> ObserversListMap;
 
-  // These are marked mutable to facilitate having NotifyAll be const.
   base::Lock list_lock_;  // Protects the observer_lists_.
   ObserversListMap observer_lists_;
   const NotificationType type_;

base/observer_list_unittest.cc

@@ -61,9 +61,10 @@ class ThreadSafeDisrupter : public Foo {
   Foo* doomed_;
 };
 
+template <typename ObserverListType>
 class AddInObserve : public Foo {
  public:
-  explicit AddInObserve(ObserverList<Foo>* observer_list)
+  explicit AddInObserve(ObserverListType* observer_list)
       : added(false),
         observer_list(observer_list),
         adder(1) {
@@ -76,14 +77,11 @@ class AddInObserve : public Foo {
   }
 
   bool added;
-  ObserverList<Foo>* observer_list;
+  ObserverListType* observer_list;
   Adder adder;
 };
 
 
-class ObserverListThreadSafeTest : public testing::Test {
-};
-
 static const int kThreadRunTime = 2000;  // ms to run the multi-threaded test.
 
 // A thread for use in the ThreadSafeObserver test
@@ -357,10 +355,22 @@ TEST(ObserverListThreadSafeTest, CrossThreadNotifications) {
   ThreadSafeObserverHarness(3, true);
 }
 
+TEST(ObserverListThreadSafeTest, OutlivesMessageLoop) {
+  MessageLoop* loop = new MessageLoop;
+  scoped_refptr<ObserverListThreadSafe<Foo> > observer_list(
+      new ObserverListThreadSafe<Foo>);
+
+  Adder a(1);
+  observer_list->AddObserver(&a);
+  delete loop;
+  // Test passes if we don't crash here.
+  observer_list->Notify(&Foo::Observe, 1);
+}
+
 TEST(ObserverListTest, Existing) {
   ObserverList<Foo> observer_list(ObserverList<Foo>::NOTIFY_EXISTING_ONLY);
   Adder a(1);
-  AddInObserve b(&observer_list);
+  AddInObserve<ObserverList<Foo> > b(&observer_list);
 
   observer_list.AddObserver(&a);
   observer_list.AddObserver(&b);
@@ -377,6 +387,31 @@ TEST(ObserverListTest, Existing) {
   EXPECT_EQ(1, b.adder.total);
 }
 
+// Same as above, but for ObserverListThreadSafe
+TEST(ObserverListThreadSafeTest, Existing) {
+  MessageLoop loop;
+  scoped_refptr<ObserverListThreadSafe<Foo> > observer_list(
+      new ObserverListThreadSafe<Foo>(ObserverList<Foo>::NOTIFY_EXISTING_ONLY));
+  Adder a(1);
+  AddInObserve<ObserverListThreadSafe<Foo> > b(observer_list.get());
+
+  observer_list->AddObserver(&a);
+  observer_list->AddObserver(&b);
+
+  observer_list->Notify(&Foo::Observe, 1);
+  loop.RunAllPending();
+
+  EXPECT_TRUE(b.added);
+  // B's adder should not have been notified because it was added during
+  // notificaiton.
+  EXPECT_EQ(0, b.adder.total);
+
+  // Notify again to make sure b's adder is notified.
+  observer_list->Notify(&Foo::Observe, 1);
+  loop.RunAllPending();
+  EXPECT_EQ(1, b.adder.total);
+}
+
 class AddInClearObserve : public Foo {
  public:
   explicit AddInClearObserve(ObserverList<Foo>* list)