Cleanup swapped out RenderViewHosts when the process goes away.

Cleanup swapped out RenderViewHosts when the process goes away.

Currently swapped out RVHs are not cleaned up for a given WebContents if the renderer hosting the associated RVs crashes. During webview postMessage, when sending a message from the embedder to the guest after recovering from a crash, we use the routing ID of the existing swapped out RVH which does not have a corresponding RV in the new renderer process. This produces a null e.source contentWindow and so the new guest has no way to communicate back to the embedder.

BUG=258993
Test=RenderViewHostManagerTest.CleanUpSwappedOutRVHOnProcessCrash

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

[Charlie Reis, 2013-07-10 17:23:30]

Please wait for Nasko's review as well, but drive-by LGTM with nits.  :)

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager_unittest.cc (right):

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager_unittest.cc:949: ////
Navigate to an initial URL.
nit: Only two //

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager_unittest.cc:969:
base::TERMINATION_STATUS_NORMAL_TERMINATION,
Should this be TERMINATION_STATUS_PROCESS_CRASHED?

[nasko, 2013-07-10 17:29:00]

LGTM with a couple of nits.

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:90: // Keep track of
renderer processes as they start to shut down.
nit: Update the comment to include that we track crashed/killed processes, not
just the ones that start to shut down.

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager_unittest.cc (right):

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager_unittest.cc:945: // those
associated RenderViews crashes.
nit: Add a link to the bug here.

[Fady Samuel, 2013-07-10 17:51:33]

Nits addressed. CQ'ing.

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager.cc (right):

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:90: // Keep track of
renderer processes as they start to shut down.
On 2013/07/10 17:29:00, nasko wrote:
> nit: Update the comment to include that we track crashed/killed processes, not
> just the ones that start to shut down.

Done.

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager_unittest.cc (right):

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager_unittest.cc:945: // those
associated RenderViews crashes.
On 2013/07/10 17:29:00, nasko wrote:
> nit: Add a link to the bug here.

Done.

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager_unittest.cc:949: ////
Navigate to an initial URL.
On 2013/07/10 17:23:30, creis wrote:
> nit: Only two //

Done.

https://codereview.chromium.org/18512004/diff/5001/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager_unittest.cc:969:
base::TERMINATION_STATUS_NORMAL_TERMINATION,
On 2013/07/10 17:23:30, creis wrote:
> Should this be TERMINATION_STATUS_PROCESS_CRASHED?

Done.

[commit-bot: I haz the power, 2013-07-10 17:51:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/18512004/11001

[commit-bot: I haz the power, 2013-07-10 20:22:41]

Change committed as 210919

[Charlie Reis, 2013-07-10 21:10:38]

https://chromiumcodereview.appspot.com/18512004/diff/11001/content/browser/we...
File content/browser/web_contents/render_view_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/18512004/diff/11001/content/browser/we...
content/browser/web_contents/render_view_host_manager_unittest.cc:962:
opener1->CreateSwappedOutRenderView(rvh1->GetSiteInstance());
Wait, this is the same SiteInstance as opener1's current RVH.  That should be
illegal-- we should never have a swapped out RVH in the same SiteInstance as
that tab's current RVH.  That would break if we try to swap out the current one,
since they're keyed by SiteInstance ID.

I'll put together a separate CL to fix it.

[Fady Samuel, 2013-07-10 21:27:33]

https://chromiumcodereview.appspot.com/18512004/diff/11001/content/browser/we...
File content/browser/web_contents/render_view_host_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/18512004/diff/11001/content/browser/we...
content/browser/web_contents/render_view_host_manager_unittest.cc:962:
opener1->CreateSwappedOutRenderView(rvh1->GetSiteInstance());
On 2013/07/10 21:10:39, creis wrote:
> Wait, this is the same SiteInstance as opener1's current RVH.  That should be
> illegal-- we should never have a swapped out RVH in the same SiteInstance as
> that tab's current RVH.  That would break if we try to swap out the current
one,
> since they're keyed by SiteInstance ID.
> 
> I'll put together a separate CL to fix it.
> 

Heh, a bit of copy-pasta, I didn't notice. Thanks for catching this.