Delete the download shelf before we destroy the browser

Delete the download shelf before we destroy the browser in BrowserView.

BUG=84523
TEST=Sending WM_ENDSESSION to the browser while the download shelf is open doesn't crash


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

[asanka, 2011-06-01 20:43:16]

Another one :)

[Ben Goodger (Google), 2011-06-01 20:47:50]

http://codereview.chromium.org/7105026/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/7105026/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view.cc:956: download_shelf_.reset();
My question is - what other views are there that this might apply to? Are both
of these views not parent owned? Are there any others?

Who calls this?

[asanka, 2011-06-01 21:06:42]

http://codereview.chromium.org/7105026/diff/1/chrome/browser/ui/views/frame/b...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/7105026/diff/1/chrome/browser/ui/views/frame/b...
chrome/browser/ui/views/frame/browser_view.cc:956: download_shelf_.reset();
On 2011/06/01 20:47:50, Ben Goodger wrote:
> My question is - what other views are there that this might apply to? Are both
> of these views not parent owned? Are there any others?
> 
> Who calls this?

This is called by BrowserList::CloseAllBrowsers(), which can be called while
processing a WM_ENDSESSION message.

The problem here is that DownloadItemView shouldn't outlive the Browser.  On a
more general note, I think the comment in ~BrowserView also applies here:

  // Child views maintain PrefMember attributes that point to
  // OffTheRecordProfile's PrefService which gets deleted by ~Browser.
  RemoveAllChildViews(true);
  // Explicitly set browser_ to NULL.
  browser_.reset();

So perhaps we should RemoveAllChildViews() before we destroy browser_ in
DestroyBrowser().  I'll look and see what else might go wrong when we destroy
the browser_ while the BrowserView is still around.

[Ben Goodger (Google), 2011-06-01 21:21:57]

On Wed, Jun 1, 2011 at 2:06 PM, <asanka@chromium.org> wrote:

> So perhaps we should RemoveAllChildViews() before we destroy browser_ in
> DestroyBrowser().  I'll look and see what else might go wrong when we
> destroy the browser_ while the BrowserView is still around.


RemoveAllChildViews will delete those views that are parent-owned, but views
like the bookmark bar and the download shelf probably aren't. Are there
other views that aren't? You may need to reset those too.

-Ben

[asanka, 2011-06-02 18:58:18]

On 2011/06/01 21:21:57, Ben Goodger wrote:
> On Wed, Jun 1, 2011 at 2:06 PM, <mailto:asanka@chromium.org> wrote:
> 
> > So perhaps we should RemoveAllChildViews() before we destroy browser_ in
> > DestroyBrowser().  I'll look and see what else might go wrong when we
> > destroy the browser_ while the BrowserView is still around.
> 
> 
> RemoveAllChildViews will delete those views that are parent-owned, but views
> like the bookmark bar and the download shelf probably aren't. Are there
> other views that aren't? You may need to reset those too.
> 
> -Ben

These two are the only ones that aren't parent owned.  I added
RemoveAllChildViews() as well before browser_.reset().

[asanka, 2011-06-07 20:54:53]

Ping?

[Ben Goodger (Google), 2011-06-07 20:57:53]

LGTM

[commit-bot: I haz the power, 2011-06-08 14:32:46]

Try job failure for 7105026-6001 on linux_clang for step compile:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-06-08 15:28:24]

Try job failure for 7105026-6001 on linux for step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[M-A Ruel, 2011-06-08 15:36:45]

On 2011/06/08 15:28:24, commit-bot wrote:
> Try job failure for 7105026-6001 on linux for step "compile" (clobber build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

I'm adding code to retry when HEAD is broken, requeueing again.

[commit-bot: I haz the power, 2011-06-08 16:38:57]

Change committed as 88343