Fix a "benign" data race during thread construction

Fix a "benign" data race during thread construction

This patch fixes data race on Thread::task_runner() which happens during thread
creation:

Origin thread                      New thread
-------------                      ----------
1. Construct a Thread
   |
    '-----------------------------.
                                   |
                                   V

3. Call Thread::task_runner()      2. Bind the message loop on the thread
   => MessageLoop::task_runner()      => MessageLoop::SetTaskRunner()

Steps 2 and 3 introduce a data race with one thread reading the task runner
pointer and the other writing to it.

The race is "benign" because the new thread is setting
MessageLoop::task_runner_ to the same value it already has (it is also
initialized in the constructor).

This patch fixes the problem by not unnecessarily writing to the task runner.

BUG=508789

[Sami, 2015-07-10 09:46:14]

skyostil@chromium.org changed reviewers:
+ danakj@chromium.org

[Sami, 2015-07-10 09:46:30]

PTAL.

[danakj, 2015-07-10 18:32:38]

https://codereview.chromium.org/1232573002/diff/1/base/message_loop/message_l...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1232573002/diff/1/base/message_loop/message_l...
base/message_loop/message_loop.cc:410: unbound_task_runner_ = nullptr;
I'm feeling like this unbound_task_runner_ is providing less value and more
confusion over time.

What is is doing more than a bool bound_to_thread_ that only exists when
DCHECK_IS_ON()?

[Sami, 2015-07-13 10:43:31]

Thanks for taking a look.

https://codereview.chromium.org/1232573002/diff/1/base/message_loop/message_l...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1232573002/diff/1/base/message_loop/message_l...
base/message_loop/message_loop.cc:410: unbound_task_runner_ = nullptr;
On 2015/07/10 18:32:38, danakj wrote:
> I'm feeling like this unbound_task_runner_ is providing less value and more
> confusion over time.
> 
> What is is doing more than a bool bound_to_thread_ that only exists when
> DCHECK_IS_ON()?

Yes, it's not all that useful but I can't think of a simpler way to do this. A
bool won't cut it because we still need an object on which we can call
BindToCurrentThread(). If we changed the type of task_runner_ to something that
has BindToCurrentThread(), then we'd need to leak that interface to the outside
world in SetTaskRunner.

The only other way I can think of is to keep task_runner_ a
SingleThreadTaskRunner and then do a downcast in BindToCurrentThread and add the
debug-only boolean. WDYT?

[danakj, 2015-07-13 18:33:48]

https://codereview.chromium.org/1232573002/diff/1/base/message_loop/message_l...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/1232573002/diff/1/base/message_loop/message_l...
base/message_loop/message_loop.cc:410: unbound_task_runner_ = nullptr;
On 2015/07/13 10:43:31, Sami wrote:
> On 2015/07/10 18:32:38, danakj wrote:
> > I'm feeling like this unbound_task_runner_ is providing less value and more
> > confusion over time.
> > 
> > What is is doing more than a bool bound_to_thread_ that only exists when
> > DCHECK_IS_ON()?
> 
> Yes, it's not all that useful but I can't think of a simpler way to do this. A
> bool won't cut it because we still need an object on which we can call
> BindToCurrentThread(). If we changed the type of task_runner_ to something
that
> has BindToCurrentThread(), then we'd need to leak that interface to the
outside
> world in SetTaskRunner.
> 
> The only other way I can think of is to keep task_runner_ a
> SingleThreadTaskRunner and then do a downcast in BindToCurrentThread and add
the
> debug-only boolean. WDYT?

Ah, I see. LGTM then.

[Sami, 2015-07-13 18:40:48]

Thanks! I'll roll this into the original patch
(https://codereview.chromium.org/998063002/) and reland.