Fix races with MessagePipeReader due to the Mojo IPC channel being thread-safe.

Fix races with MessagePipeReader due to the Mojo IPC channel being thread-safe.

This change does three things:
1. Checks the threading of MessagePipeReader using a ThreadChecker.
2. Eliminates races on |pending_send_error_| by using atomic ops.
3. Documents and removes checks which are no longer valid because Send()
   can be called on a non-IO thread.

BUG=522888, 492867
TESTED=Enabled Mojo IPC channel and ran StatsTableBrowserTest.StartWithStatTable
       with tsan 100 times.
       Before: IPC-related races and DCHECK failures
       After: No IPC-related races and DCHECK failures.

Committed: https://crrev.com/0b0e7480d9fd3816cbee891bf1618bc02b112cb4
Cr-Commit-Position: refs/heads/master@{#346967}

[Anand Mistry (off Chromium), 2015-08-25 07:38:00]

amistry@chromium.org changed reviewers:
+ glider@chromium.org, rockot@chromium.org

[Anand Mistry (off Chromium), 2015-08-25 07:38:01]

rockot: Reviewer chosen on the ancient, scientific principal of "you touched it,
you own it".
glider: tsan. 'nuff said!

[Alexander Potapenko, 2015-08-25 11:22:34]

I believe accesses to pending_send_error_ must have acquire/release semantics
here.
If my understanding your intent correctly, this is semantically similar to
base/synchronization/cancellation_flag.h

https://codereview.chromium.org/1318453002/diff/1/ipc/mojo/ipc_message_pipe_r...
File ipc/mojo/ipc_message_pipe_reader.cc (right):

https://codereview.chromium.org/1318453002/diff/1/ipc/mojo/ipc_message_pipe_r...
ipc/mojo/ipc_message_pipe_reader.cc:51: if (pending_send_error() ==
MOJO_RESULT_OK)
I think the read of pending_send_error_ here must be an acquire load.

https://codereview.chromium.org/1318453002/diff/1/ipc/mojo/ipc_message_pipe_r...
ipc/mojo/ipc_message_pipe_reader.cc:56: CloseWithError(pending_send_error());
Why not reuse the value returned by pending_send_error() before?

[Anand Mistry (off Chromium), 2015-08-25 12:16:08]

I'll address the other comment tomorrow, but I'll address the memory semantics
now. I think this case is different from CancellationFlag. Although the comment
in cancellation_flag.h says it shouldn't be used for synchronisation, I suspect
it is in many cases, hence the need for barriers. This code is just trying to
get a value from some thread to the IO thread. It's not trying to synchronise
anything.

Also, tsan doesn't complain :P

https://codereview.chromium.org/1318453002/diff/1/ipc/mojo/ipc_message_pipe_r...
File ipc/mojo/ipc_message_pipe_reader.cc (right):

https://codereview.chromium.org/1318453002/diff/1/ipc/mojo/ipc_message_pipe_r...
ipc/mojo/ipc_message_pipe_reader.cc:51: if (pending_send_error() ==
MOJO_RESULT_OK)
On 2015/08/25 11:22:34, Alexander Potapenko wrote:
> I think the read of pending_send_error_ here must be an acquire load.

I don't think so. There are no other memory accesses that need to be ordered
between threads. |pending_send_error_| is the only variable that is shared
between threads (well, and |handle_copy_| which is const). Everything else is
only accessed on the IO thread.

[Alexander Potapenko, 2015-08-25 12:46:30]

> I don't think so. There are no other memory accesses that need to be ordered
> between threads. |pending_send_error_| is the only variable that is shared
> between threads (well, and |handle_copy_| which is const). Everything else is
> only accessed on the IO thread.

Isn't it so that assigning an error value to |pending_send_error_| indicates
that some object's state is invalid from now on?

[Anand Mistry (off Chromium), 2015-08-25 13:04:18]

On 2015/08/25 12:46:30, Alexander Potapenko wrote:
> > I don't think so. There are no other memory accesses that need to be ordered
> > between threads. |pending_send_error_| is the only variable that is shared
> > between threads (well, and |handle_copy_| which is const). Everything else
is
> > only accessed on the IO thread.
> 
> Isn't it so that assigning an error value to |pending_send_error_| indicates
> that some object's state is invalid from now on?

Yes. It indicates that the state of the Mojo handle is invalid. However, Mojo
does appropriate locking internally and is thread-safe. So that state is visible
to us without any extra synchronisation on our part.

[Anand Mistry (off Chromium), 2015-08-26 03:43:19]

https://codereview.chromium.org/1318453002/diff/1/ipc/mojo/ipc_message_pipe_r...
File ipc/mojo/ipc_message_pipe_reader.cc (right):

https://codereview.chromium.org/1318453002/diff/1/ipc/mojo/ipc_message_pipe_r...
ipc/mojo/ipc_message_pipe_reader.cc:56: CloseWithError(pending_send_error());
On 2015/08/25 11:22:34, Alexander Potapenko wrote:
> Why not reuse the value returned by pending_send_error() before?

Done.

[Ken Rockot(use gerrit already), 2015-08-26 06:13:53]

lgtm

https://codereview.chromium.org/1318453002/diff/20001/ipc/mojo/ipc_message_pi...
File ipc/mojo/ipc_message_pipe_reader.h (right):

https://codereview.chromium.org/1318453002/diff/20001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:109: // TODO(amistry): This isn't quite right
because handles can be re-used and
yeah, this is a little unfortunate, but not a practical concern at the moment.
can you please file a bug to address this and include a link to it here?

[Anand Mistry (off Chromium), 2015-08-26 07:19:17]

https://codereview.chromium.org/1318453002/diff/20001/ipc/mojo/ipc_message_pi...
File ipc/mojo/ipc_message_pipe_reader.h (right):

https://codereview.chromium.org/1318453002/diff/20001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:109: // TODO(amistry): This isn't quite right
because handles can be re-used and
On 2015/08/26 06:13:53, Ken Rockot wrote:
> yeah, this is a little unfortunate, but not a practical concern at the moment.
> can you please file a bug to address this and include a link to it here?

Done.

[Alexander Potapenko, 2015-08-26 13:56:33]

I'm convinced, LGTM :)

[Anand Mistry (off Chromium), 2015-08-26 22:02:00]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-08-26 22:02:00]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1318453002/#ps40001
(title: "Add bug reference.")

[commit-bot: I haz the power, 2015-08-26 22:03:06]

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

[commit-bot: I haz the power, 2015-08-26 22:18:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-26 22:18:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Anand Mistry (off Chromium), 2015-08-26 22:21:59]

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

[Anand Mistry (off Chromium), 2015-08-26 22:21:59]

agl@chromium.org: Please review changes because owners.

[Anand Mistry (off Chromium), 2015-08-31 21:46:50]

On 2015/08/26 22:21:59, Anand Mistry wrote:
> mailto:agl@chromium.org: Please review changes because owners.

ag: Ping!

[Anand Mistry (off Chromium), 2015-09-01 22:17:25]

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

[Anand Mistry (off Chromium), 2015-09-01 22:17:25]

-agl, +jam for IPC owners.

[jam, 2015-09-02 05:58:19]

lgtm

[Anand Mistry (off Chromium), 2015-09-02 16:51:04]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2015-09-02 16:51:50]

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

[commit-bot: I haz the power, 2015-09-02 18:04:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-02 18:05:50]

Patchset 3 (id:??) landed as
https://crrev.com/0b0e7480d9fd3816cbee891bf1618bc02b112cb4
Cr-Commit-Position: refs/heads/master@{#346967}