SupportsUserData is not thread safe

SupportsUserData is not thread safe

Lets ensure none of its usages are unsafe.

BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155794

[joth, 2012-09-08 17:24:10]

Hi Avi, just copying you in on this as an FYI -- I stumbled across potentially
non-threadsafe usages of SupportsUserData, still trying to smoke out issues this
catches but looks like adding a ThreadChecker to it will be useful step. (It
can't extend NonThreadSafe as that creates multiple inheritance hell for some of
its subclasses that already extended NonThreadSafe)

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

Yikes!

1. Nice catch; the SupportsUserData code LGTM
2. I'm adding some reviewers and some FYI:

jam@ I think is appropriate to look at the other code; it looks plausible to me
but he should be aware of this. rdsmith@, too, because the download system is a
big user of SupportsUserData and they should be aware of the threading issues.

http://codereview.chromium.org/10919137/diff/9002/base/supports_user_data.cc
File base/supports_user_data.cc (right):

http://codereview.chromium.org/10919137/diff/9002/base/supports_user_data.cc#...
base/supports_user_data.cc:10: // Harmless to construct on a different thread to
subsequent usage.
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.

http://codereview.chromium.org/10919137/diff/9002/base/supports_user_data.cc#...
base/supports_user_data.cc:37: DCHECK(thread_checker_.CalledOnValidThread() ||
user_data_.empty());
Not entirely convinced this is technically safe, but I'm not worried
practically.

[Randy Smith (Not in Mondays), 2012-09-08 17:43:31]

Downloads code only uses this on the UI thread; we're ok.  LGTM.

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

Thanks.
Added Darin for base/ review.

http://codereview.chromium.org/10919137/diff/9002/base/supports_user_data.cc
File base/supports_user_data.cc (right):

http://codereview.chromium.org/10919137/diff/9002/base/supports_user_data.cc#...
base/supports_user_data.cc:10: // Harmless to construct on a different thread to
subsequent usage.
On 2012/09/08 17:33:51, Avi wrote:
> 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.

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?)

http://codereview.chromium.org/10919137/diff/9002/base/supports_user_data.cc#...
base/supports_user_data.cc:37: DCHECK(thread_checker_.CalledOnValidThread() ||
user_data_.empty());
On 2012/09/08 17:33:51, Avi wrote:
> Not entirely convinced this is technically safe, but I'm not worried
> practically.

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

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

http://codereview.chromium.org/10919137/diff/9002/base/supports_user_data.cc
File base/supports_user_data.cc (right):

http://codereview.chromium.org/10919137/diff/9002/base/supports_user_data.cc#...
base/supports_user_data.cc:10: // Harmless to construct on a different thread to
subsequent usage.
On 2012/09/08 21:33:41, joth wrote:
> I'm assuming the act of
> sending the pointer to the other thread causes necessary memory barriers

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.

> then the base SupportsUserData will have been constructed

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.

[darin (slow to review), 2012-09-10 06:20:23]

On 2012/09/09 00:03:58, Avi wrote:
> http://codereview.chromium.org/10919137/diff/9002/base/supports_user_data.cc
> File base/supports_user_data.cc (right):
> 
>
http://codereview.chromium.org/10919137/diff/9002/base/supports_user_data.cc#...
> base/supports_user_data.cc:10: // Harmless to construct on a different thread
to
> subsequent usage.
> On 2012/09/08 21:33:41, joth wrote:
> > I'm assuming the act of
> > sending the pointer to the other thread causes necessary memory barriers
> 
> 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.

I could be wrong, but my understanding is that acquiring the mutex used to
protect the task queue counts as a memory barrier.  See
MessageLoop::incoming_queue_lock_.

[Avi (use Gerrit), 2012-09-10 14:17:34]

Good point. pthread mutexes apparently guarantee a memory barrier.

SLGTM. Still freaks me out that threading issues are so easy to get wrong... :(

[joth, 2012-09-10 16:26:58]

On 2012/09/10 14:17:34, Avi wrote:
> Good point. pthread mutexes apparently guarantee a memory barrier.
> 
> SLGTM. Still freaks me out that threading issues are so easy to get wrong...
:(

Thanks Avi.

Darin -- just need a base approval.

[darin (slow to review), 2012-09-10 16:40:18]

LGTM for src/base/

[commit-bot: I haz the power, 2012-09-10 16:54:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joth@chromium.org/10919137/9002

[commit-bot: I haz the power, 2012-09-10 19:18:56]

Change committed as 155794