Add SwappedOut to TabContentsDelegate to allow us to correctly delete TabContents.

Add SwappedOut to TabContentsDelegate to allow us to correctly delete TabContents.

When a page has an unload handler and we swap it out to make way for a Prerendered tab, we expect a call to the TabContentsDelegate when it closes. For cross-site navigations, we don't get that message as the RenderViewHost is swapped instead of closing.

BUG=103966
TEST=Enable prerender from omnibox (and ensure it's not disabled by field trial) and repeatedly navigate between cnn.com and telegraph.co.uk in the Omnibox. Note in the TaskManager that you do not see zombie Tab processes.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110145

[dominich, 2011-11-11 22:15:08]

I'm open to alternative solutions to this problem. I admit I'm not completely
sure why this is different to when we swap in a prerender tab from a link other
than the timing between the running of the unload handlers and the swapping of
the RenderViewHost might be different.

To be clear - we do run the unload handlers correctly in all cases. This CL just
allows us to delete the TabContents.

[Charlie Reis, 2011-11-11 22:23:51]

Drive-by comments.  (I'm concerned about the swap out logic independently of the
zombie process you're describing.)

Is there a bug on file for this?  If not, please create one.

http://codereview.chromium.org/8539027/diff/1/content/browser/tab_contents/ta...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/8539027/diff/1/content/browser/tab_contents/ta...
content/browser/tab_contents/tab_contents.cc:1649: if (delegate() && rvh ==
render_view_host())
You're seeing the current RenderViewHost send a swapped out message?  That
sounds like a race bug that we should be handling differently.  The current RVH
should never be swapped out.

[dominich, 2011-11-11 23:09:53]

On 2011/11/11 22:23:51, creis wrote:
> Drive-by comments.  (I'm concerned about the swap out logic independently of
the
> zombie process you're describing.)
> 
> Is there a bug on file for this?  If not, please create one.
> 
>
http://codereview.chromium.org/8539027/diff/1/content/browser/tab_contents/ta...
> File content/browser/tab_contents/tab_contents.cc (right):
> 
>
http://codereview.chromium.org/8539027/diff/1/content/browser/tab_contents/ta...
> content/browser/tab_contents/tab_contents.cc:1649: if (delegate() && rvh ==
> render_view_host())
> You're seeing the current RenderViewHost send a swapped out message?  That
> sounds like a race bug that we should be handling differently.  The current
RVH
> should never be swapped out.

Yes, that's what I'm seeing. I'm suspicious of this too, but I'm not clear on
what the logic should be in this case.

The RVH is sending the SwapOut message (and getting the ACK) instead of the
Close message.

[Charlie Reis, 2011-11-11 23:45:41]

On 2011/11/11 23:09:53, dominich wrote:
> > You're seeing the current RenderViewHost send a swapped out message?  That
> > sounds like a race bug that we should be handling differently.  The current
> RVH
> > should never be swapped out.
> 
> Yes, that's what I'm seeing. I'm suspicious of this too, but I'm not clear on
> what the logic should be in this case.
> 
> The RVH is sending the SwapOut message (and getting the ACK) instead of the
> Close message.

Yes, the SwapOut message rather than the Close message is intended, because
we're not closing the tab.  We keep swapped out RenderViewHosts around in a
TabContents so that things like window.opener continue to work after going back.

Is pre-rendering swapping in an entire TabContents, and not just a
RenderViewHost?  Ugh.  That changes a lot of expectations, like whether a tab
can have a current but swapped out RVH.  I suppose it's not a race, then, and
yes, you'll need to delete it.

[dominich, 2011-11-11 23:48:55]

On 2011/11/11 23:45:41, creis wrote:
> On 2011/11/11 23:09:53, dominich wrote:
> > > You're seeing the current RenderViewHost send a swapped out message?  That
> > > sounds like a race bug that we should be handling differently.  The
current
> > RVH
> > > should never be swapped out.
> > 
> > Yes, that's what I'm seeing. I'm suspicious of this too, but I'm not clear
on
> > what the logic should be in this case.
> > 
> > The RVH is sending the SwapOut message (and getting the ACK) instead of the
> > Close message.
> 
> Yes, the SwapOut message rather than the Close message is intended, because
> we're not closing the tab.  We keep swapped out RenderViewHosts around in a
> TabContents so that things like window.opener continue to work after going
back.
> 
> Is pre-rendering swapping in an entire TabContents, and not just a
> RenderViewHost?  Ugh.  That changes a lot of expectations, like whether a tab
> can have a current but swapped out RVH.  I suppose it's not a race, then, and
> yes, you'll need to delete it.

Yes, there are many other parts of the code that are made much simpler by
swapping in an entire TabContents instead of just an RVH. This is the first
issue I've seen the other way.

So would it be more correct to close the RVH somehow instead of swapping it out?
Or is it reasonable to continue as I have in this CL?

[Charlie Reis, 2011-11-11 23:54:19]

On 2011/11/11 23:48:55, dominich wrote:
> Yes, there are many other parts of the code that are made much simpler by
> swapping in an entire TabContents instead of just an RVH. This is the first
> issue I've seen the other way.
> 
> So would it be more correct to close the RVH somehow instead of swapping it
out?
> Or is it reasonable to continue as I have in this CL?

Please continue as you were.  Switching to a Close message would be far worse--
the actual tab might close if the hang monitor timeout expires, and it would
make the swap out logic even more complex.

I think we should have a test for this, but other than that I'll give my
rubber-stamp LGTM as a drive-by reviewer.  Please still have the other reviewers
take a look.

Thanks,
Charlie

[dominich, 2011-11-11 23:57:48]

On 2011/11/11 23:54:19, creis wrote:
> On 2011/11/11 23:48:55, dominich wrote:
> > Yes, there are many other parts of the code that are made much simpler by
> > swapping in an entire TabContents instead of just an RVH. This is the first
> > issue I've seen the other way.
> > 
> > So would it be more correct to close the RVH somehow instead of swapping it
> out?
> > Or is it reasonable to continue as I have in this CL?
> 
> Please continue as you were.  Switching to a Close message would be far
worse--
> the actual tab might close if the hang monitor timeout expires, and it would
> make the swap out logic even more complex.

The one thing that I do need to check is if this correctly destroys the RVH and
Render processes when the TabContents is deleted. If not, we still have a leak
and it's going to be harder to detect.

> 
> I think we should have a test for this, but other than that I'll give my
> rubber-stamp LGTM as a drive-by reviewer.  Please still have the other
reviewers
> take a look.

Thanks - I'll think about how to write a test for it.

> 
> Thanks,
> Charlie

[jam, 2011-11-14 19:29:55]

lgtm

[cbentzel, 2011-11-15 12:20:34]

Thanks for tracking this down.

http://codereview.chromium.org/8539027/diff/4001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8539027/diff/4001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:94:
MessageLoop::current()->PostDelayedTask(FROM_HERE,
Why is this needed? Do the other mechanisms not guarantee that CloseContents or
SwappedOut are called at the correct time?

http://codereview.chromium.org/8539027/diff/4001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:95:
base::Bind(&OnCloseTabContentsDeleter::ScheduleTabContentsForDeletion,
There's already some hang timers in RenderViewHost/RenderWidgetHost which will
eventually call TabContentsDelegate::CloseContents. Why is this timeout needed
as well?

[dominich, 2011-11-15 15:33:28]

http://codereview.chromium.org/8539027/diff/4001/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_manager.cc (right):

http://codereview.chromium.org/8539027/diff/4001/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:95:
base::Bind(&OnCloseTabContentsDeleter::ScheduleTabContentsForDeletion,
On 2011/11/15 12:20:34, cbentzel wrote:
> There's already some hang timers in RenderViewHost/RenderWidgetHost which will
> eventually call TabContentsDelegate::CloseContents. Why is this timeout needed
> as well? 

The hang timers didn't fire in this case as the RVH wasn't aware that the
TabContents should have closed. This timeout will catch any other edge cases
that we might miss, and the UMA stats should let us know if there's a
regression.

[cbentzel, 2011-11-15 15:50:53]

LGTM

On Tue, Nov 15, 2011 at 10:33 AM, <dominich@chromium.org> wrote:

>
> http://codereview.chromium.**org/8539027/diff/4001/chrome/**
>
browser/prerender/prerender_**manager.cc<http://codereview.chromium.org/8539027/diff/4001/chrome/browser/prerender/prerender_manager.cc>
> File chrome/browser/prerender/**prerender_manager.cc (right):
>
> http://codereview.chromium.**org/8539027/diff/4001/chrome/**
>
browser/prerender/prerender_**manager.cc#newcode95<http://codereview.chromium.org/8539027/diff/4001/chrome/browser/prerender/prerender_manager.cc#newcode95>
> chrome/browser/prerender/**prerender_manager.cc:95:
> base::Bind(&**OnCloseTabContentsDeleter::**ScheduleTabContentsForDeletion*
> *,
> On 2011/11/15 12:20:34, cbentzel wrote:
>
>> There's already some hang timers in RenderViewHost/**RenderWidgetHost
>>
> which will
>
>> eventually call TabContentsDelegate::**CloseContents. Why is this
>>
> timeout needed
>
>> as well?
>>
>
> The hang timers didn't fire in this case as the RVH wasn't aware that
> the TabContents should have closed. This timeout will catch any other
> edge cases that we might miss, and the UMA stats should let us know if
> there's a regression.
>
>
http://codereview.chromium.**org/8539027/<http://codereview.chromium.org/8539...
>

[mmenke, 2011-11-15 17:26:51]

LGTM.  Sorry for the delayed response, hadn't realized I was a reviewer on this.

[commit-bot: I haz the power, 2011-11-15 17:32:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominich@chromium.org/8539027/4001

[commit-bot: I haz the power, 2011-11-15 19:23:00]

Change committed as 110145