Attempt 2 at Fixes crash introduced @ 153047 (you can hit crash by maximizing a

Attempt 2 at Fixes crash introduced @ 153047 (you can hit crash by maximizing a
window). The cross fade code deletes the layer when the animation
finishes. The newly added code was accessing members after the
animation finished and the animator was deleted, resulting in the
crash. Since I'm sure this will come up more in the future I've
restructured the code to allow for deletion when calling out like
this.

The cross fade test exercises this code path now, but I'll see about a
more focused tests shortly.

BUG=none
TEST=covered by tests.
R=vollick@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=155533

[sky, 2012-08-25 16:01:43]

Patchset 1 is the patchset you reviewed last time, 2 the fix.

There may be more places that need to check if destroyed, but this make the bots
happy.

http://codereview.chromium.org/10869066/diff/2001/ash/wm/window_animations_un...
File ash/wm/window_animations_unittest.cc (right):

http://codereview.chromium.org/10869066/diff/2001/ash/wm/window_animations_un...
ash/wm/window_animations_unittest.cc:180:
static_cast<ui::AnimationContainerElement*>(old_layer->GetAnimator())->Step(
The old code is fine, but this better matches what normally happens when running
chrome.

[sky, 2012-08-28 17:48:12]

Ping

[Ian Vollick, 2012-08-28 17:59:00]

On 2012/08/28 17:48:12, sky wrote:
> Ping

Sorry for the slow reply. This LGTM, but I wonder if we want to check if we've
been destructed whenever we call LayerAnimationSequence::Progress with the full
duration (this seems to happen a lot).

[sky, 2012-09-04 21:00:33]

Is this what you meant?

Sorry, I had to merge so the diffs aren't right.

[sky, 2012-09-04 21:57:00]

On 2012/09/04 21:00:33, sky wrote:
> Is this what you meant?
> 
> Sorry, I had to merge so the diffs aren't right.

Hold off on this, I have to look at the failure and my windows build is
currently not working:(

[sky, 2012-09-05 23:06:51]

Ok, I think I got it this time. Take another look?

I was forced to go the refcounted separate object approach since
ProgressAnimation may in turn invoke FinishAnimation.

[Ian Vollick, 2012-09-06 00:57:35]

LGTM, but I think there are a number of other places where we run the risk of
deleting 'this' and should bail (listed below). Maybe it would make sense to
create a new bug for this and assign it to me?

http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
File ui/compositor/layer_animator.cc (right):

http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
ui/compositor/layer_animator.cc:437:
running_animations_copy[i].sequence->duration(), delegate());
Seems like there's a chance we could delete 'this' in response to this call to
Progress. Could we do the same thing here that we do in FinishAnimation?

http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
ui/compositor/layer_animator.cc:511:
running_animations_copy[i].sequence->duration(), delegate());
This seems like another place we can bail early if we've been deleted.

http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
ui/compositor/layer_animator.cc:532:
sequences[i]->Progress(sequences[i]->duration(), delegate());
Same here.

http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
ui/compositor/layer_animator.cc:544: sequence->Progress(sequence->duration(),
delegate());
Same here.

[sky, 2012-09-06 15:54:07]

If these are the only other places that need adjusting I'm happy to do
it OTOH if you think there are more and want to go through with it you
can take over the work. I'm fine iether way.

  -Scott

On Wed, Sep 5, 2012 at 5:57 PM,  <vollick@chromium.org> wrote:
> LGTM, but I think there are a number of other places where we run the risk
> of
> deleting 'this' and should bail (listed below). Maybe it would make sense to
> create a new bug for this and assign it to me?
>
>
>
http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
> File ui/compositor/layer_animator.cc (right):
>
>
http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
> ui/compositor/layer_animator.cc:437:
> running_animations_copy[i].sequence->duration(), delegate());
> Seems like there's a chance we could delete 'this' in response to this
> call to Progress. Could we do the same thing here that we do in
> FinishAnimation?
>
>
http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
> ui/compositor/layer_animator.cc:511:
> running_animations_copy[i].sequence->duration(), delegate());
> This seems like another place we can bail early if we've been deleted.
>
>
http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
> ui/compositor/layer_animator.cc:532:
> sequences[i]->Progress(sequences[i]->duration(), delegate());
> Same here.
>
>
http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
> ui/compositor/layer_animator.cc:544:
> sequence->Progress(sequence->duration(), delegate());
> Same here.
>
> http://codereview.chromium.org/10869066/

[Ian Vollick, 2012-09-07 20:29:50]

On 2012/09/06 15:54:07, sky wrote:
> If these are the only other places that need adjusting I'm happy to do
> it OTOH if you think there are more and want to go through with it you
> can take over the work. I'm fine iether way.
> 

I'm happy to do the work. It seems like it might snowball a little bit since
lots more methods will need to be prepared for 'this' getting destructed. I'm
fine either way, too. It would be nice to get this in this patch, but maybe it
would make more sense to land this now, create a bug and sort the rest out
later.

>   -Scott
> 
> On Wed, Sep 5, 2012 at 5:57 PM,  <mailto:vollick@chromium.org> wrote:
> > LGTM, but I think there are a number of other places where we run the risk
> > of
> > deleting 'this' and should bail (listed below). Maybe it would make sense to
> > create a new bug for this and assign it to me?
> >
> >
> >
>
http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
> > File ui/compositor/layer_animator.cc (right):
> >
> >
>
http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
> > ui/compositor/layer_animator.cc:437:
> > running_animations_copy[i].sequence->duration(), delegate());
> > Seems like there's a chance we could delete 'this' in response to this
> > call to Progress. Could we do the same thing here that we do in
> > FinishAnimation?
> >
> >
>
http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
> > ui/compositor/layer_animator.cc:511:
> > running_animations_copy[i].sequence->duration(), delegate());
> > This seems like another place we can bail early if we've been deleted.
> >
> >
>
http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
> > ui/compositor/layer_animator.cc:532:
> > sequences[i]->Progress(sequences[i]->duration(), delegate());
> > Same here.
> >
> >
>
http://codereview.chromium.org/10869066/diff/23001/ui/compositor/layer_animat...
> > ui/compositor/layer_animator.cc:544:
> > sequence->Progress(sequence->duration(), delegate());
> > Same here.
> >
> > http://codereview.chromium.org/10869066/