Fix MojoApplicationHost assertion when navigating from RenderProcessGone.

Fix MojoApplicationHost assertion when navigating from RenderProcessGone.

Termination of a renderer process results in a call to
WebContentsObserver::RenderProcessGone. Navigating from
RenderProcessGone using NavigationControllerImpl::Reload or LoadURL
results in an assertion due to MojoApplicationHost::Init() being called
again on an already initialized host. Reset |mojo_application_host_| in
RenderProcessHostImpl::ProcessDied before calling
ViewHostMsg_RenderProcessGone message handlers to avoid this problem.

BUG=448435
Committed: https://crrev.com/101f015eb605098054517fdc98de1665a9bb1431
Cr-Commit-Position: refs/heads/master@{#314435}

[Marshall, 2015-01-13 19:02:30]

marshall@chromium.org changed reviewers:
+ avi@chromium.org

[Marshall, 2015-01-13 19:03:28]

avi: Please review (or suggest a different reviewer if someone else would be
better)

[Avi (use Gerrit), 2015-01-13 19:20:33]

If you're enough of a mojo type to know this is the right thing to do, then I'm
happy as a content person to approve (LGTM). If you're not, can you double-check
with a mojo person?

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

https://codereview.chromium.org/848073002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:2052:
mojo_application_host_.reset(new MojoApplicationHost);
As a non-mojo type, a comment would be good here. Maybe something like:

// RenderProcessGone handlers might navigate or perform other actions that
require a connection;
// ensure that there is one before calling them.

or the like.

[Marshall, 2015-01-13 19:36:41]

marshall@chromium.org changed reviewers:
+ ben@chromium.org

[Marshall, 2015-01-13 19:36:42]

ben: Please review for the correctness of Mojo-related changes (or suggest an
appropriate reviewer). Thanks!

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

https://codereview.chromium.org/848073002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_impl.cc:2052:
mojo_application_host_.reset(new MojoApplicationHost);
On 2015/01/13 19:20:33, Avi wrote:
> As a non-mojo type, a comment would be good here. Maybe something like:
> 
> // RenderProcessGone handlers might navigate or perform other actions that
> require a connection;
> // ensure that there is one before calling them.
> 
> or the like.

Done.

[Ken Rockot(use gerrit already), 2015-02-03 02:25:35]

as a mojo-type, this lgtm

[Sam McNally, 2015-02-03 04:28:59]

sammc@chromium.org changed reviewers:
+ sammc@chromium.org

[Sam McNally, 2015-02-03 04:28:59]

https://codereview.chromium.org/848073002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/848073002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:2042:
RenderProcessExited(this, status, exit_code));
Could RenderProcessExited or NOTIFICATION_RENDERER_PROCESS_CLOSED observers do
anything that requires mojo_application_host_ to be valid?

[Marshall, 2015-02-03 19:48:25]

Thanks for the reviews!

https://codereview.chromium.org/848073002/diff/20001/content/browser/renderer...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/848073002/diff/20001/content/browser/renderer...
content/browser/renderer_host/render_process_host_impl.cc:2042:
RenderProcessExited(this, status, exit_code));
On 2015/02/03 04:28:59, Sam McNally wrote:
> Could RenderProcessExited or NOTIFICATION_RENDERER_PROCESS_CLOSED observers do
> anything that requires mojo_application_host_ to be valid?

A quick survey via codesearch didn't indicate any such uses to me, but I'm not
completely sure what "requires mojo_application_host_ to be valid" would look
like. Hopefully any such requirement will be caught by the CQ trybots while
testing this change.

[Sam McNally, 2015-02-03 22:34:25]

lgtm

[Marshall, 2015-02-03 22:36:49]

The CQ bit was checked by marshall@chromium.org

[commit-bot: I haz the power, 2015-02-03 22:37:23]

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

[commit-bot: I haz the power, 2015-02-03 23:38:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-03 23:40:05]

Patchset 2 (id:??) landed as
https://crrev.com/101f015eb605098054517fdc98de1665a9bb1431
Cr-Commit-Position: refs/heads/master@{#314435}

[Ken Rockot(use gerrit already), 2015-02-06 20:41:37]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/866593004/ by rockot@chromium.org.

The reason for reverting is: Causing http://crbug.com/456224.

While 448435 is valid, it was not causing crashes like this. I am actively
investigating why this change is causing crashes, but we should revert in the
meantime.
.