IPC: Fix attachment brokering race condition.

IPC: Fix attachment brokering race condition.

A channel must be registered as a broker communication channel before it is
connected. When possible, invert the sequence of the call to connect a channel,
and the call to register the channel as a broker. In some cases, the channel
constructor and the channel initializer had to be separated, so that the
registration could happen in between.

This requirement is now enforced by a CHECK, which verifies that a channel
cannot be registered after it is connected.

BUG=598088
Committed: https://crrev.com/9097190cafdecfb1b5796b65c83af92e861ccaf8
Cr-Commit-Position: refs/heads/master@{#389618}

[erikchen, 2016-04-21 04:29:58]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 04:30:05]

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

[commit-bot: I haz the power, 2016-04-21 04:32:10]

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

[commit-bot: I haz the power, 2016-04-21 04:32:11]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2016-04-21 04:37:32]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 04:37:48]

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

[commit-bot: I haz the power, 2016-04-21 04:52:45]

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

[commit-bot: I haz the power, 2016-04-21 04:52:46]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[erikchen, 2016-04-21 05:21:06]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 05:21:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903663004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903663004/60001

[commit-bot: I haz the power, 2016-04-21 05:36:16]

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

[commit-bot: I haz the power, 2016-04-21 05:36:17]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2016-04-21 17:16:07]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 17:16:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903663004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903663004/80001

[erikchen, 2016-04-21 17:23:50]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 17:24:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903663004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903663004/100001

[erikchen, 2016-04-21 17:37:43]

Description was changed from

==========
IPC: Fix attachment brokering race condition.

A channel must be registered as a broker communication channel before it is
connected.

BUG=598088
==========

to

==========
IPC: Fix attachment brokering race condition.

A channel must be registered as a broker communication channel before it is
connected. When possible, invert the sequence of the call to connect a channel,
and the call to register the channel as a broker. In some cases, the channel
constructor and the channel initializer had to be separated, so that the
registration could happen in between.

BUG=598088
==========

[commit-bot: I haz the power, 2016-04-21 17:50:46]

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

[commit-bot: I haz the power, 2016-04-21 17:50:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-04-21 18:17:36]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 18:18:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903663004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903663004/120001

[commit-bot: I haz the power, 2016-04-21 18:48:53]

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

