Introduce ChannelMojo

Introduce ChannelMojo

This CL introduces ChannelMojo IPC::Channel implementation
and optionally applies it for renderer-browser IPC channel.

Current stability is like 5-seconds browser and There are rough edges.
It often closes the channel so needs to be more robust.
Even though the level of stability, having it in the tree will helps
team to try and improve it.

BUG=377980
R=darin@chromium.org,jam@chromium.org,viettrungluu@chromium.org
TEST=ipc_channel_mojo_unittest.cc

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

[viettrungluu, 2014-07-15 16:28:07]

I haven't read through all of this change yet, but I want to send some initial
nits/comments. (Probably darin and/or jam should look at this carefully too.)

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:176: // Get connected ChannelHandle which the chanenl owns.
chanenl -> channel

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder.cc
File ipc/ipc_channel_builder.cc (right):

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
2014, etc. (and everywhere else)

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.cc:30: };
Probably you should have a DISALLOW_COPY_AND_ASSIGN here, for good form.

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.cc:32: }
nit: A "// namespace" comment would be nice

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.cc:37: return make_scoped_ptr(new
PlatformChannelBuilder(
nit: I think you can just do scoped_ptr<ChannelBuilder>(new ...);, which is
shorter than make_scoped_ptr + PassAs.

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder.h
File ipc/ipc_channel_builder.h (right):

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
nit: 2012 -> 2014 (and no "(c)" in the current copyright statement)

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.h:17: // passed to contstructor of ChannelProxy or
SyncChannel to tell how
contstructor -> the constructor
how they creates -> them how to create their

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.h:19: class ChannelBuilder {
This should probably be called a "factory" instead of a "builder".

https://codereview.chromium.org/382333002/diff/80001/mojo/embedder/embedder.h
File mojo/embedder/embedder.h (right):

https://codereview.chromium.org/382333002/diff/80001/mojo/embedder/embedder.h...
mojo/embedder/embedder.h:20: MOJO_SYSTEM_IMPL_EXPORT void InitIfNeeded();
I'd really rather avoid adding this. It's a symptom of sloppiness in
initialization (which Chrome suffers from in general).

[Hajime Morrita, 2014-07-15 18:46:18]

Thanks for the review Trung!
I updated the CL.

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel.h#newcod...
ipc/ipc_channel.h:176: // Get connected ChannelHandle which the chanenl owns.
On 2014/07/15 16:28:06, viettrungluu wrote:
> chanenl -> channel
Oops. Done.

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder.cc
File ipc/ipc_channel_builder.cc (right):

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2014/07/15 16:28:07, viettrungluu wrote:
> 2014, etc. (and everywhere else)

Done.

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.cc:30: };
On 2014/07/15 16:28:06, viettrungluu wrote:
> Probably you should have a DISALLOW_COPY_AND_ASSIGN here, for good form.

Done.

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.cc:32: }
On 2014/07/15 16:28:06, viettrungluu wrote:
> nit: A "// namespace" comment would be nice

Done.

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.cc:37: return make_scoped_ptr(new
PlatformChannelBuilder(
On 2014/07/15 16:28:07, viettrungluu wrote:
> nit: I think you can just do scoped_ptr<ChannelBuilder>(new ...);, which is
> shorter than make_scoped_ptr + PassAs.
Right. Done.

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder.h
File ipc/ipc_channel_builder.h (right):

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2014/07/15 16:28:07, viettrungluu wrote:
> nit: 2012 -> 2014 (and no "(c)" in the current copyright statement)

Done.

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.h:17: // passed to contstructor of ChannelProxy or
SyncChannel to tell how
On 2014/07/15 16:28:07, viettrungluu wrote:
> contstructor -> the constructor
> how they creates -> them how to create their

Done.

https://codereview.chromium.org/382333002/diff/80001/ipc/ipc_channel_builder....
ipc/ipc_channel_builder.h:19: class ChannelBuilder {
On 2014/07/15 16:28:07, viettrungluu wrote:
> This should probably be called a "factory" instead of a "builder".
The name has been taken, unfortunately.

[darin (slow to review), 2014-07-15 23:32:49]

Just some initial comments... I have much more to review...

https://codereview.chromium.org/382333002/diff/100001/ipc/ipc_channel_builder.h
File ipc/ipc_channel_builder.h (right):

https://codereview.chromium.org/382333002/diff/100001/ipc/ipc_channel_builder...
ipc/ipc_channel_builder.h:17: // passed to the contstructor of ChannelProxy or
SyncChannel to tell them
nit: contstructor -> constructor

https://codereview.chromium.org/382333002/diff/100001/ipc/ipc_channel_builder...
ipc/ipc_channel_builder.h:23: static scoped_ptr<ChannelBuilder>
CreatePlatformBuilder(
nit: Perhaps this function should just be named "Create". It'll be called as
"ChannelBuilder::Create". I'm not sure that adding the words "platform" and
"builder" help that much. "builder" is redundant, and I think "platform" just
implies that you are getting a platform-specific builder.

https://codereview.chromium.org/382333002/diff/100001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/382333002/diff/100001/ipc/ipc_channel_proxy.h...
ipc/ipc_channel_proxy.h:75: scoped_ptr<ChannelBuilder> builder,
ChannelFactory definitely feels like the better name here. It looks like the
existing ChannelFactory could be renamed. It is only used by AppShimHostManager
(apps/app_shim/app_shim_host_manager_mac.h), and I'm not sure that it is even
well named given what it does. It isn't responsible for creating channels, but
rather it responds to incoming connections. Hmm...

[Hajime Morrita, 2014-07-16 00:22:57]

Thanks you for the review!

I'll address these but before that...

>
https://codereview.chromium.org/382333002/diff/100001/ipc/ipc_channel_proxy.h...
> ipc/ipc_channel_proxy.h:75: scoped_ptr<ChannelBuilder> builder,
> ChannelFactory definitely feels like the better name here. It looks like the
> existing ChannelFactory could be renamed. It is only used by
AppShimHostManager
> (apps/app_shim/app_shim_host_manager_mac.h), and I'm not sure that it is even
> well named given what it does. It isn't responsible for creating channels, but
> rather it responds to incoming connections. Hmm...

I have a CL for this. Could you PTAL? https://codereview.chromium.org/305973002/

[Hajime Morrita, 2014-07-16 19:17:56]

I updated the CL to address some of comments.
Also I added a stability fix. Now this is a little more than a 5-second browser.

I'll rename ChannelBuilder  to ChannelFactory once
https://codereview.chromium.org/305973002/ is approved and landed.

https://codereview.chromium.org/382333002/diff/100001/ipc/ipc_channel_builder.h
File ipc/ipc_channel_builder.h (right):

https://codereview.chromium.org/382333002/diff/100001/ipc/ipc_channel_builder...
ipc/ipc_channel_builder.h:17: // passed to the contstructor of ChannelProxy or
SyncChannel to tell them
On 2014/07/15 23:32:49, darin wrote:
> nit: contstructor -> constructor
Oops:( Done.

https://codereview.chromium.org/382333002/diff/100001/ipc/ipc_channel_builder...
ipc/ipc_channel_builder.h:23: static scoped_ptr<ChannelBuilder>
CreatePlatformBuilder(
On 2014/07/15 23:32:49, darin wrote:
> nit: Perhaps this function should just be named "Create". It'll be called as
> "ChannelBuilder::Create". I'm not sure that adding the words "platform" and
> "builder" help that much. "builder" is redundant, and I think "platform" just
> implies that you are getting a platform-specific builder.
Makes sense. Renamed.

[viettrungluu, 2014-07-17 15:11:55]

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.cc:89: class HelloMessage {
Why do you need to send any hello messages, etc.?

The existing IPC code does a hello dance only exists because of Windows XP
(which doesn't provide any API to selectively pass handles to child processes)
AFAIK, but there are ways around that. jschuh@ says that he's going to work on
such a workaround in the next few weeks.

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.cc:562: // Apparently there is no way to delete
ChannelInfo and it will leak.
See DestroyChannelOnIOThread(), which for some reason is missing a comment.
Oops.

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

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:57: // Returns received handles. The lifetime
should be taken over by
If that's the case, maybe you should have a method like

void PassHandleBuffer(std::vector<MojoHandle>* handle_buffer) {
  handle_buffer->clear();
  std::swap(handle_buffer_, *handle_buffer);
}

instead? And then you can DCHECK(handle_buffer_.empty()); after you call
OnMessageArrived();.

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:64: // The the pipe and participate to
current event loop with the pipe.
I can't make sense of this comment.

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:75: // The client have to implment these
callback to get the readiness
Why not just leave these be pure virtual?

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:78: virtual void OnMessageArrived() {}
May "received" is a better name than "arrived"?

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:82: protected:
Why do you need protected members (which are frowned on/disallowed by the style
guide), given that you have accessors?

If the client methods need to mutate them (e.g., do an std::swap), then you can
provide suitable methods for that too.

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:83: std::vector<char>  data_buffer_;
nit: extra space

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:96: };
DISALLOW_COPY_AND_ASSIGN

[Hajime Morrita, 2014-07-17 18:32:13]

Thanks for the review Trung! Could you take another look?

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.cc:89: class HelloMessage {
On 2014/07/17 15:11:54, viettrungluu wrote:
> Why do you need to send any hello messages, etc.?
> 
> The existing IPC code does a hello dance only exists because of Windows XP
> (which doesn't provide any API to selectively pass handles to child processes)
> AFAIK, but there are ways around that. jschuh@ says that he's going to work on
> such a workaround in the next few weeks.

I'm using the magic header so that we can detect anyone else uses underlying
IPC::Channel accidentally. We might not need it.

We need some kind of handshake to pass another channel for sending IPC::Message
bytes though. I'd like to use the bootstrap channel as a control channel which
other Mojo-based services can use to establish their own MessagePipe. For
example, MojoApplicationHost can use that control channel to send its
bootstrapping pipe, instead using IPC::Message. Does this make sense?

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.cc:562: // Apparently there is no way to delete
ChannelInfo and it will leak.
On 2014/07/17 15:11:54, viettrungluu wrote:
> See DestroyChannelOnIOThread(), which for some reason is missing a comment.
> Oops.
Oh I see. Problem solved!

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

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:57: // Returns received handles. The lifetime
should be taken over by
On 2014/07/17 15:11:54, viettrungluu wrote:
> If that's the case, maybe you should have a method like
> 
> void PassHandleBuffer(std::vector<MojoHandle>* handle_buffer) {
>   handle_buffer->clear();
>   std::swap(handle_buffer_, *handle_buffer);
> }
> 
> instead? And then you can DCHECK(handle_buffer_.empty()); after you call
> OnMessageArrived();.

Done.

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:64: // The the pipe and participate to
current event loop with the pipe.
On 2014/07/17 15:11:54, viettrungluu wrote:
> I can't make sense of this comment.
Turned out that this could be part of the constructor.
Removed.

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:75: // The client have to implment these
callback to get the readiness
On 2014/07/17 15:11:54, viettrungluu wrote:
> Why not just leave these be pure virtual?

Done.

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:78: virtual void OnMessageArrived() {}
On 2014/07/17 15:11:54, viettrungluu wrote:
> May "received" is a better name than "arrived"?

Done.

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:82: protected:
On 2014/07/17 15:11:54, viettrungluu wrote:
> Why do you need protected members (which are frowned on/disallowed by the
style
> guide), given that you have accessors?
> 
> If the client methods need to mutate them (e.g., do an std::swap), then you
can
> provide suitable methods for that too.
My bad :-( Fixed.

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:83: std::vector<char>  data_buffer_;
On 2014/07/17 15:11:54, viettrungluu wrote:
> nit: extra space

Done.

https://codereview.chromium.org/382333002/diff/160001/ipc/mojo/ipc_message_pi...
ipc/mojo/ipc_message_pipe_reader.h:96: };
On 2014/07/17 15:11:54, viettrungluu wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Hajime Morrita, 2014-07-21 18:14:37]

PTAL? Now ChannelBuilder is renamed to ChannelFactory.

[Hajime Morrita, 2014-07-22 22:02:59]

Darin: Ping? Rename is done, build error is gone.

[darin (slow to review), 2014-07-23 23:03:33]

https://codereview.chromium.org/382333002/diff/260001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/382333002/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:568: bool
RenderProcessHostImpl::IsMojoChannelEnabled() const {
nit: these functions should be listed after MaybeActivateMojo to preserve order
of methods as they appear in the header file.

nit: perhaps this method should be renamed to ShouldUseMojoChannel? this
function returns true before the mojo channel has actually been enabled. it
seems you could also inline this into CreateChannelProxy.

https://codereview.chromium.org/382333002/diff/260001/content/child/child_thr...
File content/child/child_thread.cc (right):

https://codereview.chromium.org/382333002/diff/260001/content/child/child_thr...
content/child/child_thread.cc:233: } else {
nit: no need for "else" after "return"

https://codereview.chromium.org/382333002/diff/260001/content/child/child_thr...
File content/child/child_thread.h (right):

https://codereview.chromium.org/382333002/diff/260001/content/child/child_thr...
content/child/child_thread.h:60: struct CONTENT_EXPORT InitOptions {
nit: You could probably just call this Options, which would be similar to
base::Thread::Options.

https://codereview.chromium.org/382333002/diff/260001/content/child/child_thr...
content/child/child_thread.h:61: explicit InitOptions(std::string name = "",
bool mojo = false)
nit: Google C++ style recommends against using default arguments. it is OK to
provide overrides of the constructor however :-/

https://codereview.chromium.org/382333002/diff/260001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/382333002/diff/260001/content/renderer/render...
content/renderer/render_thread_impl.cc:294: bool IsMojoChannelEnabled() {
nit: ShouldUseMojoChannel

https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_channel.h#newco...
ipc/ipc_channel.h:176: // Get connected ChannelHandle which the channel owns.
you might mention that ownership is transferred to the caller. you might also
add a WARN_UNUSED_RESULT annotation here.

https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_export.h
File ipc/ipc_export.h (right):

https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_export.h#newcode20
ipc/ipc_export.h:20: #if defined(IPC_MOJO_IMPLEMENTATION)
why do we need a different component? can we just use the existing one?

https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_test_base.cc#ne...
ipc/ipc_test_base.cc:62: scoped_ptr<IPC::Channel> IPCTestBase::ReleaseChannel()
{
elsewhere you use "Take" (TakePipeHandle) rather than "Release" for a very
similar concept. "Release" seems a bit more canonical, so maybe you should use
that in both places.

https://codereview.chromium.org/382333002/diff/260001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/382333002/diff/260001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.cc:88: // for clarify and extensibility.
nit: clarify -> clarity

[darin (slow to review), 2014-07-23 23:04:07]

LGTM w/ nits resolved.

[Hajime Morrita, 2014-07-24 00:38:44]

Thanks for the review Darin!

https://codereview.chromium.org/382333002/diff/260001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/382333002/diff/260001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:568: bool
RenderProcessHostImpl::IsMojoChannelEnabled() const {
On 2014/07/23 23:03:32, darin wrote:
> nit: these functions should be listed after MaybeActivateMojo to preserve
order
> of methods as they appear in the header file.
> 
> nit: perhaps this method should be renamed to ShouldUseMojoChannel? this
> function returns true before the mojo channel has actually been enabled. it
> seems you could also inline this into CreateChannelProxy.

Done.

https://codereview.chromium.org/382333002/diff/260001/content/child/child_thr...
File content/child/child_thread.cc (right):

https://codereview.chromium.org/382333002/diff/260001/content/child/child_thr...
content/child/child_thread.cc:233: } else {
On 2014/07/23 23:03:33, darin wrote:
> nit: no need for "else" after "return"

Done.

https://codereview.chromium.org/382333002/diff/260001/content/child/child_thr...
File content/child/child_thread.h (right):

https://codereview.chromium.org/382333002/diff/260001/content/child/child_thr...
content/child/child_thread.h:60: struct CONTENT_EXPORT InitOptions {
On 2014/07/23 23:03:33, darin wrote:
> nit: You could probably just call this Options, which would be similar to
> base::Thread::Options.

Done.

https://codereview.chromium.org/382333002/diff/260001/content/child/child_thr...
content/child/child_thread.h:61: explicit InitOptions(std::string name = "",
bool mojo = false)
On 2014/07/23 23:03:33, darin wrote:
> nit: Google C++ style recommends against using default arguments. it is OK to
> provide overrides of the constructor however :-/
Done. I should re-read the coding guideline to unlearn Blink style :-(

https://codereview.chromium.org/382333002/diff/260001/content/renderer/render...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/382333002/diff/260001/content/renderer/render...
content/renderer/render_thread_impl.cc:294: bool IsMojoChannelEnabled() {
On 2014/07/23 23:03:33, darin wrote:
> nit: ShouldUseMojoChannel

Done.

https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_channel.h#newco...
ipc/ipc_channel.h:176: // Get connected ChannelHandle which the channel owns.
On 2014/07/23 23:03:33, darin wrote:
> you might mention that ownership is transferred to the caller. you might also
> add a WARN_UNUSED_RESULT annotation here.

Done.

https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_export.h
File ipc/ipc_export.h (right):

https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_export.h#newcode20
ipc/ipc_export.h:20: #if defined(IPC_MOJO_IMPLEMENTATION)
On 2014/07/23 23:03:33, darin wrote:
> why do we need a different component? can we just use the existing one?

I tried to make this part of IPC and let ipc depends on mojo, but a bot  blamed
that it created a circular dependency.

https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_test_base.cc
File ipc/ipc_test_base.cc (right):

https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_test_base.cc#ne...
ipc/ipc_test_base.cc:62: scoped_ptr<IPC::Channel> IPCTestBase::ReleaseChannel()
{
On 2014/07/23 23:03:33, darin wrote:
> elsewhere you use "Take" (TakePipeHandle) rather than "Release" for a very
> similar concept. "Release" seems a bit more canonical, so maybe you should use
> that in both places.

IPC::Channel has had another Take method. I'll rename it as a followup change.

https://codereview.chromium.org/382333002/diff/260001/ipc/mojo/ipc_channel_mo...
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/382333002/diff/260001/ipc/mojo/ipc_channel_mo...
ipc/mojo/ipc_channel_mojo.cc:88: // for clarify and extensibility.
On 2014/07/23 23:03:33, darin wrote:
> nit: clarify -> clarity

Done.

[Hajime Morrita, 2014-07-24 00:39:29]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-07-24 00:41:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/382333002/280001

[commit-bot: I haz the power, 2014-07-24 05:59:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-24 06:03:34]

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

[commit-bot: I haz the power, 2014-07-24 06:03:36]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[Hajime Morrita, 2014-07-24 16:51:14]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-07-24 16:52:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/382333002/300001

[darin (slow to review), 2014-07-24 17:07:19]

On 2014/07/24 00:38:44, morrita wrote:
...
> https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_export.h
> File ipc/ipc_export.h (right):
> 
>
https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_export.h#newcode20
> ipc/ipc_export.h:20: #if defined(IPC_MOJO_IMPLEMENTATION)
> On 2014/07/23 23:03:33, darin wrote:
> > why do we need a different component? can we just use the existing one?
> 
> I tried to make this part of IPC and let ipc depends on mojo, but a bot 
blamed
> that it created a circular dependency.

Hmm... that is unexpected.

> cd mojo
> grep -r ipc .
./embedder/platform_channel_utils_posix.h:// TODO(vtl): This number is taken
from ipc/file_descriptor_set_posix.h:
./public/tools/bindings/pylib/mojom/generate/module.py:# collections of
interfaces and structs to be used by mojo ipc clients and

It would be good to understand how the circular dependency is being introduced.


> 
> https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_test_base.cc
> File ipc/ipc_test_base.cc (right):
> 
>
https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_test_base.cc#ne...
> ipc/ipc_test_base.cc:62: scoped_ptr<IPC::Channel>
IPCTestBase::ReleaseChannel()
> {
> On 2014/07/23 23:03:33, darin wrote:
> > elsewhere you use "Take" (TakePipeHandle) rather than "Release" for a very
> > similar concept. "Release" seems a bit more canonical, so maybe you should
use
> > that in both places.
> 
> IPC::Channel has had another Take method. I'll rename it as a followup change.

OK. Maybe there isn't enough consistency for it to matter one way or another.

[Hajime Morrita, 2014-07-24 18:10:32]

On 2014/07/24 17:07:19, darin wrote:
> On 2014/07/24 00:38:44, morrita wrote:
> ...
> > https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_export.h
> > File ipc/ipc_export.h (right):
> > 
> >
>
https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_export.h#newcode20
> > ipc/ipc_export.h:20: #if defined(IPC_MOJO_IMPLEMENTATION)
> > On 2014/07/23 23:03:33, darin wrote:
> > > why do we need a different component? can we just use the existing one?
> > 
> > I tried to make this part of IPC and let ipc depends on mojo, but a bot 
> blamed
> > that it created a circular dependency.
> 
> Hmm... that is unexpected.
> 
> > cd mojo
> > grep -r ipc .
> ./embedder/platform_channel_utils_posix.h:// TODO(vtl): This number is taken
> from ipc/file_descriptor_set_posix.h:
> ./public/tools/bindings/pylib/mojom/generate/module.py:# collections of
> interfaces and structs to be used by mojo ipc clients and
> 
> It would be good to understand how the circular dependency is being
introduced.
> 

There are no longer the error file stored, but IIRC:

- The dependency check of GYP is done by per .gyp file level, instead of
per-target granularity.
- mojo.gyp has dependency on webkit and gpu (for HTMLViewer and View manager)
- So we get loops like ipc -> mojo -> gpu -> ipc

We could split mojo.gyp into mojo_core.gyp and mojo_services.gyp, etc.
I'm not sure what is the correct granularity here though.

[commit-bot: I haz the power, 2014-07-24 21:20:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-24 22:01:14]

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

[commit-bot: I haz the power, 2014-07-24 22:01:15]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[Ken Russell (switch to Gerrit), 2014-07-24 22:08:36]

On 2014/07/24 21:20:36, I haz the power (commit-bot) wrote:
> FYI, CQ is re-trying this CL (attempt #1).
> The failing builders are:
>   linux_chromium_chromeos_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
>   linux_chromium_rel_swarming on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
>   win_chromium_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
>   win_chromium_x64_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
>   win_gpu_triggered_tests on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

FWIW, the win_gpu_triggered_tests failures look real.

[Hajime Morrita, 2014-07-24 22:54:55]

On 2014/07/24 22:08:36, Ken Russell wrote:
> On 2014/07/24 21:20:36, I haz the power (commit-bot) wrote:
> > FYI, CQ is re-trying this CL (attempt #1).
> > The failing builders are:
> >   linux_chromium_chromeos_rel on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
> >   linux_chromium_rel_swarming on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
> >   win_chromium_rel on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
> >   win_chromium_x64_rel on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
> >   win_gpu_triggered_tests on tryserver.chromium.gpu
> >
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
> 

> FWIW, the win_gpu_triggered_tests failures look real.
Ouch. Thanks for the catch! I'll take a look.

[Hajime Morrita, 2014-07-25 00:30:33]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-07-25 00:32:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/382333002/320001

[darin (slow to review), 2014-07-25 03:43:00]

On 2014/07/24 18:10:32, morrita wrote:
> On 2014/07/24 17:07:19, darin wrote:
> > On 2014/07/24 00:38:44, morrita wrote:
> > ...
> > > https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_export.h
> > > File ipc/ipc_export.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/382333002/diff/260001/ipc/ipc_export.h#newcode20
> > > ipc/ipc_export.h:20: #if defined(IPC_MOJO_IMPLEMENTATION)
> > > On 2014/07/23 23:03:33, darin wrote:
> > > > why do we need a different component? can we just use the existing one?
> > > 
> > > I tried to make this part of IPC and let ipc depends on mojo, but a bot 
> > blamed
> > > that it created a circular dependency.
> > 
> > Hmm... that is unexpected.
> > 
> > > cd mojo
> > > grep -r ipc .
> > ./embedder/platform_channel_utils_posix.h:// TODO(vtl): This number is taken
> > from ipc/file_descriptor_set_posix.h:
> > ./public/tools/bindings/pylib/mojom/generate/module.py:# collections of
> > interfaces and structs to be used by mojo ipc clients and
> > 
> > It would be good to understand how the circular dependency is being
> introduced.
> > 
> 
> There are no longer the error file stored, but IIRC:
> 
> - The dependency check of GYP is done by per .gyp file level, instead of
> per-target granularity.
> - mojo.gyp has dependency on webkit and gpu (for HTMLViewer and View manager)
> - So we get loops like ipc -> mojo -> gpu -> ipc

oh :(


> We could split mojo.gyp into mojo_core.gyp and mojo_services.gyp, etc.
> I'm not sure what is the correct granularity here though.

We might want to have mojo.gyp for the "mojo" and "mojo_shell" targets.
I like the idea of a mojo_core.gyp and mojo_services.gyp. We could also
add a mojo_examples.gyp. This is probably worth preflighting on mojo-dev@.

I think the above is preferable to splitting up modules like ipc. As we
sprinkle Mojo usage throughout Chromium projects, we don't want to end up
having to similar splitting.

-Darin

[commit-bot: I haz the power, 2014-07-25 06:34:54]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-25 07:56:15]

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

[commit-bot: I haz the power, 2014-07-25 07:56:16]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Hajime Morrita, 2014-07-30 23:36:05]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-07-30 23:38:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/382333002/360001

[Hajime Morrita, 2014-07-31 00:45:54]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-07-31 00:46:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/382333002/380001

[commit-bot: I haz the power, 2014-07-31 07:45:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[commit-bot: I haz the power, 2014-07-31 08:01:57]

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

[commit-bot: I haz the power, 2014-07-31 08:01:58]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Hajime Morrita, 2014-07-31 20:28:48]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-07-31 20:30:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/382333002/400001

[Hajime Morrita, 2014-07-31 21:16:01]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-07-31 21:19:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/382333002/420001

[commit-bot: I haz the power, 2014-08-01 03:04:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/41246)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 03:15:16]

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

[commit-bot: I haz the power, 2014-08-01 03:15:18]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Hajime Morrita, 2014-08-02 01:01:15]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-08-02 01:03:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/382333002/440001

[Hajime Morrita, 2014-08-02 01:29:53]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-08-02 01:30:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/382333002/450049

[commit-bot: I haz the power, 2014-08-02 08:45:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-02 08:54:41]

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

[commit-bot: I haz the power, 2014-08-02 08:54:43]

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

[Hajime Morrita, 2014-08-04 18:05:57]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-08-04 18:11:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/morrita@chromium.org/382333002/470001

[commit-bot: I haz the power, 2014-08-04 23:44:23]

Change committed as 287402

[Tom Sepez, 2014-08-07 18:36:22]

https://codereview.chromium.org/382333002/diff/470001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/382333002/diff/470001/ipc/ipc_channel.h#newco...
ipc/ipc_channel.h:173: // Get its own process id. This value is told to the
peer.
Can you elaborate on "this value is told to the peer."  Can an evil renderer lie
about this?  What happens then?

[Hajime Morrita, 2014-08-07 19:58:21]

Tom, thanks for taking a look!

https://codereview.chromium.org/382333002/diff/470001/ipc/ipc_channel.h
File ipc/ipc_channel.h (right):

https://codereview.chromium.org/382333002/diff/470001/ipc/ipc_channel.h#newco...
ipc/ipc_channel.h:173: // Get its own process id. This value is told to the
peer.
On 2014/08/07 18:36:22, Tom Sepez wrote:
> Can you elaborate on "this value is told to the peer."  Can an evil renderer
lie
> about this?
Yes it can, as it can do that for current IPC implementation. (modulo secret
number check.) This just follows what existing IPC::Channel implementation does.
 
> What happens then?
I don't know much about how the peer PID is used in the codebase.
One notable use it that this value is passed to
IPC::Listener::OnChannelConnected().