Add missing locking for the |sender_| member in IPC::SyncMessageFilter.

Add missing locking for the |sender_| member in IPC::SyncMessageFilter.

BUG=519367
Committed: https://crrev.com/1b6ce022daeba5b511782938bab5dac98ea1a541
Cr-Commit-Position: refs/heads/master@{#347455}

[Anand Mistry (off Chromium), 2015-09-03 03:25:05]

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

[Ken Rockot(use gerrit already), 2015-09-03 04:46:11]

Yikes. Good find.

LGTM but you'll need an OWNER too

[Anand Mistry (off Chromium), 2015-09-03 14:52:38]

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

[Anand Mistry (off Chromium), 2015-09-03 14:52:38]

jam@chromium.org: For OWNERS.

[jam, 2015-09-03 20:45:14]

is this only needed when is_channel_send_thread_safe_ is true?

[Anand Mistry (off Chromium), 2015-09-03 20:53:40]

On 2015/09/03 20:45:14, jam wrote:
> is this only needed when is_channel_send_thread_safe_ is true?

Yes. It should only be a problem when is_channel_send_thread_safe_ is true. But
tsan still triggers on non-threadsafe channels.

Admittedly, I think I can fix the tsan issue without fixing the locking.

[jam, 2015-09-03 23:16:37]

On 2015/09/03 20:53:40, Anand Mistry wrote:
> On 2015/09/03 20:45:14, jam wrote:
> > is this only needed when is_channel_send_thread_safe_ is true?
> 
> Yes. It should only be a problem when is_channel_send_thread_safe_ is true.

I think it's better to avoid adding locks if we don't need it.

> But
> tsan still triggers on non-threadsafe channels.
> 
> Admittedly, I think I can fix the tsan issue without fixing the locking.

that's on channel error? this is a real problem in practice i guess?

[Anand Mistry (off Chromium), 2015-09-04 01:09:03]

On 2015/09/03 23:16:37, jam wrote:
> On 2015/09/03 20:53:40, Anand Mistry wrote:
> > On 2015/09/03 20:45:14, jam wrote:
> > > is this only needed when is_channel_send_thread_safe_ is true?
> > 
> > Yes. It should only be a problem when is_channel_send_thread_safe_ is true.
> 
> I think it's better to avoid adding locks if we don't need it.

I made one change so now this CL doesn't introduce any extra locking. It just
moves it around to encompass the |sender_| member.

I think the locking can be avoided for non-threadsafe channels, but I think that
change would be more intrusive (and may be nasty). Do you want me to try and
eliminate the lock for non-threadsafe channels?

> 
> > But
> > tsan still triggers on non-threadsafe channels.
> > 
> > Admittedly, I think I can fix the tsan issue without fixing the locking.
> 
> that's on channel error? 

tsan is complaining about Send(). For non-threadsafe channels, swapping the two
variables on line 22 would shut tsan up. But the problem is still there, and
tsan would complain when using ChannelMojo. This change fixes the issue
completely.

> this is a real problem in practice i guess?

Yes, when using ChannelMojo. A thread can call Send() at the same time the IO
thread is calling OnChannelError(), causing Send() to potentially call
sender_->Send() on a nullptr.

[jam, 2015-09-04 15:45:28]

lgtm

[Anand Mistry (off Chromium), 2015-09-04 17:09:54]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-09-04 17:09:55]

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/1311043009/#ps40001
(title: "Move lock.")

[commit-bot: I haz the power, 2015-09-04 17:10:24]

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

[commit-bot: I haz the power, 2015-09-04 18:46:40]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-04 18:47:52]

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