Fix a NULL ptr dereference crash. Happens when launching an "app" with no ico...

Fix a NULL ptr dereference crash. Happens when launching an "app" with no icon.

BUG=none
TEST=manual


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

[arv (Not doing code reviews), 2010-07-27 21:52:56]

http://codereview.chromium.org/2832086/diff/1/2
File chrome/browser/views/app_launched_animation_win.cc (right):

http://codereview.chromium.org/2832086/diff/1/2#newcode124
chrome/browser/views/app_launched_animation_win.cc:124: popup_->Close();
I think there is a bug here. popup_ is only ever set in the if clause above. It
seems like we need to "delete this" in the else clause?

I'm also a bit confused whether we should call delete in AnimationEnded?

[michaeln, 2010-07-27 22:09:13]

On 2010/07/27 21:52:56, arv wrote:
> http://codereview.chromium.org/2832086/diff/1/2
> File chrome/browser/views/app_launched_animation_win.cc (right):
> 
> http://codereview.chromium.org/2832086/diff/1/2#newcode124
> chrome/browser/views/app_launched_animation_win.cc:124: popup_->Close();
> I think there is a bug here. popup_ is only ever set in the if clause above.
It
> seems like we need to "delete this" in the else clause?
> 
> I'm also a bit confused whether we should call delete in AnimationEnded?

Based on svn history I thought you wrote this code which is why i sent it to you
to look at. If there's a leak (not saying there is) maybe that could be taken
care of in a separate patch.

[michaeln, 2010-07-27 22:11:30]

I'll let you know if it's leaking in a moment.

[michaeln, 2010-07-27 22:19:09]

On 2010/07/27 22:11:30, michaeln wrote:
> I'll let you know if it's leaking in a moment.

yup... AppLaunchedAnimationWin instances are leaked

[arv (Not doing code reviews), 2010-07-27 22:43:49]

Thanks for looking into this.

I mostly copied DownloadStartedAnimationWin so I assume that one will
also leak since it is never deleted. The DownloadStartedAnimationWin
will always call popup_->Close() but I don't think that is enough any
more.

erik



On Tue, Jul 27, 2010 at 15:19,  <michaeln@chromium.org> wrote:
> On 2010/07/27 22:11:30, michaeln wrote:
>>
>> I'll let you know if it's leaking in a moment.
>
> yup... AppLaunchedAnimationWin instances are leaked
>
> http://codereview.chromium.org/2832086/show
>

[michaeln, 2010-07-27 23:38:51]

Like we chatted about, removed the else clause and filed a bug.
http://code.google.com/p/chromium/issues/detail?id=50440

[arv (Not doing code reviews), 2010-07-27 23:41:06]

LGTM