Revert 153291 - 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=129033
TEST=covered by tests.
[email protected]
Review URL: https://chromiumcodereview.appspot.com/10874064
[email protected]
Review URL: https://chromiumcodereview.appspot.com/10882043
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@153312 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/ash/wm/window_animations.cc b/ash/wm/window_animations.cc
index e0456d1..0aae796b 100644
--- a/ash/wm/window_animations.cc
+++ b/ash/wm/window_animations.cc
@@ -50,6 +50,7 @@
namespace {
const float kWindowAnimation_Vertical_TranslateY = 15.f;
+bool delayed_old_layer_deletion_in_cross_fade_for_test_ = false;
}
DEFINE_WINDOW_PROPERTY_KEY(WindowVisibilityAnimationType,
@@ -663,7 +664,13 @@
// ui::ImplicitAnimationObserver overrides:
virtual void OnImplicitAnimationsCompleted() OVERRIDE {
- delete this;
+ // ImplicitAnimationObserver's base class uses the object after
+ // calling this function, so we cannot delete |this|. The |layer_|
+ // may be gone by the next message loop run when shutting down, so
+ // clean them up now.
+ if (!delayed_old_layer_deletion_in_cross_fade_for_test_)
+ Cleanup();
+ MessageLoop::current()->DeleteSoon(FROM_HERE, this);
}
private:
@@ -975,5 +982,9 @@
}
}
+void SetDelayedOldLayerDeletionInCrossFadeForTest(bool value) {
+ delayed_old_layer_deletion_in_cross_fade_for_test_ = value;
+}
+
} // namespace internal
} // namespace ash
diff --git a/ash/wm/window_animations.h b/ash/wm/window_animations.h
index 192b0e7..9ddbbac 100644
--- a/ash/wm/window_animations.h
+++ b/ash/wm/window_animations.h
@@ -119,6 +119,10 @@
ASH_EXPORT bool AnimateOnChildWindowVisibilityChanged(
aura::Window* window, bool visible);
+// Delay the old layer deletion so that test can verify the behavior of
+// old layer.
+ASH_EXPORT void SetDelayedOldLayerDeletionInCrossFadeForTest(bool value);
+
} // namespace internal
} // namespace ash
diff --git a/ash/wm/window_animations_unittest.cc b/ash/wm/window_animations_unittest.cc
index 2e521d0..0a5f625 100644
--- a/ash/wm/window_animations_unittest.cc
+++ b/ash/wm/window_animations_unittest.cc
@@ -19,18 +19,7 @@
namespace ash {
namespace internal {
-class WindowAnimationsTest : public ash::test::AshTestBase {
- public:
- WindowAnimationsTest() {}
-
- virtual void TearDown() OVERRIDE {
- ui::LayerAnimator::set_disable_animations_for_test(true);
- AshTestBase::TearDown();
- }
-
- private:
- DISALLOW_COPY_AND_ASSIGN(WindowAnimationsTest);
-};
+typedef ash::test::AshTestBase WindowAnimationsTest;
TEST_F(WindowAnimationsTest, HideShow) {
scoped_ptr<aura::Window> window(
@@ -151,7 +140,7 @@
}
TEST_F(WindowAnimationsTest, CrossFadeToBounds) {
- ui::LayerAnimator::set_disable_animations_for_test(false);
+ internal::SetDelayedOldLayerDeletionInCrossFadeForTest(true);
scoped_ptr<Window> window(
aura::test::CreateTestWindowWithId(0, NULL));
@@ -176,9 +165,8 @@
EXPECT_EQ(1.0f, window->layer()->GetTargetOpacity());
EXPECT_EQ(ui::Transform(), window->layer()->GetTargetTransform());
- // Run the animations to completion.
- old_layer->GetAnimator()->StopAnimating();
- window->layer()->GetAnimator()->StopAnimating();
+ // Allow the animation observer to delete itself.
+ RunAllPendingInMessageLoop();
// Cross fade to a smaller size, as in a restore animation.
old_layer = window->layer();
@@ -196,8 +184,8 @@
EXPECT_EQ(1.0f, window->layer()->GetTargetOpacity());
EXPECT_EQ(ui::Transform(), window->layer()->GetTargetTransform());
- old_layer->GetAnimator()->StopAnimating();
- window->layer()->GetAnimator()->StopAnimating();
+ RunAllPendingInMessageLoop();
+ internal::SetDelayedOldLayerDeletionInCrossFadeForTest(false);
}
TEST_F(WindowAnimationsTest, GetCrossFadeDuration) {
diff --git a/ui/compositor/layer_animation_observer.cc b/ui/compositor/layer_animation_observer.cc
index a4bd5b4..b41676a 100644
--- a/ui/compositor/layer_animation_observer.cc
+++ b/ui/compositor/layer_animation_observer.cc
@@ -56,14 +56,10 @@
// ImplicitAnimationObserver
ImplicitAnimationObserver::ImplicitAnimationObserver()
- : active_(false),
- destroyed_(NULL) {
+ : active_(false) {
}
-ImplicitAnimationObserver::~ImplicitAnimationObserver() {
- if (destroyed_)
- *destroyed_ = true;
-}
+ImplicitAnimationObserver::~ImplicitAnimationObserver() {}
void ImplicitAnimationObserver::SetActive(bool active) {
active_ = active;
@@ -77,12 +73,7 @@
void ImplicitAnimationObserver::OnLayerAnimationEnded(
LayerAnimationSequence* sequence) {
- bool destroyed = false;
- destroyed_ = &destroyed;
sequence->RemoveObserver(this);
- if (destroyed)
- return;
- destroyed_ = NULL;
DCHECK(attached_sequences().find(sequence) == attached_sequences().end());
CheckCompleted();
}
diff --git a/ui/compositor/layer_animation_observer.h b/ui/compositor/layer_animation_observer.h
index 3484a2c4..8810090 100644
--- a/ui/compositor/layer_animation_observer.h
+++ b/ui/compositor/layer_animation_observer.h
@@ -113,10 +113,6 @@
void CheckCompleted();
bool active_;
-
- // Set to true in the destructor (if non-NULL). Used to detect deletion while
- // calling out.
- bool* destroyed_;
};
} // namespace ui
diff --git a/ui/compositor/layer_animator.cc b/ui/compositor/layer_animator.cc
index 1921474..5fc289b 100644
--- a/ui/compositor/layer_animator.cc
+++ b/ui/compositor/layer_animator.cc
@@ -43,16 +43,13 @@
transition_duration_(transition_duration),
tween_type_(Tween::LINEAR),
is_started_(false),
- disable_timer_for_test_(false),
- destroyed_(NULL) {
+ disable_timer_for_test_(false) {
}
LayerAnimator::~LayerAnimator() {
for (size_t i = 0; i < running_animations_.size(); ++i)
running_animations_[i].sequence->OnAnimatorDestroyed();
ClearAnimations();
- if (destroyed_)
- *destroyed_ = true;
}
// static
@@ -283,8 +280,6 @@
void LayerAnimator::Step(base::TimeTicks now) {
TRACE_EVENT0("ui", "LayerAnimator::Step");
- bool destroyed = false;
- destroyed_ = &destroyed;
last_step_time_ = now;
// We need to make a copy of the running animations because progressing them
// and finishing them may indirectly affect the collection of running
@@ -299,15 +294,11 @@
if (delta >= running_animations_copy[i].sequence->duration() &&
!running_animations_copy[i].sequence->is_cyclic()) {
FinishAnimation(running_animations_copy[i].sequence);
- if (destroyed)
- return;
needs_redraw = true;
- } else if (ProgressAnimation(running_animations_copy[i].sequence, delta)) {
+ } else if (ProgressAnimation(running_animations_copy[i].sequence, delta))
needs_redraw = true;
- }
}
- destroyed_ = NULL;
if (needs_redraw && delegate())
delegate()->ScheduleDrawForAnimation();
}
diff --git a/ui/compositor/layer_animator.h b/ui/compositor/layer_animator.h
index ba15f25..65ec27a 100644
--- a/ui/compositor/layer_animator.h
+++ b/ui/compositor/layer_animator.h
@@ -306,10 +306,6 @@
// aborted.
ObserverList<LayerAnimationObserver> observers_;
- // Set to true in the destructor (if non-NULL). Used to detect deletion while
- // calling out.
- bool* destroyed_;
-
DISALLOW_COPY_AND_ASSIGN(LayerAnimator);
};
