Eliminate a potential race in IPC::ChannelProxy

Eliminate a potential race in IPC::ChannelProxy

Doing the following steps with ChannelProxy leads to a data race:
1) Create the ChannelProxy, but don't initialize it.
2) Add a filter.
3) Init the ChannelProxy.

The problem is, AddFilter() posts a task from the Listener thread to the IPC task runner to do OnAddFilter. Prior to this patch, OnAddFilter will try to read channel_ even though channel_ may not have been initialized, and it's accessed without any synchronization.

This patch only really adds the filter if peer_pid_ has been set on the IPC::Channel thread; otherwise, it waits until the connection has been established to really add filters.

See the bug for more detail.

BUG=244383
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256188

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258406

[dmichael (off chromium), 2014-02-27 20:19:50]

jam: Please punt to the right person if it's not you.

See this comment on the bug for an explanation of the problem:
https://code.google.com/p/chromium/issues/detail?id=244383#c9

I'm happy to clean up this CL & add some documentation, but before working too
hard on it, I wanted to make sure this isn't somehow intended.

[dmichael (off chromium), 2014-02-27 20:42:31]

(Note, RemoveFilter is still broken in this patch; I can work on trying to fix
that if you agree the usage listed in the bug is something we want to support)

[jam, 2014-02-28 00:41:41]

lgtm with nit

please add more information to the cl description describing the bug

https://codereview.chromium.org/183553004/diff/1/ipc/ipc_channel_proxy.cc
File ipc/ipc_channel_proxy.cc (right):

https://codereview.chromium.org/183553004/diff/1/ipc/ipc_channel_proxy.cc#new...
ipc/ipc_channel_proxy.cc:208: peer_pid_ = channel_->peer_pid();
nit: if you move these two lines to the two of the method, then you can avoid
adding the new member variable and instead use peer_pid_ instead as the signal

[dmichael (off chromium), 2014-02-28 23:25:46]

PTAL...  I had to make a few other changes, and made some minor style changes.

https://codereview.chromium.org/183553004/diff/80001/ipc/ipc_channel_proxy.cc
File ipc/ipc_channel_proxy.cc (right):

https://codereview.chromium.org/183553004/diff/80001/ipc/ipc_channel_proxy.cc...
ipc/ipc_channel_proxy.cc:270: return;
There's no need to call OnChannelClosed; it's already closed, since that's the
only place that we reset channel_. (It also bails immediately if channel_ is
NULL). So I thought this was a little easier to understand.

[jam, 2014-03-03 16:27:03]

https://codereview.chromium.org/183553004/diff/80001/ipc/ipc_channel_proxy.cc
File ipc/ipc_channel_proxy.cc (right):

https://codereview.chromium.org/183553004/diff/80001/ipc/ipc_channel_proxy.cc...
ipc/ipc_channel_proxy.cc:291: return;  // The channel has been closed, so don't
really add the filters.
can this condition really get hit since we're only getting here through
OnChannelConnected now?

[dmichael (off chromium), 2014-03-03 16:37:02]

It looks like I broke most/all the PPAPINaCl browser tests on the trybots with
this change, so I need to figure that out before I publish again...

https://codereview.chromium.org/183553004/diff/80001/ipc/ipc_channel_proxy.cc
File ipc/ipc_channel_proxy.cc (right):

https://codereview.chromium.org/183553004/diff/80001/ipc/ipc_channel_proxy.cc...
ipc/ipc_channel_proxy.cc:291: return;  // The channel has been closed, so don't
really add the filters.
On 2014/03/03 16:27:03, jam wrote:
> can this condition really get hit since we're only getting here through
> OnChannelConnected now?
Good point, looks like it can't happen now.

I'll remove that.

[dmichael (off chromium), 2014-03-07 16:19:03]

Okay, sorry for the delay. This change uncovered an old bug in
ipc_channel_nacl.cc that took a while to pin down. It should be ready now. PTAL

[jam, 2014-03-10 20:32:37]

lgtm

[dmichael (off chromium), 2014-03-10 20:35:00]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-03-10 20:44:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/183553004/120001

[commit-bot: I haz the power, 2014-03-10 20:58:13]

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

[commit-bot: I haz the power, 2014-03-10 20:58:14]

Try jobs failed on following builders: mac_chromium_compile_dbg

[dmichael (off chromium), 2014-03-10 21:20:19]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-03-10 21:50:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/183553004/140001

[commit-bot: I haz the power, 2014-03-10 22:36:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/183553004/140001

[commit-bot: I haz the power, 2014-03-10 23:49:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/183553004/140001

[commit-bot: I haz the power, 2014-03-11 10:59:09]

Change committed as 256188

[johnme, 2014-03-11 13:53:41]

A revert of this CL has been created in
https://codereview.chromium.org/194923004/ by johnme@chromium.org.

The reason for reverting is: Since this has landed, testsuite
content_browsertests is failing on bot Android Tests (dbg) on the chromium.linux
waterfall.

Specifically, the WebContentsImplBrowserTest.OpenURLSubframe test is
consistently crashing, with the following DCHECK:

[FATAL:device_orientation_message_filter.cc(18)] Check failed:
BrowserThread::CurrentlyOn(BrowserThread::IO).

This corresponds the the following DCHECK about being on the IO thread:

DeviceOrientationMessageFilter::~DeviceOrientationMessageFilter() {
  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
  if (is_started_)
    DeviceInertialSensorService::GetInstance()->RemoveConsumer(
        CONSUMER_TYPE_ORIENTATION);
}

This same DCHECK failed in one of the try jobs on this CL:
http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...

Since this DCHECK has never been observed to fail before (certainly not in the
last 200 builds), and has failed both in this CL's try job and twice in a row on
the waterfall since landing this CL, it seems very likely that this CL is the
cause.

At a guess, the changes to ChannelProxy::Context::OnRemoveFilter seem quite
relevant here..

[dmichael (off chromium), 2014-03-19 19:15:01]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2014-03-19 19:16:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/183553004/170001

[dmichael (off chromium), 2014-03-19 22:36:48]

The CQ bit was unchecked by dmichael@chromium.org

[dmichael (off chromium), 2014-03-19 22:36:49]

The CQ bit was checked by dmichael@chromium.org

[dmichael (off chromium), 2014-03-20 16:16:46]

The CQ bit was unchecked by dmichael@chromium.org

[dmichael (off chromium), 2014-03-20 16:17:01]

The CQ bit was checked by dmichael@chromium.org

[dmichael (off chromium), 2014-03-20 17:34:16]

The CQ bit was checked by dmichael@chromium.org

[Sergey Berezin, 2014-03-20 17:59:51]

The CQ bit was unchecked by sergeyberezin@chromium.org

[Sergey Berezin, 2014-03-20 18:01:11]

The CQ bit was checked by sergeyberezin@chromium.org

[Sergey Berezin, 2014-03-20 18:03:11]

On 2014/03/20 17:59:51, Sergey Berezin wrote:
> The CQ bit was unchecked by mailto:sergeyberezin@chromium.org

CQ was processing the wrong patchset. I re-checked the commit box, it should be
more sane now.

[commit-bot: I haz the power, 2014-03-20 18:03:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/183553004/190001

[commit-bot: I haz the power, 2014-03-20 21:00:54]

Change committed as 258406