Make ObserverListThreadSafe key its observers by PlatformThreadId instead of MessageLoop.

base/observer_list_threadsafe.h

Index: base/observer_list_threadsafe.h
diff --git a/base/observer_list_threadsafe.h b/base/observer_list_threadsafe.h
index 6a228233b8bd5b5e6af62bc31133f1ac9e5d9bff..2fcf04e5cb43e35f53cda2e81696dede0e8b6375 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"
 
 ///////////////////////////////////////////////////////////////////////////////
 //
@@ -90,17 +91,12 @@ class ObserverListThreadSafe
   // the same list more than once.
   void AddObserver(ObserverType* obs) {
     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_);

[willchan no longer on Chromium, 2011/11/22 19:15:23]

It's not clear to me that this is safe. Won't ObserverListContext construct a
NULL MessageLoopProxy if MessageLoop::current() is NULL? Later on, we might
crash on Notify() then. Is that desired behavior?

[Robert Sesek, 2011/11/22 19:24:39]

You're right that it may crash if the current loop is NULL, but it seems the
comment above is no longer applicable as all tests pass, which is the only
context in which an observer could be added without a loop.

That said, I'd be OK with re-adding the check in AddObserver; the real
problematic piece of code is dropping RemoveObserver ops when there's no loop
because it leaves garbage pointers around. When combined with TCMalloc's address
reuse, it means that something being added to the list can technically be "found
in it" even if the object is theoretically garbage.

Alternatively, the Notify variants could ensure that the loop is non-NULL before
posting.

[willchan no longer on Chromium, 2011/11/22 19:31:24]

Yep, it looks like the time implementation changed, so the comment isn't valid.
I prefer the former solution, not adding to the ObserverList if the MessageLoop
is NULL. Can you add a test for this case?

[Robert Sesek, 2011/11/22 20:48:51]

Done.

I also tried to add a test for the broken behavior before, but it's pretty darn
impossible without reaching directly into the ObserverListThreadSafe internals.

+      list = &(observer_lists_[thread_id]->list);
     }
     list->AddObserver(obs);
   }
@@ -113,12 +109,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 +222,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 +247,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 +255,8 @@ class ObserverListThreadSafe
     }
   }
 
-  typedef std::map<MessageLoop*, ObserverListContext*> ObserversListMap;
+  typedef std::map<base::PlatformThreadId, ObserverListContext*>
+      ObserversListMap;
 
   base::Lock list_lock_;  // Protects the observer_lists_.
   ObserversListMap observer_lists_;