Always keep a ChannelProxy alive in RenderProcessHostImpl

Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces at the moment.

This CL changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

ProcessDied() re-initializes the ChannelProxy to prepare
for potential RPHI reuse, rather than simply resetting it.

Additionally this removes the EnableSendQueue() call on
RPH, as it is either redundant or incorrect - it merely
set |is_initialized_| to false. This could only be reset to
true by Init(), and only when |channel_| was null. The
only possible net effect of this would have been to pause
the channel indefinitely (which is wrong and unlikely), or
to pause the channel during initialization (which is
redundant because it's already effectively paused until
Init()).

BUG=612500
Committed: https://crrev.com/cd2de7ccd49ab1fa092526488761f0c855688654
Cr-Commit-Position: refs/heads/master@{#425358}

[Ken Rockot(use gerrit already), 2016-10-11 23:24:16]

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

[commit-bot: I haz the power, 2016-10-11 23:25:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-10-11 23:29:22]

Description was changed from

==========
WIP: RPHI channel lifetime

BUG=
==========

to

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces.

This changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

BUG=612500
==========

[Ken Rockot(use gerrit already), 2016-10-11 23:35:53]

Description was changed from

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces.

This changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

BUG=612500
==========

to

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces.

This changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

Additionally this removes the EnableSendQueue() call on
RPH, as it is either redundant or incorrect - it merely
set |is_initialized_| to false. This could only be reset to
by true Init(), and only when |channel_| was null. The
only possible net effect of this would have been to pause
the channel indefinitely (which is wrong and unlikely), or
to pause the channel during initialization (which is
redundant because it's already effectively paused until
Init()).

BUG=612500
==========

[Ken Rockot(use gerrit already), 2016-10-11 23:49:20]

Description was changed from

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces.

This changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

Additionally this removes the EnableSendQueue() call on
RPH, as it is either redundant or incorrect - it merely
set |is_initialized_| to false. This could only be reset to
by true Init(), and only when |channel_| was null. The
only possible net effect of this would have been to pause
the channel indefinitely (which is wrong and unlikely), or
to pause the channel during initialization (which is
redundant because it's already effectively paused until
Init()).

BUG=612500
==========

to

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces at the moment.

This CL changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

Additionally this removes the EnableSendQueue() call on
RPH, as it is either redundant or incorrect - it merely
set |is_initialized_| to false. This could only be reset to
by true Init(), and only when |channel_| was null. The
only possible net effect of this would have been to pause
the channel indefinitely (which is wrong and unlikely), or
to pause the channel during initialization (which is
redundant because it's already effectively paused until
Init()).

BUG=612500
==========

[Ken Rockot(use gerrit already), 2016-10-11 23:50:03]

Description was changed from

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces at the moment.

This CL changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

Additionally this removes the EnableSendQueue() call on
RPH, as it is either redundant or incorrect - it merely
set |is_initialized_| to false. This could only be reset to
by true Init(), and only when |channel_| was null. The
only possible net effect of this would have been to pause
the channel indefinitely (which is wrong and unlikely), or
to pause the channel during initialization (which is
redundant because it's already effectively paused until
Init()).

BUG=612500
==========

to

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces at the moment.

This CL changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

ProcessDied() re-initializes the ChannelProxy to prepare
for potential RPHI reuse, rather than simply resetting it.

Additionally this removes the EnableSendQueue() call on
RPH, as it is either redundant or incorrect - it merely
set |is_initialized_| to false. This could only be reset to
by true Init(), and only when |channel_| was null. The
only possible net effect of this would have been to pause
the channel indefinitely (which is wrong and unlikely), or
to pause the channel during initialization (which is
redundant because it's already effectively paused until
Init()).

BUG=612500
==========

[Ken Rockot(use gerrit already), 2016-10-11 23:52:18]

rockot@chromium.org changed reviewers:
+ nick@chromium.org

[Ken Rockot(use gerrit already), 2016-10-11 23:52:18]

Please take a look. I believe I've gotten this right, but of course there might
always be some subtelty hiding somewhere.

So far my local browser tests appear to be happy, and this does indeed fix the
broken test in the message conversion CL.

[Ken Rockot(use gerrit already), 2016-10-11 23:54:08]

Description was changed from

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces at the moment.

This CL changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

ProcessDied() re-initializes the ChannelProxy to prepare
for potential RPHI reuse, rather than simply resetting it.

Additionally this removes the EnableSendQueue() call on
RPH, as it is either redundant or incorrect - it merely
set |is_initialized_| to false. This could only be reset to
by true Init(), and only when |channel_| was null. The
only possible net effect of this would have been to pause
the channel indefinitely (which is wrong and unlikely), or
to pause the channel during initialization (which is
redundant because it's already effectively paused until
Init()).

BUG=612500
==========

to

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces at the moment.

This CL changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

ProcessDied() re-initializes the ChannelProxy to prepare
for potential RPHI reuse, rather than simply resetting it.

Additionally this removes the EnableSendQueue() call on
RPH, as it is either redundant or incorrect - it merely
set |is_initialized_| to false. This can only be reset to
true by Init(), and only when |channel_| was null. The
only possible net effect of this would have been to pause
the channel indefinitely (which is wrong and unlikely), or
to pause the channel during initialization (which is
redundant because it's already effectively paused until
Init()).

BUG=612500
==========

[Ken Rockot(use gerrit already), 2016-10-11 23:54:34]

Description was changed from

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces at the moment.

This CL changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

ProcessDied() re-initializes the ChannelProxy to prepare
for potential RPHI reuse, rather than simply resetting it.

Additionally this removes the EnableSendQueue() call on
RPH, as it is either redundant or incorrect - it merely
set |is_initialized_| to false. This can only be reset to
true by Init(), and only when |channel_| was null. The
only possible net effect of this would have been to pause
the channel indefinitely (which is wrong and unlikely), or
to pause the channel during initialization (which is
redundant because it's already effectively paused until
Init()).

BUG=612500
==========

to

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces at the moment.

This CL changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

ProcessDied() re-initializes the ChannelProxy to prepare
for potential RPHI reuse, rather than simply resetting it.

Additionally this removes the EnableSendQueue() call on
RPH, as it is either redundant or incorrect - it merely
set |is_initialized_| to false. This could only be reset to
true by Init(), and only when |channel_| was null. The
only possible net effect of this would have been to pause
the channel indefinitely (which is wrong and unlikely), or
to pause the channel during initialization (which is
redundant because it's already effectively paused until
Init()).

BUG=612500
==========

[Ken Rockot(use gerrit already), 2016-10-11 23:59:02]

Patchset #1 (id:1) has been deleted

[Ken Rockot(use gerrit already), 2016-10-11 23:59:06]

Patchset #1 (id:20001) has been deleted

[Ken Rockot(use gerrit already), 2016-10-11 23:59:12]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2016-10-12 01:39:43]

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

