IPC: Make ChannelReader inherit from SupportsAttachmentBrokering.

IPC: Make ChannelReader inherit from SupportsAttachmentBrokering.

Each IPC::Channel will have a reference to an AttachmentBroker. This is
accomplished by requiring each of the Channel::Create methods to have a
|broker| parameter. For now, the |broker| parameter has a default value of
nullptr. This default parameter only exists so that this CL and subsequent
refactors can be decomposed into smaller CLs. The default parameter will be
removed once all callers of Channel::Create have been updated to pass in an
appropriate broker.

BUG=493414
Committed: https://crrev.com/27aa7d8b9e17ed5dd8f6b247c87ae7988f61d793
Cr-Commit-Position: refs/heads/master@{#334699}

[erikchen, 2015-06-15 22:18:04]

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

[commit-bot: I haz the power, 2015-06-15 22:18:55]

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

[commit-bot: I haz the power, 2015-06-15 22:36:11]

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

[commit-bot: I haz the power, 2015-06-15 22:36:13]

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

[erikchen, 2015-06-15 23:09:09]

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

[commit-bot: I haz the power, 2015-06-15 23:11:58]

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

[commit-bot: I haz the power, 2015-06-15 23:27:32]

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

[commit-bot: I haz the power, 2015-06-15 23:27:32]

Dry run: 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, 2015-06-16 00:11:45]

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

[commit-bot: I haz the power, 2015-06-16 00:12:26]

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

[erikchen, 2015-06-16 01:18:38]

erikchen@chromium.org changed reviewers:
+ wfh@chromium.org

[erikchen, 2015-06-16 01:18:39]

wfh: Please review.

Note that this CL depends on https://codereview.chromium.org/1180313007/.
Patch set 4 is functionally identical to patch set 6, and passes all try bots.
Let me know if you have any questions.

[Will Harris, 2015-06-16 12:57:49]

wfh@chromium.org changed reviewers:
+ tsepez@chromium.org

[Will Harris, 2015-06-16 12:57:59]

+tsepez for review

[Will Harris, 2015-06-16 12:59:40]

This CL looks really complex. I will try and take a look but I am very
unfamiliar with this codebase.

Are you planning on refactoring the existing attachment system (for posix files
and mojo handles) to use attachment broker?

[Will Harris, 2015-06-16 13:06:04]

https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_nacl.h
File ipc/ipc_channel_nacl.h (right):

https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_nacl.h...
ipc/ipc_channel_nacl.h:120: AttachmentBroker* broker_;
where will this object be created and how will the lifetime be managed, all the
code in this CL passes nullptr so hard to determine.

[Tom Sepez, 2015-06-16 16:36:45]

Didn't see a class definition for SupportsAttachmentBrokerinng in the CL, does
the class that inherits from it compile properly?

https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_nacl.h
File ipc/ipc_channel_nacl.h (right):

https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_nacl.h...
ipc/ipc_channel_nacl.h:119: // |broker_| must outlive this instance.
nit: maybe move this comment to the ctor, where the caller will actually pass in
the broker.

https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_posix.h
File ipc/ipc_channel_posix.h (right):

https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_posix....
ipc/ipc_channel_posix.h:181: // |broker_| must outlive this instance.
nit: ditto.

[erikchen, 2015-06-16 17:18:35]

On 2015/06/16 16:36:45, Tom Sepez wrote:
> Didn't see a class definition for SupportsAttachmentBrokerinng in the CL, does
> the class that inherits from it compile properly?
> 
> https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_nacl.h
> File ipc/ipc_channel_nacl.h (right):
> 
>
https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_nacl.h...
> ipc/ipc_channel_nacl.h:119: // |broker_| must outlive this instance.
> nit: maybe move this comment to the ctor, where the caller will actually pass
in
> the broker.
> 
> https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_posix.h
> File ipc/ipc_channel_posix.h (right):
> 
>
https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_posix....
> ipc/ipc_channel_posix.h:181: // |broker_| must outlive this instance.
> nit: ditto.

I have multiple CLs out for review. This CL depends on
https://codereview.chromium.org/1180313007/ (this information is also available
earlier in the comment chain, #12).
Note that Patch set 4 is functionally identical to patch set 6, and passes all
try bots.

[erikchen, 2015-06-16 17:36:48]

tsepez: PTAL

https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_nacl.h
File ipc/ipc_channel_nacl.h (right):

https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_nacl.h...
ipc/ipc_channel_nacl.h:119: // |broker_| must outlive this instance.
On 2015/06/16 16:36:44, Tom Sepez wrote:
> nit: maybe move this comment to the ctor, where the caller will actually pass
in
> the broker.

I kept the comment here, and added a similar comment to the constructor.

https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_nacl.h...
ipc/ipc_channel_nacl.h:120: AttachmentBroker* broker_;
On 2015/06/16 13:06:04, Will Harris wrote:
> where will this object be created and how will the lifetime be managed, all
the
> code in this CL passes nullptr so hard to determine.

The short answer is: Each process is responsible for ensuring the lifetime of
this object, and in general this is one of the last objects to be deleted when a
process is destroyed.

The long answer is: See this CL: https://codereview.chromium.org/1184523003/ for
more context. The CL I referenced is not going to go out for review (since it
touches ~50 files), and this CL that you're currently reviewing is a strict
subset of this CL.

https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_posix.h
File ipc/ipc_channel_posix.h (right):

https://codereview.chromium.org/1185133006/diff/100001/ipc/ipc_channel_posix....
ipc/ipc_channel_posix.h:181: // |broker_| must outlive this instance.
On 2015/06/16 16:36:44, Tom Sepez wrote:
> nit: ditto.

Done. I also went ahead and updated channel_win and channel_mojo.

[Tom Sepez, 2015-06-16 18:48:04]

Looks like I was reviewing these in the wrong order.  LGTM given the prior CL.

[erikchen, 2015-06-16 20:21:47]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-06-16 20:23:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1185133006/120001

[commit-bot: I haz the power, 2015-06-16 21:21:10]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-06-16 21:22:37]

Patchset 7 (id:??) landed as
https://crrev.com/27aa7d8b9e17ed5dd8f6b247c87ae7988f61d793
Cr-Commit-Position: refs/heads/master@{#334699}