Make ObserverListThreadSafe key its observers by PlatformThreadId instead of MessageLoop.

base/observer_list_threadsafe.h

 // 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.
 
 #ifndef BASE_OBSERVER_LIST_THREADSAFE_H_
 #define BASE_OBSERVER_LIST_THREADSAFE_H_
 #pragma once
 
 #include <algorithm>
 #include <map>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/callback_old.h"
 #include "base/location.h"
 #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"
+#include "base/threading/platform_thread.h"
 
 ///////////////////////////////////////////////////////////////////////////////
 //
 // OVERVIEW:
 //
 //   A thread-safe container for a list of observers.
 //   This is similar to the observer_list (see observer_list.h), but it
 //   is more robust for multi-threaded situations.
 //
 //   The following use cases are supported:
@@ skipping 51 matching lines @@
       NotificationType;
 
   ObserverListThreadSafe()
       : type_(ObserverListBase<ObserverType>::NOTIFY_ALL) {}
   explicit ObserverListThreadSafe(NotificationType type) : type_(type) {}
 
   // 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();
-    // 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);
   }
 
   // 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.
   // 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)
-      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.
         return;
       }
       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.
@@ skipping 89 matching lines @@
   // ObserverList.  This function MUST be called on the thread which owns
   // the unsafe ObserverList.
   template <class Method, class Params>
   void NotifyWrapper(ObserverListContext* context,
       const UnboundMethod<ObserverType, Method, Params>& method) {
 
     // Check that this list still needs notifications.
     {
       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
       // does not match this one, then we do not need to finish this
       // notification.
       if (it == observer_lists_.end() || it->second != context)
         return;
     }
 
     {
       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 (context->list.size() == 0) {
       {
         base::AutoLock lock(list_lock_);
         // 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());
+            observer_lists_.find(base::PlatformThread::CurrentId());
         if (it != observer_lists_.end() && it->second == context)
           observer_lists_.erase(it);
       }
       delete context;
     }
   }
 
-  typedef std::map<MessageLoop*, ObserverListContext*> ObserversListMap;
+  typedef std::map<base::PlatformThreadId, ObserverListContext*>
+      ObserversListMap;
 
   base::Lock list_lock_;  // Protects the observer_lists_.
   ObserversListMap observer_lists_;
   const NotificationType type_;
 
   DISALLOW_COPY_AND_ASSIGN(ObserverListThreadSafe);
 };
 
 #endif  // BASE_OBSERVER_LIST_THREADSAFE_H_