Trivial removal of SwappedOut() did not break any tests (https://codereview.chromium.org/1896943002/). Disappointed.

Next steps:
1. See if  crbug.com/103966  reproduces with the change
2. Find out if there is a test covering the case in  crbug.com/103966  (seems not)
3. If the test can be made in a non-flaky manner, add the test
4. Ensure the test passes, send out the change for review

 Issue 103966  is not reproducible with the change: switching between omnibox prerenders for cnn.com and telegraph.co.uk does not leak processes after 2x10 iterations.

The reason why there is no leak is: ViewHostMsg_ClosePage_ACK arrives for old webcontents. From GDB:

prerender::PrerenderManager::OnCloseWebContentsDeleter::CloseContents
0x000055555ab09275 in content::WebContentsImpl::Close
0x000055555a848abe in content::RenderViewHostImpl::ClosePageIgnoringUnloadEvents
0x000055555a84c3be in content::RenderViewHostImpl::OnClosePageACK

The Close ACK is a response to Close IPC sent from the browser. The way Close sent looks counter-intuitive to me:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/frame_host/render_frame_host_manager.cc&q=render_view_host%5C(%5C)-%3EClosePage%5C(%5C)&sq=package:chromium&type=cs&l=420

The comments there suggest that this is a "Non-cross-process transition", which looks contrary to Prerender process being swapped in.

nasko: Is this expected? Can we rely on having Close always being called on the page? This Close is coming from OnBeforeUnloadACK, but I am not sure how it would work on other codepaths.

mmenke: In the PrerenderBrowserTest.PrerenderUnload test the content::WebContentsImpl::Close() is coming from WebContentsImpl::RendererUnresponsive(), which I am guessing is not covering all possible ways for the page to go away. If we are concerned about renderer leaks, how should we test all these ways for old webcontents to be replaced?

The comment you point out is actually correct in that it classifies the reasons why BeforeUnload Ack would be received. Cross-process transition is limited to single WebContents and if that is not the case, then we are closing WebContents. 

I think this is consistent with prerender, as it operates on a full WebContents, correct? If I understand prerender correctly, it creates a new WebContents and if the result is to be used, it deletes the old one (existing) and replaces it with the newly prerendered WebContents.
If that is true, then it is indeed a "tab" being closed. I think the comment could be more precise by doing s/tab/WebContents/, but at the frame_host/ layer, we don't know the difference.

I think WebContents lifetime should guarantee the lifetime of the objects it manages, which includes all the RenderFrameHosts underneath it. I don't see a reason why you can't rely on WebContents cleaning up after itself, assuming you are explicitly deleting it.

> [Prerender] creates a new WebContents and if the result is to be used, it
> deletes the old one (existing) and replaces it with the newly prerendered
> WebContents.

This is correct.

> I think WebContents lifetime should guarantee the lifetime of the objects it
> manages, which includes all the RenderFrameHosts underneath it.

Great!

My remaining worry is on level of render_frame_host_manager or above. We
subscribe to render_frame_host_->render_view_host()->ClosePage(), and the only
place I see it called is in the OnBeforeUnloadACK().

Is OnBeforeUnloadACK() always called on Prerender swap in? What happens if there
was no beforeunload handler? Does it differ in PlzNavigate and OOPIF worlds?

Seems like these cases would be better tested if the tests are closer to
RenderFrameHostManager, and not PrerenderManager. Are there such tests that
check for all ways we could replace the WebContents in a tab?

(I am utterly unfamiliar - sry)

> My remaining worry is on level of render_frame_host_manager or above. We
> subscribe to render_frame_host_->render_view_host()->ClosePage(), and the only
> place I see it called is in the OnBeforeUnloadACK().

I didn't quite follow. Can you point me to what code is "subscribing" to this?

> Is OnBeforeUnloadACK() always called on Prerender swap in? What happens if there
> was no beforeunload handler? Does it differ in PlzNavigate and OOPIF worlds?

OnBeforeUnloadACK is called when the navigation is allowed to proceed. I'm not sure how it interacts with prerender, but I'd like to think that we execute the handler before we allow the prerendered WebContents to be made active.
Currently, it behaves the same way, regardless of whether there is a handler registered or not. Ideally, we can short-circuit the IPC roundtrip, but that is just an optimization and shouldn't change how things behave outside of content/.

> Seems like these cases would be better tested if the tests are closer to
> RenderFrameHostManager, and not PrerenderManager. Are there such tests that
> check for all ways we could replace the WebContents in a tab?

Tab is a chrome/ layer concept. content/ deals with WebContents and it is a holistic unit, so no way to "replace" one WebContents with another. There is no way to test such replacement close to RenderFrameHostManager, unfortunately.

> > My remaining worry is on level of render_frame_host_manager or above. We
> > subscribe to render_frame_host_->render_view_host()->ClosePage(), and the only
> > place I see it called is in the OnBeforeUnloadACK().
>
> I didn't quite follow. Can you point me to what code is "subscribing" to this?

Here:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/prerender/prerender_manager.cc&l=142&q=PrerenderManager::OnCloseWebContentsDeleter::CloseContents&sq=package:chromium

> > Is OnBeforeUnloadACK() always called on Prerender swap in? What happens if there
> > was no beforeunload handler? Does it differ in PlzNavigate and OOPIF worlds?
>
> OnBeforeUnloadACK is called when the navigation is allowed to proceed. I'm not
> sure how it interacts with prerender, but I'd like to think that we execute
> the handler before we allow the prerendered WebContents to be made active.

Looking closer makes me happier. The way we "swap in" to replace old WebContents:
1. Subscribe to CloseContents() with our WebContentsDelegate
2. old_web_contents->DispatchBeforeUnload()
3. After 3 seconds, destroy the old_web_contents regardless of beforeunload
   dispatching
4. In CloseContents() we also destroy old_web_contents, but maybe sooner

.. modulo some deduplication of destroying and other cleanup happening around
asynchronously.

See:
  https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/prerender/prerender_manager.cc&l=527&sq=package:chromium&rcl=1461208640

Haven't decided how to test this behavior (yet), but I think it is sufficient to
guarantee no leaked WebContents.

WDYT? Maybe there are some edge cases that you can envision with this approach?

> Currently, it behaves the same way, regardless of whether there is a handler
> registered or not. Ideally, we can short-circuit the IPC roundtrip, but that
> is just an optimization and shouldn't change how things behave outside of
> content/.

Good.

> > Seems like these cases would be better tested if the tests are closer to
> > RenderFrameHostManager, and not PrerenderManager. Are there such tests that
> > check for all ways we could replace the WebContents in a tab?
>
> Tab is a chrome/ layer concept. content/ deals with WebContents and it is a
> holistic unit, so no way to "replace" one WebContents with another. There is
> no way to test such replacement close to RenderFrameHostManager,
> unfortunately.

I see. I wrote above what we do to "replace".

This seems pretty self-contained and not terribly complicated.

Seems pretty clean and definitely not needing the SwappedOut callback. It
seems redundant to CloseContents.

Yes, ensuring WebContents doesn't leak is a good strategy. It should be
easy to implement through
WebContentsObserver implementation, as it will be destroyed along with the
WebContents it
is observing.

Nothing that I can think of. Maybe ping davidben@ for more ideas for tricky
cases.

Ah, reply through email didn't preserve some of the original ... sorry about that.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9299995e124ff8fe4542790b1a9f1aed6252ec1c

commit 9299995e124ff8fe4542790b1a9f1aed6252ec1c
Author: pasko <pasko@chromium.org>
Date: Thu Apr 28 08:16:26 2016

Remove PrerenderManager::OnCloseWebContentsDeleter::SwappedOut

Formerly this event was needed to eliminate old WebContents leak on swap
( crbug.com/103966 ). Recently with heroic refactoring the
WebContentsDelegate::SwappedOut() stopped being delivered.

This makes it safe to remove OnCloseWebContentsDeleter::SwappedOut().

The old WebContents does not leak because the way it is replaced is:
1. Subscribe to CloseContents() with our WebContentsDelegate
2. old_web_contents->DispatchBeforeUnload()
3. After 3 seconds, destroy the old_web_contents regardless of beforeunload
   dispatching
4. In CloseContents() we also destroy old_web_contents, but maybe sooner

.. modulo some deduplication of destroying and other cleanup happening around
asynchronously.

BUG= 600693 

Review-Url: https://codereview.chromium.org/1896943002
Cr-Commit-Position: refs/heads/master@{#390327}

[modify] https://crrev.com/9299995e124ff8fe4542790b1a9f1aed6252ec1c/chrome/browser/prerender/prerender_browsertest.cc
[modify] https://crrev.com/9299995e124ff8fe4542790b1a9f1aed6252ec1c/chrome/browser/prerender/prerender_manager.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f4f74cd515b1f1fd16e9604d9a5cc9c4af1cf28d

commit f4f74cd515b1f1fd16e9604d9a5cc9c4af1cf28d
Author: pasko <pasko@chromium.org>
Date: Mon May 09 14:42:37 2016

Add two tests to ensure WebContents does not leak on prerender swap in

The test with beforeunload handler is as planned: the event should be dispatched
and the webcontents should be deleted. The test relies on the possibility to
send out an async XHR, even though the results of it are never seen in the
renderer.

I noticed that in PrerenderManager::SwapInternal() with any of
{unload,beforeunload} handler is set, the
old_web_contents->NeedToFireBeforeUnload() returns true. The second test is to
check the path when false is returned.

BUG= 600693 

Review-Url: https://codereview.chromium.org/1940363002
Cr-Commit-Position: refs/heads/master@{#392313}

[modify] https://crrev.com/f4f74cd515b1f1fd16e9604d9a5cc9c4af1cf28d/chrome/browser/prerender/prerender_browsertest.cc
[add] https://crrev.com/f4f74cd515b1f1fd16e9604d9a5cc9c4af1cf28d/chrome/test/data/prerender/prerender_loader_with_beforeunload.html