SupportsUserData is not thread safe

base/supports_user_data.cc

Index: base/supports_user_data.cc
diff --git a/base/supports_user_data.cc b/base/supports_user_data.cc
index b2f1228ce341295f2eba55271b371551df16613d..2a0263ed0d1881e6552f04b44a34fadf3e441cb2 100644
--- a/base/supports_user_data.cc
+++ b/base/supports_user_data.cc
@@ -6,9 +6,13 @@
 
 namespace base {
 
-SupportsUserData::SupportsUserData() {}
+SupportsUserData::SupportsUserData() {
+  // Harmless to construct on a different thread to subsequent usage.

[Avi (use Gerrit), 2012/09/08 17:33:51]

Technically speaking, it might not be. If it's immediately handed off to a
different thread and used, accesses from the other thread might hit up a
partially constructed map. I'm not too concerned about that scenario, though.

[joth, 2012/09/08 21:33:41]

Even if the subclass sends |this| to another thread in its constructor, then the
base SupportsUserData will have been constructed. I'm assuming the act of
sending the pointer to the other thread causes necessary memory barriers: AIUI
we rely on this throughout the code base (construct a Foo and immediate post it
off to another thread?)

[Avi (use Gerrit), 2012/09/09 00:03:58]

That's an assumption that surprises me. Do we put memory barriers in our
inter-thread messaging system? I'm doubting that, though I'm welcome to be
surprised. It certainly wouldn't be the compiler's doing.
That's not actually true. It's not that compiler has to emit code that
constructs the superclass before the subclass, it's that it has to emit code
that behaves _as_if_ it did. Add in things like out-of-order execution of memory
accesses, and you're not so sure any more.

There's two sides to this thing. Theoretical, in which case _any_ accessing of
pointers across threads requires synchronization, and actual, in which case I'm
not really too worried about the theoretical dangers.

But they are there.

+  thread_checker_.DetachFromThread();
+}
 
 SupportsUserData::Data* SupportsUserData::GetUserData(const void* key) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DataMap::const_iterator found = user_data_.find(key);
   if (found != user_data_.end())
     return found->second.get();
@@ -16,13 +20,21 @@ SupportsUserData::Data* SupportsUserData::GetUserData(const void* key) const {
 }
 
 void SupportsUserData::SetUserData(const void* key, Data* data) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   user_data_[key] = linked_ptr<Data>(data);
 }
 
 void SupportsUserData::RemoveUserData(const void* key) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   user_data_.erase(key);
 }
 
-SupportsUserData::~SupportsUserData() {}
+void SupportsUserData::DetachUserDataThread() {
+  thread_checker_.DetachFromThread();
+}
+
+SupportsUserData::~SupportsUserData() {
+  DCHECK(thread_checker_.CalledOnValidThread() || user_data_.empty());

[Avi (use Gerrit), 2012/09/08 17:33:51]

Not entirely convinced this is technically safe, but I'm not worried
practically.

[joth, 2012/09/08 21:33:41]

yep... I converged on the same compromise in NotificationRegistrar d'tor.

+}
 
 }  // namespace base