ChannelMojo: Handle when ChannelMojo outlives ChannelMojoHost

ChannelMojo: Handle when ChannelMojo outlives ChannelMojoHost

In some case ChannelMojo outlives ChannelMojoHost because
two objects are living in diffent thread.
Instead of using lifecycle callbacks, this CL relies on
WeakPtr. See comment on ipc_channel_mojo_host.h for
more details.

This CL also fixes a crash on --single-process mode.

R=viettrungluu@chromium.org
TBR=jam@chromium.org
TEST=content_browsertests (with --enable-renderer-mojo-channel on)
BUG=377980
Committed: https://crrev.com/e9453ea69cbb3d52e67394e75c747b0b2d970621
Cr-Commit-Position: refs/heads/master@{#296871}

[viettrungluu, 2014-09-25 19:38:34]

https://codereview.chromium.org/599333002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/599333002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:2110:
base::ProcessHandle process = child_process_launcher_
When can child_process_launcher_ be null? Maybe you should just use
RenderProcessHostImpl::GetHandle()?

https://codereview.chromium.org/599333002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/599333002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.h:266:
scoped_ptr<IPC::ChannelMojoHost, IPC::ChannelMojoHostDeleter>
In general, this seems fragile/awkward to me.

It seems to me that part of the purpose of ChannelMojoHost should be to hide
details about ChannelMojo, and so ChannelMojoHost should just live on the main
thread (or any thread, really).

It suggests to me that instead of ChannelMojo talking directly to
ChannelMojoHost, it should talk to some private object owned by the
ChannelMojoHost (and whose lifecycle you can modify appropriately).

The cleanest way of doing something like this would be to have ChannelMojo have
some ChannelMojo::Delegate interface that it talks to (and which has certain
lifetime requirements), and for the ChannelMojoHost to have some private object
that implements this.

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo.cc
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo.cc...
ipc/mojo/ipc_channel_mojo.cc:86: : host_(host ? host->ToWeakPtr() :
base::WeakPtr<ChannelMojoHost>()),
Maybe it's better to just do in the body:

if (host) {
  host_ = host->ToWeakPtr();
  host_->OnChannelCreated(...);
}

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo.h
File ipc/mojo/ipc_channel_mojo.h (right):

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo.h#...
ipc/mojo/ipc_channel_mojo.h:120: void HostWasDestroyed();
I don't think this is used (or implemented).

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
File ipc/mojo/ipc_channel_mojo_host.cc (right):

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
ipc/mojo/ipc_channel_mojo_host.cc:15: void
DeleteChannelMojoHost(IPC::ChannelMojoHost* ptr) {
Can't you use DeletePointer (from bind_helpers.h) instead?

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
ipc/mojo/ipc_channel_mojo_host.cc:26: }
DCHECK_EQ(base::MessageLoop::current()->message_loop_proxy(), task_runner_)?

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
File ipc/mojo/ipc_channel_mojo_host.h (right):

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
ipc/mojo/ipc_channel_mojo_host.h:30: //   some API which touches |channel_| is
safe only in IO thread.
You should explicitly say which things are I/O-thread-only.

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
ipc/mojo/ipc_channel_mojo_host.h:36: explicit
ChannelMojoHost(scoped_refptr<base::TaskRunner> task_runner);
You should document what task runner this is (or maybe rename "task_runner" to
"io_task_runner" or something like that).

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
ipc/mojo/ipc_channel_mojo_host.h:40: scoped_refptr<base::TaskRunner>
task_runner() const { return task_runner_; }
Does this need to be public? Why?

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
ipc/mojo/ipc_channel_mojo_host.h:51: scoped_refptr<base::TaskRunner>
task_runner_;
Can this be made const?

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
ipc/mojo/ipc_channel_mojo_host.h:52: base::WeakPtr<ChannelMojo> channel_;
You should say that this should only be used on the I/O thread.

[Hajime Morrita, 2014-09-25 22:00:04]

PTAL?

https://codereview.chromium.org/599333002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/599333002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:2110:
base::ProcessHandle process = child_process_launcher_
On 2014/09/25 19:38:34, viettrungluu wrote:
> When can child_process_launcher_ be null? Maybe you should just use
> RenderProcessHostImpl::GetHandle()?

Done.

https://codereview.chromium.org/599333002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_impl.h (right):

https://codereview.chromium.org/599333002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.h:266:
scoped_ptr<IPC::ChannelMojoHost, IPC::ChannelMojoHostDeleter>
On 2014/09/25 19:38:34, viettrungluu wrote:
> In general, this seems fragile/awkward to me.
> 
> It seems to me that part of the purpose of ChannelMojoHost should be to hide
> details about ChannelMojo, and so ChannelMojoHost should just live on the main
> thread (or any thread, really).
> 
> It suggests to me that instead of ChannelMojo talking directly to
> ChannelMojoHost, it should talk to some private object owned by the
> ChannelMojoHost (and whose lifecycle you can modify appropriately).
> 
> The cleanest way of doing something like this would be to have ChannelMojo
have
> some ChannelMojo::Delegate interface that it talks to (and which has certain
> lifetime requirements), and for the ChannelMojoHost to have some private
object
> that implements this.

Thanks for the suggestion! This totally makes sense.

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo.cc
File ipc/mojo/ipc_channel_mojo.cc (right):

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo.cc...
ipc/mojo/ipc_channel_mojo.cc:86: : host_(host ? host->ToWeakPtr() :
base::WeakPtr<ChannelMojoHost>()),
On 2014/09/25 19:38:34, viettrungluu wrote:
> Maybe it's better to just do in the body:
> 
> if (host) {
>   host_ = host->ToWeakPtr();
>   host_->OnChannelCreated(...);
> }

Done.

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo.h
File ipc/mojo/ipc_channel_mojo.h (right):

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo.h#...
ipc/mojo/ipc_channel_mojo.h:120: void HostWasDestroyed();
On 2014/09/25 19:38:34, viettrungluu wrote:
> I don't think this is used (or implemented).

Oops. Removed.

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
File ipc/mojo/ipc_channel_mojo_host.cc (right):

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
ipc/mojo/ipc_channel_mojo_host.cc:15: void
DeleteChannelMojoHost(IPC::ChannelMojoHost* ptr) {
On 2014/09/25 19:38:34, viettrungluu wrote:
> Can't you use DeletePointer (from bind_helpers.h) instead?

Done.

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
ipc/mojo/ipc_channel_mojo_host.cc:26: }
On 2014/09/25 19:38:34, viettrungluu wrote:
> DCHECK_EQ(base::MessageLoop::current()->message_loop_proxy(), task_runner_)?

Done.

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
File ipc/mojo/ipc_channel_mojo_host.h (right):

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
ipc/mojo/ipc_channel_mojo_host.h:36: explicit
ChannelMojoHost(scoped_refptr<base::TaskRunner> task_runner);
On 2014/09/25 19:38:34, viettrungluu wrote:
> You should document what task runner this is (or maybe rename "task_runner" to
> "io_task_runner" or something like that).

Done.

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
ipc/mojo/ipc_channel_mojo_host.h:40: scoped_refptr<base::TaskRunner>
task_runner() const { return task_runner_; }
On 2014/09/25 19:38:34, viettrungluu wrote:
> Does this need to be public? Why?
Removed. Garbage made by try-n-error :-(

https://codereview.chromium.org/599333002/diff/1/ipc/mojo/ipc_channel_mojo_ho...
ipc/mojo/ipc_channel_mojo_host.h:51: scoped_refptr<base::TaskRunner>
task_runner_;
On 2014/09/25 19:38:34, viettrungluu wrote:
> Can this be made const?

Done.

[viettrungluu, 2014-09-25 22:09:07]

lgtm w/nits

https://codereview.chromium.org/599333002/diff/20001/ipc/mojo/ipc_channel_moj...
File ipc/mojo/ipc_channel_mojo_host.cc (right):

https://codereview.chromium.org/599333002/diff/20001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_host.cc:13: // The delete class lives in IO thread to
talk to ChannelMojo on
"delete" -> "delegate"
"in" -> "on the"

https://codereview.chromium.org/599333002/diff/20001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_host.cc:16: // The object must be touched only in IO
thread.
"in" -> "on the"

[Hajime Morrita, 2014-09-25 22:15:12]

Thanks! Landing...

https://codereview.chromium.org/599333002/diff/20001/ipc/mojo/ipc_channel_moj...
File ipc/mojo/ipc_channel_mojo_host.cc (right):

https://codereview.chromium.org/599333002/diff/20001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_host.cc:13: // The delete class lives in IO thread to
talk to ChannelMojo on
On 2014/09/25 22:09:07, viettrungluu wrote:
> "delete" -> "delegate"
> "in" -> "on the"

Done.

https://codereview.chromium.org/599333002/diff/20001/ipc/mojo/ipc_channel_moj...
ipc/mojo/ipc_channel_mojo_host.cc:16: // The object must be touched only in IO
thread.
On 2014/09/25 22:09:07, viettrungluu wrote:
> "in" -> "on the"

Done.

[Hajime Morrita, 2014-09-25 22:15:51]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-09-25 22:16:35]

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

[Hajime Morrita, 2014-09-25 23:03:04]

The CQ bit was unchecked by morrita@chromium.org

[Hajime Morrita, 2014-09-25 23:56:16]

The CQ bit was checked by morrita@chromium.org

[commit-bot: I haz the power, 2014-09-25 23:56:45]

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

[commit-bot: I haz the power, 2014-09-26 03:20:55]

Committed patchset #4 (id:60001) as 3b37273710c24f1c829c7bb273d9657f541cd918

[commit-bot: I haz the power, 2014-09-26 03:21:29]

Patchset 4 (id:??) landed as
https://crrev.com/e9453ea69cbb3d52e67394e75c747b0b2d970621
Cr-Commit-Position: refs/heads/master@{#296871}