[commit-bot: I haz the power, 2016-04-21 18:48:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[erikchen, 2016-04-21 19:31:55]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 19:32:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903663004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903663004/140001

[erikchen, 2016-04-21 19:35:56]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 19:36:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903663004/70021
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903663004/70021

[erikchen, 2016-04-21 19:39:56]

Description was changed from

==========
IPC: Fix attachment brokering race condition.

A channel must be registered as a broker communication channel before it is
connected. When possible, invert the sequence of the call to connect a channel,
and the call to register the channel as a broker. In some cases, the channel
constructor and the channel initializer had to be separated, so that the
registration could happen in between.

BUG=598088
==========

to

==========
IPC: Fix attachment brokering race condition.

A channel must be registered as a broker communication channel before it is
connected. When possible, invert the sequence of the call to connect a channel,
and the call to register the channel as a broker. In some cases, the channel
constructor and the channel initializer had to be separated, so that the
registration could happen in between.

This requirement is now enforced by a DCHECK, which verifies that a channel
cannot be registered after it is connected.

BUG=598088
==========

[commit-bot: I haz the power, 2016-04-21 20:16:37]

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

[commit-bot: I haz the power, 2016-04-21 20:16:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-04-21 21:59:40]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 22:00:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903663004/170001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903663004/170001

[commit-bot: I haz the power, 2016-04-21 22:32:34]

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

[commit-bot: I haz the power, 2016-04-21 22:32:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[erikchen, 2016-04-21 23:19:17]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 23:19:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903663004/190001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903663004/190001

[erikchen, 2016-04-22 00:15:59]

erikchen@chromium.org changed reviewers:
+ creis@chromium.org, mseaborn@chromium.org, sergeyu@chromium.org,
tsepez@chromium.org

[erikchen, 2016-04-22 00:16:00]

mseaborn: Please review components/nacl
creis: Please review content/
tsepez: Please review ipc/
sergeyu: Please review remoting/

[Tom Sepez, 2016-04-22 00:33:22]

LGTM, nits.

https://codereview.chromium.org/1903663004/diff/190001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/1903663004/diff/190001/ipc/ipc_channel.h#newc...
ipc/ipc_channel.h:169: // The subclass implementation must call OnConnect().
nit: when? "when the connection is completed"? or what happens?

https://codereview.chromium.org/1903663004/diff/190001/ipc/ipc_channel.h#newc...
ipc/ipc_channel.h:264: // Subclasses should call this method after connecting.
should vs. must?

[commit-bot: I haz the power, 2016-04-22 00:53:29]

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

[commit-bot: I haz the power, 2016-04-22 00:53:31]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-04-22 01:42:18]

https://codereview.chromium.org/1903663004/diff/190001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/1903663004/diff/190001/ipc/ipc_channel.h#newc...
ipc/ipc_channel.h:169: // The subclass implementation must call OnConnect().
On 2016/04/22 00:33:22, Tom Sepez wrote:
> nit: when? "when the connection is completed"? or what happens?

I renamed the method to WillConnect() and updated the comment to indicate that
the method must be called at the beginning of the implementation.

This exists entirely to power the DCHECK in OnSetAttachmentBrokerEndpoint() to
prevent this error from happening again.

https://codereview.chromium.org/1903663004/diff/190001/ipc/ipc_channel.h#newc...
ipc/ipc_channel.h:264: // Subclasses should call this method after connecting.
On 2016/04/22 00:33:22, Tom Sepez wrote:
> should vs. must?

Updated the comment to use MUST.

[Sergey Ulanov, 2016-04-22 18:17:54]

lgtm

[Charlie Reis, 2016-04-22 20:00:43]

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

[Charlie Reis, 2016-04-22 20:00:46]

I can give a rubber stamp, but I'd feel better having someone more familiar with
the IPC logic review the content/ changes.  Maybe jam@?

A few naive questions below.

https://codereview.chromium.org/1903663004/diff/210001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (left):

https://codereview.chromium.org/1903663004/diff/210001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:831: return
IPC::ChannelProxy::Create(
Your CL description makes it sounds like this call isn't safe, since it calls
Init before Register.  Is there any way to make it safe in general (by passing
in what's needed to register, perhaps), rather than trying to patch up all the
call sites?

https://codereview.chromium.org/1903663004/diff/210001/ipc/ipc_channel_proxy.cc
File ipc/ipc_channel_proxy.cc (right):

https://codereview.chromium.org/1903663004/diff/210001/ipc/ipc_channel_proxy....
ipc/ipc_channel_proxy.cc:489: DCHECK(!did_init_);
Is this bad enough that we should catch it in practice with a CHECK?

[jam, 2016-04-22 22:03:33]

lgtm

[erikchen, 2016-04-25 20:10:14]

Description was changed from

==========
IPC: Fix attachment brokering race condition.

A channel must be registered as a broker communication channel before it is
connected. When possible, invert the sequence of the call to connect a channel,
and the call to register the channel as a broker. In some cases, the channel
constructor and the channel initializer had to be separated, so that the
registration could happen in between.

This requirement is now enforced by a DCHECK, which verifies that a channel
cannot be registered after it is connected.

BUG=598088
==========

to

==========
IPC: Fix attachment brokering race condition.

A channel must be registered as a broker communication channel before it is
connected. When possible, invert the sequence of the call to connect a channel,
and the call to register the channel as a broker. In some cases, the channel
constructor and the channel initializer had to be separated, so that the
registration could happen in between.

This requirement is now enforced by a CHECK, which verifies that a channel
cannot be registered after it is connected.

BUG=598088
==========

[erikchen, 2016-04-25 20:10:57]

https://codereview.chromium.org/1903663004/diff/210001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (left):

https://codereview.chromium.org/1903663004/diff/210001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:831: return
IPC::ChannelProxy::Create(
On 2016/04/22 20:00:45, Charlie Reis wrote:
> Your CL description makes it sounds like this call isn't safe, since it calls
> Init before Register.  Is there any way to make it safe in general (by passing
> in what's needed to register, perhaps), rather than trying to patch up all the
> call sites?

I considered this. The reason I chose not to do this is because attachment
brokering is only supported on Mac and Windows, and this ends up cluttering the
public interface on other OSes.

https://codereview.chromium.org/1903663004/diff/210001/ipc/ipc_channel_proxy.cc
File ipc/ipc_channel_proxy.cc (right):

https://codereview.chromium.org/1903663004/diff/210001/ipc/ipc_channel_proxy....
ipc/ipc_channel_proxy.cc:489: DCHECK(!did_init_);
On 2016/04/22 20:00:45, Charlie Reis wrote:
> Is this bad enough that we should catch it in practice with a CHECK?

Yes, done.

[erikchen, 2016-04-25 20:11:50]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-04-25 20:11:50]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
sergeyu@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1903663004/#ps230001
(title: "Comments from creis@.")

[commit-bot: I haz the power, 2016-04-25 20:12:06]

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

[Charlie Reis, 2016-04-25 20:13:35]

Ok, thanks.  LGTM as well.

[erikchen, 2016-04-25 20:14:44]

On 2016/04/25 20:12:06, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1903663004/230001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1903663004/230001

mseaborn: I'm submitting this without your review for
components/nacl/loader/nacl_listener.cc because you are OOO, this fixes a
serious bug, and change is a 1-liner that inverts the order in which two methods
are called [from the incorrect ordering to the correct ordering].

[commit-bot: I haz the power, 2016-04-25 20:33:47]

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

[commit-bot: I haz the power, 2016-04-25 20:33:49]

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

[erikchen, 2016-04-25 21:40:59]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2016-04-25 21:41:02]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
creis@chromium.org, sergeyu@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1903663004/#ps250001
(title: "Rebase (scoped_ptr->std::unique_ptr)")

[commit-bot: I haz the power, 2016-04-25 21:41:46]

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

[commit-bot: I haz the power, 2016-04-25 23:45:45]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2016-04-25 23:47:50]

Description was changed from

==========
IPC: Fix attachment brokering race condition.

A channel must be registered as a broker communication channel before it is
connected. When possible, invert the sequence of the call to connect a channel,
and the call to register the channel as a broker. In some cases, the channel
constructor and the channel initializer had to be separated, so that the
registration could happen in between.

This requirement is now enforced by a CHECK, which verifies that a channel
cannot be registered after it is connected.

BUG=598088
==========

to

==========
IPC: Fix attachment brokering race condition.

A channel must be registered as a broker communication channel before it is
connected. When possible, invert the sequence of the call to connect a channel,
and the call to register the channel as a broker. In some cases, the channel
constructor and the channel initializer had to be separated, so that the
registration could happen in between.

This requirement is now enforced by a CHECK, which verifies that a channel
cannot be registered after it is connected.

BUG=598088

Committed: https://crrev.com/9097190cafdecfb1b5796b65c83af92e861ccaf8
Cr-Commit-Position: refs/heads/master@{#389618}
==========

[commit-bot: I haz the power, 2016-04-25 23:47:51]

Patchset 14 (id:??) landed as
https://crrev.com/9097190cafdecfb1b5796b65c83af92e861ccaf8
Cr-Commit-Position: refs/heads/master@{#389618}

[Mark Seaborn, 2016-04-26 18:11:20]

On 2016/04/22 00:16:00, erikchen wrote:
> mseaborn: Please review components/nacl

LGTM for the record.