Fix race on mojo message pump shutdown.

Fix race on mojo message pump shutdown.

Calling |MessagePump::Quit()| from a Thread's message loop races with any
caller of |Thread::message_loop()| and |Thread::task_runner()| (see comment for
|Thread::message_loop()|). This fixes the problem by allowing the mojo message
pump to continue operating as a default message pump after the Mojo EDK has been
shut down, allowing the normal thread shutdown process to occur.

Committed: https://crrev.com/f575e10a19933cd251663358bd83639a8aa18ac4
Cr-Commit-Position: refs/heads/master@{#368536}

[Anand Mistry (off Chromium), 2016-01-06 00:32:45]

amistry@chromium.org changed reviewers:
+ jam@chromium.org

[Anand Mistry (off Chromium), 2016-01-06 00:32:46]

Ideally, I'd like to delete Thread::message_loop() and modify
Thread::task_runner() to not cause issues with MessagePump::Quit(). But that may
not be possible.

[jam, 2016-01-07 00:38:05]

Thanks for the fix.

Since there's no bug or repro instructions, I'm not sure I understand the
problem. What does the callstack look like?
also you probably want to undo
https://codereview.chromium.org/1501343002/diff/220001/base/message_loop/mess...
as well?

[Anand Mistry (off Chromium), 2016-01-07 02:50:58]

On 2016/01/07 00:38:05, jam wrote:
> Thanks for the fix.
> 
> Since there's no bug or repro instructions, I'm not sure I understand the
> problem. What does the callstack look like?
> also you probably want to undo
>
https://codereview.chromium.org/1501343002/diff/220001/base/message_loop/mess...
> as well?

So, I haven't come across a crash. I noticed this as I was changing
MessagePumpMojo for WaitSet. There's a TSAN suppression, but if I remove it, I
get:
WARNING: ThreadSanitizer: data race (pid=35860)
  Read of size 8 at 0x7d4400009678 by main thread (mutexes: write M7657, write
M8043):
    #0 task_runner
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/threading/thread.h:168:12
(libbase.so+0x00000011c3a9)
    #1 base::Thread::StopSoon()
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/threading/thread.cc:176
(libbase.so+0x00000011c3a9)
    #2 base::Thread::Stop()
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/threading/thread.cc:151:3
(libbase.so+0x00000011bf5c)
    #3 ~WatcherThreadManager
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../mojo/message_pump/handle_watcher.cc:220:3
(libmessage_pump.so+0x000000006338)
...
  Previous write of size 8 at 0x7d4400037578 by thread T7:
    #0 base::Thread::ThreadMain()
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/threading/thread.cc:275:17
(libbase.so+0x00000011c7a4)
    #1 base::(anonymous namespace)::ThreadFunc(void*)
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/threading/platform_thread_posix.cc:67:3
(libbase.so+0x000000113748)

There's also a race with
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/message_pump/...
where if the message pump thread finishes while this statement runs, there may
be either a nullptr dereference in accessing the message loop's task runner, or
task_runner itself could return nullptr and generate a SEGV when trying to post
the task.

[jam, 2016-01-08 17:47:37]

On 2016/01/07 02:50:58, Anand Mistry wrote:
> On 2016/01/07 00:38:05, jam wrote:
> > Thanks for the fix.
> > 
> > Since there's no bug or repro instructions, I'm not sure I understand the
> > problem. What does the callstack look like?
> > also you probably want to undo
> >
>
https://codereview.chromium.org/1501343002/diff/220001/base/message_loop/mess...
> > as well?
> 
> So, I haven't come across a crash. I noticed this as I was changing
> MessagePumpMojo for WaitSet. There's a TSAN suppression, but if I remove it, I
> get:
> WARNING: ThreadSanitizer: data race (pid=35860)
>   Read of size 8 at 0x7d4400009678 by main thread (mutexes: write M7657, write
> M8043):
>     #0 task_runner
>
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/threading/thread.h:168:12
> (libbase.so+0x00000011c3a9)
>     #1 base::Thread::StopSoon()
>
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/threading/thread.cc:176
> (libbase.so+0x00000011c3a9)
>     #2 base::Thread::Stop()
>
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/threading/thread.cc:151:3
> (libbase.so+0x00000011bf5c)
>     #3 ~WatcherThreadManager
>
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../mojo/message_pump/handle_watcher.cc:220:3
> (libmessage_pump.so+0x000000006338)
> ...
>   Previous write of size 8 at 0x7d4400037578 by thread T7:
>     #0 base::Thread::ThreadMain()
>
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/threading/thread.cc:275:17
> (libbase.so+0x00000011c7a4)
>     #1 base::(anonymous namespace)::ThreadFunc(void*)
>
/usr/local/google/home/amistry/chrome-source/src/out/Release.gn/../../base/threading/platform_thread_posix.cc:67:3
> (libbase.so+0x000000113748)
> 
> There's also a race with
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/message_pump/...
> where if the message pump thread finishes while this statement runs, there may
> be either a nullptr dereference in accessing the message loop's task runner,
or
> task_runner itself could return nullptr and generate a SEGV when trying to
post
> the task.

I see. ok please remove the now unnecessary line in this link, and also the
other one I referenced earlier (both were due to my change making the message
loop quit early).

[jam, 2016-01-11 04:50:15]

lgtm

[Anand Mistry (off Chromium), 2016-01-11 06:25:43]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2016-01-11 06:26:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1566573002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1566573002/20001

[commit-bot: I haz the power, 2016-01-11 09:06:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-11 09:08:50]

Description was changed from

==========
Fix race on mojo message pump shutdown.

Calling |MessagePump::Quit()| from a Thread's message loop races with any
caller of |Thread::message_loop()| and |Thread::task_runner()| (see comment for
|Thread::message_loop()|). This fixes the problem by allowing the mojo message
pump to continue operating as a default message pump after the Mojo EDK has been
shut down, allowing the normal thread shutdown process to occur.
==========

to

==========
Fix race on mojo message pump shutdown.

Calling |MessagePump::Quit()| from a Thread's message loop races with any
caller of |Thread::message_loop()| and |Thread::task_runner()| (see comment for
|Thread::message_loop()|). This fixes the problem by allowing the mojo message
pump to continue operating as a default message pump after the Mojo EDK has been
shut down, allowing the normal thread shutdown process to occur.

Committed: https://crrev.com/f575e10a19933cd251663358bd83639a8aa18ac4
Cr-Commit-Position: refs/heads/master@{#368536}
==========

[commit-bot: I haz the power, 2016-01-11 09:08:51]

Patchset 2 (id:??) landed as
https://crrev.com/f575e10a19933cd251663358bd83639a8aa18ac4
Cr-Commit-Position: refs/heads/master@{#368536}