Fix issues when PrerenderContents::Destroy() is called during TabContents destruction.

Prevent DCHECK (And later double deletion) when
PrerenderContents::Destroy() is called in response to
TabContents destruction.

Re-enables PrerenderBrowserTest.PrerenderHttpAuthentication,
which ran into the issue on ChromeOS.

BUG=82835, 82913
TEST=PrerenderBrowserTest.PrerenderHttpAuthentication on ChromeOS

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

[mmenke, 2011-05-18 13:49:26]

[+dominich]

[dominich, 2011-05-18 14:18:59]

LGTM

I've added shishir to CC as he originally made the change to delete
PrerenderContents instances that are not in the prerender_list_.

[mmenke, 2011-05-18 14:20:36]

Thanks.  I've been having trouble keeping track of who did which change.

On 2011/05/18 14:18:59, dominic wrote:
> LGTM
> 
> I've added shishir to CC as he originally made the change to delete
> PrerenderContents instances that are not in the prerender_list_.

[Shishir, 2011-05-18 18:34:32]

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

http://codereview.chromium.org/7017014/diff/2012/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:589:
pending_delete_list_.push_back(entry);
I had initially pushed this outside the loop because:
1. Having it here causes leaks in prerender_manager unittest.
2. It is incompatible with the change I am making in window.opener where I need
to destroy an entry after I call GetEntry() which removes the entry from the
prerender_list. Please look at
http://codereview.chromium.org/6915019/patch/37001/38005

[mmenke, 2011-05-18 18:50:37]

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

http://codereview.chromium.org/7017014/diff/2012/chrome/browser/prerender/pre...
chrome/browser/prerender/prerender_manager.cc:589:
pending_delete_list_.push_back(entry);
On 2011/05/18 18:34:32, Shishir wrote:
> I had initially pushed this outside the loop because:
> 1. Having it here causes leaks in prerender_manager unittest.
> 2. It is incompatible with the change I am making in window.opener where I
need
> to destroy an entry after I call GetEntry() which removes the entry from the
> prerender_list. Please look at
> http://codereview.chromium.org/6915019/patch/37001/38005

Ok...  Seems like the simplest way to do this robustly, then, is to just have
the PrerenderContents itself track whether it's being destroyed, and have it
delete its TabContents (if any) explicitly in its destructor.  The latter may
not cause issues, but I prefer not to get receive any callbacks while implicitly
destroying class members.

We could stop watching everything instead, but I'd be concerned about
regressions if we did that.

[mmenke, 2011-05-18 21:02:27]

Shishir:  What do you think of this?

While I could just set prerendering_has_been_cancelled_ to true in the
destructor, think it makes sense to always use the same deletion path.  My other
CL for raceless destruction does the same thing, anyways, so figured I might as
well enforce it here.

[mmenke, 2011-05-23 15:03:33]

Shishir:  Ping?

[mmenke, 2011-05-23 15:21:48]

[+cbentzel]

[Shishir, 2011-05-23 18:00:14]

LGTM

[cbentzel, 2011-05-23 18:51:28]

LGTM

http://codereview.chromium.org/7017014/diff/22005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/7017014/diff/22005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:721:
prerender_manager_->MoveEntryToPendingDelete(this);
Is there a reason MoveEntryToPendingDelete moved up here?

[mmenke, 2011-05-23 18:55:35]

http://codereview.chromium.org/7017014/diff/22005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/7017014/diff/22005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:721:
prerender_manager_->MoveEntryToPendingDelete(this);
On 2011/05/23 18:51:28, cbentzel wrote:
> Is there a reason MoveEntryToPendingDelete moved up here?

Yes, it's just for style - prerender_has_been_cancelled_ is closely associated
with being queued for deletion, so it makes the most sense to me to set
prerender_has_been_cancelled_ at the same time we queue the deletion.  Also
makes sense to set prerender_has_been_cancelled_ to true right after checking if
it's already true.

Of course, the same argument could be made for setting final_status as well.

[commit-bot: I haz the power, 2011-05-23 19:19:39]

Try job failure for 7017014-22005 on linux:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[commit-bot: I haz the power, 2011-05-23 21:02:52]

Change committed as 86339