Fix flakiness resulting from deleting TabContents before the registrar

Fix flakiness resulting from deleting TabContents before
the registrar.  Only seems to have shown up on two download
tests, curiously.

BUG=81985
TEST=PrerenderBrowserTests.*

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

[mmenke, 2011-05-21 00:10:52]

Mind taking a quick look at this?  The problem is that the
TabContentsObserver::Registrar must be destroyed before the
TabContents it's listening to is.  This is not true of
TabContentsObservers, as they safely remove themselves from
a TabContents upon its destruction.

It's also not true of NotificationObservers, as they don't
explicitly register with the TabContents.

I didn't realize I didn't need a registrar when creating the
TabContents after the observer.  There were was a crash in
the download browser tests due to this issue -

http://build.chromium.org/p/chromium/builders/Mac%2010.6%20Tests%20%28dbg%29%...

[mmenke, 2011-05-21 00:12:39]

Err...That "NotificationObservers" should be "NotificationRegistrars"

[dominich, 2011-05-21 00:19:43]

LGTM

Assuming it passes the trybots.

[mmenke, 2011-05-21 00:21:08]

Of course.  Thanks a lot.  Worried that this might cause flake on the
other tests over the weekend, which is why I didn't run it through the
trybots first.

On 2011/05/21 00:19:43, dominic wrote:
> LGTM
> 
> Assuming it passes the trybots.

[mmenke, 2011-05-21 03:03:16]

And turns out "SetTabContents(new_contents);" doesn't do what I
thought it did...  Here's an alternative fix that has passed the
trybots.  And I submitted another CL to document TabContentsObserver
a bit better.  :)

On 2011/05/21 00:21:08, Matt Menke wrote:
> Of course.  Thanks a lot.  Worried that this might cause flake on the
> other tests over the weekend, which is why I didn't run it through the
> trybots first.
> 
> On 2011/05/21 00:19:43, dominic wrote:
> > LGTM
> > 
> > Assuming it passes the trybots.

[mmenke, 2011-05-23 16:36:22]

Mind taking a quick look at this again?

Turns out the function I thought was to switch what we're observing as a
TabContentsObserver actually was the callback, so the registrar is in fact
necessary...

[dominich, 2011-05-23 16:40:31]

http://codereview.chromium.org/7057031/diff/2002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/7057031/diff/2002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:797:
tab_contents_observer_registrar_.Observe(NULL);
Why are you only calling this in the case that we have a prerender_contents_? Is
there any reason not to always call it in the destructor?

[mmenke, 2011-05-23 16:45:35]

http://codereview.chromium.org/7057031/diff/2002/chrome/browser/prerender/pre...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/7057031/diff/2002/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_contents.cc:797:
tab_contents_observer_registrar_.Observe(NULL);
On 2011/05/23 16:40:31, dominic wrote:
> Why are you only calling this in the case that we have a prerender_contents_?
Is
> there any reason not to always call it in the destructor?

When we don't have a prerender_contents_, we aren't observing a TabContents. 
Putting it here makes this always true.  We set the observer right after
creating the prerender_contents_, and we free it right before releasing or
destroying it.

While I could call it in the destructor instead, this function is now called at
the end of the destructor, as of my download cancellation fix.  It seems to make
the most sense here.  Keep all the stop watching code in one place, and call it
when we release the prerender contents, regardless of whether we're going to
destroy it or use it.

[dominich, 2011-05-23 16:52:26]

LGTM 

On 2011/05/23 16:45:35, Matt Menke wrote:
>
http://codereview.chromium.org/7057031/diff/2002/chrome/browser/prerender/pre...
> File chrome/browser/prerender/prerender_contents.cc (right):
> 
>
http://codereview.chromium.org/7057031/diff/2002/chrome/browser/prerender/pre...
> chrome/browser/prerender/prerender_contents.cc:797:
> tab_contents_observer_registrar_.Observe(NULL);
> On 2011/05/23 16:40:31, dominic wrote:
> > Why are you only calling this in the case that we have a
prerender_contents_?
> Is
> > there any reason not to always call it in the destructor?
> 
> When we don't have a prerender_contents_, we aren't observing a TabContents. 
> Putting it here makes this always true.  We set the observer right after
> creating the prerender_contents_, and we free it right before releasing or
> destroying it.
> 
> While I could call it in the destructor instead, this function is now called
at
> the end of the destructor, as of my download cancellation fix.  It seems to
make
> the most sense here.  Keep all the stop watching code in one place, and call
it
> when we release the prerender contents, regardless of whether we're going to
> destroy it or use it.

[mmenke, 2011-05-23 16:53:23]

Thanks.

On 2011/05/23 16:52:26, dominic wrote:
> LGTM 
> 
> On 2011/05/23 16:45:35, Matt Menke wrote:
> >
>
http://codereview.chromium.org/7057031/diff/2002/chrome/browser/prerender/pre...
> > File chrome/browser/prerender/prerender_contents.cc (right):
> > 
> >
>
http://codereview.chromium.org/7057031/diff/2002/chrome/browser/prerender/pre...
> > chrome/browser/prerender/prerender_contents.cc:797:
> > tab_contents_observer_registrar_.Observe(NULL);
> > On 2011/05/23 16:40:31, dominic wrote:
> > > Why are you only calling this in the case that we have a
> prerender_contents_?
> > Is
> > > there any reason not to always call it in the destructor?
> > 
> > When we don't have a prerender_contents_, we aren't observing a TabContents.

> > Putting it here makes this always true.  We set the observer right after
> > creating the prerender_contents_, and we free it right before releasing or
> > destroying it.
> > 
> > While I could call it in the destructor instead, this function is now called
> at
> > the end of the destructor, as of my download cancellation fix.  It seems to
> make
> > the most sense here.  Keep all the stop watching code in one place, and call
> it
> > when we release the prerender contents, regardless of whether we're going to
> > destroy it or use it.

[commit-bot: I haz the power, 2011-05-23 17:58:57]

Change committed as 86301