[commit-bot: I haz the power, 2016-10-12 01:39:44]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ken Rockot(use gerrit already), 2016-10-12 19:28:42]

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

[commit-bot: I haz the power, 2016-10-12 19:29:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-12 19:34:29]

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

[commit-bot: I haz the power, 2016-10-12 19:34:30]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-10-12 23:05:15]

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

[commit-bot: I haz the power, 2016-10-12 23:05:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-10-12 23:18:06]

friendly ping!

[ncarter (slow), 2016-10-12 23:32:42]

https://codereview.chromium.org/2411093002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (left):

https://codereview.chromium.org/2411093002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:845: is_initialized_ =
false;
I'd never thought deeply about this function before, but wow, it seems super
weird that when we create a RVHImpl for an already-launched
RenderProcessHostImpl, we would pause the channel. I don't think is_initialized_
would ever transition back to true in that case in the old code (since Init()
would return true). What a mess!

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

https://codereview.chromium.org/2411093002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:975:
base::StringPrintf("%d_%d", id_, instance_id_++), child_token_, connector,
Is instance_id_ needed for correctness, if we generate a new child_token_ each
time? (I can see how a sequence number would be useful for debugging, and am
fine keeping it if that's why it's here; just curious if there's a subtle other
purpose)

https://codereview.chromium.org/2411093002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2162: if (!is_dead_) {
I'm trying to reason about why we need both is_dead_ and is_initialized_. Do we
really need both? Generally, as far as RPHImpl is concerned, everything that
happens on the initial normal launch should really also happen on re-launch
after crash.

[Ken Rockot(use gerrit already), 2016-10-12 23:56:05]

https://codereview.chromium.org/2411093002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (left):

https://codereview.chromium.org/2411093002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:845: is_initialized_ =
false;
On 2016/10/12 at 23:32:42, ncarter wrote:
> I'd never thought deeply about this function before, but wow, it seems super
weird that when we create a RVHImpl for an already-launched
RenderProcessHostImpl, we would pause the channel. I don't think is_initialized_
would ever transition back to true in that case in the old code (since Init()
would return true). What a mess!

Yes, RPHI is full of surprises :)

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

https://codereview.chromium.org/2411093002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:975:
base::StringPrintf("%d_%d", id_, instance_id_++), child_token_, connector,
On 2016/10/12 at 23:32:42, ncarter wrote:
> Is instance_id_ needed for correctness, if we generate a new child_token_ each
time? (I can see how a sequence number would be useful for debugging, and am
fine keeping it if that's why it's here; just curious if there's a subtle other
purpose)

It is needed for correctness. These two identifiers are used at different,
independent layers of the system.

The token is used to identify the child process at the internal Mojo layer,
while the instance ID is used to disambiguate the render service instance from
the ServiceManager's perspective.

https://codereview.chromium.org/2411093002/diff/60001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:2162: if (!is_dead_) {
On 2016/10/12 at 23:32:42, ncarter wrote:
> I'm trying to reason about why we need both is_dead_ and is_initialized_. Do
we really need both? Generally, as far as RPHImpl is concerned, everything that
happens on the initial normal launch should really also happen on re-launch
after crash.

We may not need both. HasConnection() could *probably* just return !is_dead_,
and otherwise is_initialized_ is only used to conditionally clear the WebRTC
logging callback (yikes).

I left it here for now because it was easier for me to believe that the new
logic is strictly equivalent to the old logic, and didn't require me to figure
out if the webrtc logging thing really mattered.

[commit-bot: I haz the power, 2016-10-13 00:43:08]

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

[commit-bot: I haz the power, 2016-10-13 00:43:09]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ken Rockot(use gerrit already), 2016-10-13 03:18:44]

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

[commit-bot: I haz the power, 2016-10-13 03:18:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

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

[commit-bot: I haz the power, 2016-10-13 04:57:06]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ken Rockot(use gerrit already), 2016-10-13 07:01:21]

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

[Ken Rockot(use gerrit already), 2016-10-13 07:01:26]

Patchset #3 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-10-13 07:01:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-10-13 07:01:46]

Patchset #2 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-10-13 08:11:56]

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

[commit-bot: I haz the power, 2016-10-13 08:11:56]

Dry run: Exceeded global retry quota

[Ken Rockot(use gerrit already), 2016-10-13 14:04:22]

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

[commit-bot: I haz the power, 2016-10-13 14:04:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-13 14:13:30]

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

[commit-bot: I haz the power, 2016-10-13 14:13:31]

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

[Ken Rockot(use gerrit already), 2016-10-13 14:17:06]

FYI and much to my bewilderment, unit_tests are consistently failing on Android
bots with this CL. Does not repro locally at all. Trying to get to the bottom of
it. :/

[Ken Rockot(use gerrit already), 2016-10-13 16:13:24]

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

[commit-bot: I haz the power, 2016-10-13 16:13:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ken Rockot(use gerrit already), 2016-10-13 16:49:48]

Patchset #4 (id:160001) has been deleted

[Ken Rockot(use gerrit already), 2016-10-13 16:49:53]

Patchset #3 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-10-13 18:25:57]

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

[commit-bot: I haz the power, 2016-10-13 18:25:58]

Dry run: Exceeded global retry quota

[Ken Rockot(use gerrit already), 2016-10-13 20:05:51]

On 2016/10/13 at 18:42:38, nick wrote:
> 

Not sure if you intended to post a comment but it's blank :P

I have figured out what's causing the timeouts after finally managing to repro
within gdb: it's a lock acquisition UAF - apparently in a unit test environment
it's not safe to assume browser_context_ is still valid during ProcessDied.

Working on a fix.

[ncarter (slow), 2016-10-13 21:40:29]

lgtm assuming you fix the android issues and back out the debug code

thanks for doing this.

https://codereview.chromium.org/2411093002/diff/120001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/2411093002/diff/120001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1983: // sync IPC. Do
not commit this!
Don't commit this!

[ncarter (slow), 2016-10-13 21:41:40]

ps3 lgtm as-is

[Ken Rockot(use gerrit already), 2016-10-13 23:25:55]

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

[commit-bot: I haz the power, 2016-10-13 23:26:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 00:20:45]

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

[commit-bot: I haz the power, 2016-10-14 00:20:46]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-10-14 16:40:09]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-10-14 16:40:10]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/2411093002/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2016-10-14 16:40:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 16:47:59]

Committed patchset #4 (id:200001)

[commit-bot: I haz the power, 2016-10-14 16:50:18]

Description was changed from

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces at the moment.

This CL changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

ProcessDied() re-initializes the ChannelProxy to prepare
for potential RPHI reuse, rather than simply resetting it.

Additionally this removes the EnableSendQueue() call on
RPH, as it is either redundant or incorrect - it merely
set |is_initialized_| to false. This could only be reset to
true by Init(), and only when |channel_| was null. The
only possible net effect of this would have been to pause
the channel indefinitely (which is wrong and unlikely), or
to pause the channel during initialization (which is
redundant because it's already effectively paused until
Init()).

BUG=612500
==========

to

==========
Always keep a ChannelProxy alive in RenderProcessHostImpl

RPHI currently keeps |channel_| around only as long as the
remote process is alive. In order to allow RPHI::Send to work
even without a process, it queues messages internally when
|channel_| is null.

Channel-associated mojom interfaces may only be acquired
once a Channel exists, and so any messages sent before
Channel creation can't be moved to Channel-associated mojom
interfaces at the moment.

This CL changes RPHI so that it always has a ChannelProxy
instance in |channel_| as long as Cleanup() hasn't run
to completion yet (at that point, the RPHI is about to be
discarded anyway.)

The pre-Init queueing behavior is satisfied here by
pausing the Channel immediately on construction and then
unpausing briefly where it was previously created.

ProcessDied() re-initializes the ChannelProxy to prepare
for potential RPHI reuse, rather than simply resetting it.

Additionally this removes the EnableSendQueue() call on
RPH, as it is either redundant or incorrect - it merely
set |is_initialized_| to false. This could only be reset to
true by Init(), and only when |channel_| was null. The
only possible net effect of this would have been to pause
the channel indefinitely (which is wrong and unlikely), or
to pause the channel during initialization (which is
redundant because it's already effectively paused until
Init()).

BUG=612500

Committed: https://crrev.com/cd2de7ccd49ab1fa092526488761f0c855688654
Cr-Commit-Position: refs/heads/master@{#425358}
==========

[commit-bot: I haz the power, 2016-10-14 16:50:19]

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