Speculative fix of OnAnimatorDetached crash

Suspect the crash is because an animation observer of a child
layer deletes the parent/child when stopping the BOUNDS animation
in the Remove call.

Also changes a simillar situation in SwitchToLayer.

Bug: 1088432
Change-Id: Id9bacb230a9d0a6fa390ae3bc1a0410c983fb049
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2224031
Commit-Queue: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775267}

ui/compositor/layer.cc

@@ -308,7 +308,9 @@ void Layer::SetShowReflectedLayerSubtree(Layer* subtree_reflected_layer) {
 
   scoped_refptr<cc::MirrorLayer> new_layer =
       cc::MirrorLayer::Create(subtree_reflected_layer->cc_layer_);
-  SwitchToLayer(new_layer);
+  if (!SwitchToLayer(new_layer))
+    return;
+
   mirror_layer_ = std::move(new_layer);
 
   subtree_reflected_layer_ = subtree_reflected_layer;
@@ -375,12 +377,20 @@ void Layer::Add(Layer* child) {
 }
 
 void Layer::Remove(Layer* child) {
+  base::WeakPtr<Layer> weak_this = weak_ptr_factory_.GetWeakPtr();
+  base::WeakPtr<Layer> weak_child = child->weak_ptr_factory_.GetWeakPtr();
+
   // Current bounds are used to calculate offsets when layers are reparented.
   // Stop (and complete) an ongoing animation to update the bounds immediately.
   LayerAnimator* child_animator = child->animator_.get();
   if (child_animator)
     child_animator->StopAnimatingProperty(ui::LayerAnimationElement::BOUNDS);
 
+  // Do not proceed if |this| or |child| is released by an animation observer
+  // of |child|'s bounds animation.
+  if (!weak_this || !weak_child)
+    return;
+
   Compositor* compositor = GetCompositor();
   if (compositor)
     child->ResetCompositorForAnimatorsInTree(compositor);
@@ -725,11 +735,19 @@ void Layer::SetName(const std::string& name) {
   cc_layer_->SetDebugName(name);
 }
 
-void Layer::SwitchToLayer(scoped_refptr<cc::Layer> new_layer) {
+bool Layer::SwitchToLayer(scoped_refptr<cc::Layer> new_layer) {
   // Finish animations being handled by cc_layer_.
   if (animator_) {
+    base::WeakPtr<Layer> weak_this = weak_ptr_factory_.GetWeakPtr();
+
     animator_->StopAnimatingProperty(LayerAnimationElement::TRANSFORM);
+    if (!weak_this)
+      return false;
+
     animator_->StopAnimatingProperty(LayerAnimationElement::OPACITY);
+    if (!weak_this)
+      return false;
+
     animator_->SwitchToLayer(new_layer);
   }
 
@@ -781,12 +799,16 @@ void Layer::SwitchToLayer(scoped_refptr<cc::Layer> new_layer) {
 
   SetLayerFilters();
   SetLayerBackgroundFilters();
+  return true;
 }
 
-void Layer::SwitchCCLayerForTest() {
+bool Layer::SwitchCCLayerForTest() {
   scoped_refptr<cc::PictureLayer> new_layer = cc::PictureLayer::Create(this);
-  SwitchToLayer(new_layer);
+  if (!SwitchToLayer(new_layer))
+    return false;
+
   content_layer_ = std::move(new_layer);
+  return true;
 }
 
 // Note: The code that sets this flag would be responsible to unset it on that
@@ -889,7 +911,9 @@ void Layer::SetTransferableResource(
     scoped_refptr<cc::TextureLayer> new_layer =
         cc::TextureLayer::CreateForMailbox(this);
     new_layer->SetFlipped(true);
-    SwitchToLayer(new_layer);
+    if (!SwitchToLayer(new_layer))
+      return;
+
     texture_layer_ = new_layer;
     // Reset the frame_size_in_dip_ so that SetTextureSize() will not early out,
     // the frame_size_in_dip_ was for a previous (different) |texture_layer_|.
@@ -972,7 +996,9 @@ void Layer::SetShowReflectedSurface(const viz::SurfaceId& surface_id,
 
   if (!surface_layer_) {
     scoped_refptr<cc::SurfaceLayer> new_layer = cc::SurfaceLayer::Create();
-    SwitchToLayer(new_layer);
+    if (!SwitchToLayer(new_layer))
+      return;
+
     surface_layer_ = new_layer;
   }
 
@@ -1007,7 +1033,9 @@ void Layer::SetShowSolidColorContent() {
     return;
 
   scoped_refptr<cc::SolidColorLayer> new_layer = cc::SolidColorLayer::Create();
-  SwitchToLayer(new_layer);
+  if (!SwitchToLayer(new_layer))
+    return;
+
   solid_color_layer_ = new_layer;
 
   transfer_resource_ = viz::TransferableResource();
@@ -1620,7 +1648,9 @@ void Layer::CreateSurfaceLayerIfNecessary() {
     return;
   scoped_refptr<cc::SurfaceLayer> new_layer = cc::SurfaceLayer::Create();
   new_layer->SetSurfaceHitTestable(true);
-  SwitchToLayer(new_layer);
+  if (!SwitchToLayer(new_layer))
+    return;
+
   surface_layer_ = new_layer;
 }
 

ui/compositor/layer.h

@@ -458,7 +458,7 @@ class COMPOSITOR_EXPORT Layer : public LayerAnimationDelegate,
   float device_scale_factor() const { return device_scale_factor_; }
 
   // Triggers a call to SwitchToLayer.
-  void SwitchCCLayerForTest();
+  bool SwitchCCLayerForTest();
 
   const cc::Region& damaged_region_for_testing() const {
     return damaged_region_;
@@ -575,8 +575,11 @@ class COMPOSITOR_EXPORT Layer : public LayerAnimationDelegate,
   // Set all filters which got applied to the layer background.
   void SetLayerBackgroundFilters();
 
-  // Cleanup |cc_layer_| and replaces it with |new_layer|.
-  void SwitchToLayer(scoped_refptr<cc::Layer> new_layer);
+  // Cleanup |cc_layer_| and replaces it with |new_layer|. When stopping
+  // animations handled by old cc layer before the switch, |this| could be
+  // released by an animation observer. Returns false when it happens and
+  // callers should take cautions as well. Otherwise returns true.
+  bool SwitchToLayer(scoped_refptr<cc::Layer> new_layer) WARN_UNUSED_RESULT;
 
   void SetCompositorForAnimatorsInTree(Compositor* compositor);
   void ResetCompositorForAnimatorsInTree(Compositor* compositor);

ui/compositor/layer_unittest.cc

@@ -790,7 +790,7 @@ TEST_F(LayerWithDelegateTest, Cloning) {
   layer->SetFillsBoundsOpaquely(false);
   // Color and opaqueness targets should be preserved during cloning, even after
   // switching away from solid color content.
-  layer->SwitchCCLayerForTest();
+  ASSERT_TRUE(layer->SwitchCCLayerForTest());
 
   clone = layer->Clone();
 
@@ -2427,7 +2427,7 @@ TEST_P(LayerWithRealCompositorTest, SwitchCCLayerAnimations) {
   l1->SetOpacity(0.5f);
 
   // Change l1's cc::Layer.
-  l1->SwitchCCLayerForTest();
+  ASSERT_TRUE(l1->SwitchCCLayerForTest());
 
   // Ensure that the opacity animation completed.
   EXPECT_FLOAT_EQ(l1->opacity(), 0.5f);
@@ -2449,7 +2449,7 @@ TEST_P(LayerWithRealCompositorTest, SwitchCCLayerSolidColorNotAnimating) {
   EXPECT_EQ(transparent, root->GetTargetColor());
 
   // Changing the underlying layer should not affect targets.
-  root->SwitchCCLayerForTest();
+  ASSERT_TRUE(root->SwitchCCLayerForTest());
 
   EXPECT_FALSE(root->fills_bounds_opaquely());
   EXPECT_FALSE(
@@ -2487,7 +2487,7 @@ TEST_P(LayerWithRealCompositorTest, SwitchCCLayerSolidColorWhileAnimating) {
   EXPECT_EQ(transparent, root->GetTargetColor());
 
   // Changing the underlying layer should not affect targets.
-  root->SwitchCCLayerForTest();
+  ASSERT_TRUE(root->SwitchCCLayerForTest());
 
   EXPECT_TRUE(root->fills_bounds_opaquely());
   EXPECT_TRUE(
@@ -2515,7 +2515,7 @@ TEST_P(LayerWithRealCompositorTest, SwitchCCLayerCacheRenderSurface) {
   l1->AddCacheRenderSurfaceRequest();
 
   // Change l1's cc::Layer.
-  l1->SwitchCCLayerForTest();
+  ASSERT_TRUE(l1->SwitchCCLayerForTest());
 
   // Ensure that the cache_render_surface flag is maintained.
   EXPECT_TRUE(l1->cc_layer_for_testing()->cache_render_surface());
@@ -2532,7 +2532,7 @@ TEST_P(LayerWithRealCompositorTest, SwitchCCLayerTrilinearFiltering) {
   l1->AddTrilinearFilteringRequest();
 
   // Change l1's cc::Layer.
-  l1->SwitchCCLayerForTest();
+  ASSERT_TRUE(l1->SwitchCCLayerForTest());
 
   // Ensure that the trilinear_filtering flag is maintained.
   EXPECT_TRUE(l1->cc_layer_for_testing()->trilinear_filtering());
@@ -2550,12 +2550,38 @@ TEST_P(LayerWithRealCompositorTest, SwitchCCLayerMasksToBounds) {
   EXPECT_TRUE(l1->cc_layer_for_testing()->masks_to_bounds());
 
   // Change l1's cc::Layer.
-  l1->SwitchCCLayerForTest();
+  ASSERT_TRUE(l1->SwitchCCLayerForTest());
 
   // Ensure that the trilinear_filtering flag is maintained.
   EXPECT_TRUE(l1->cc_layer_for_testing()->masks_to_bounds());
 }
 
+// Tests that no crash happens when switching cc layer with an animation
+// observer that deletes the layer itself.
+TEST_P(LayerWithRealCompositorTest, SwitchCCLayerDeleteLayer) {
+  std::unique_ptr<Layer> root(CreateLayer(LAYER_TEXTURED));
+  std::unique_ptr<Layer> l1(CreateLayer(LAYER_TEXTURED));
+  GetCompositor()->SetRootLayer(root.get());
+  root->Add(l1.get());
+
+  TestCallbackAnimationObserver animation_observer;
+  animation_observer.SetCallback(
+      base::BindLambdaForTesting([&]() { l1.reset(); }));
+
+  auto long_duration_animation =
+      std::make_unique<ui::ScopedAnimationDurationScaleMode>(
+          ui::ScopedAnimationDurationScaleMode::SLOW_DURATION);
+  {
+    ui::ScopedLayerAnimationSettings animation(l1->GetAnimator());
+    animation.AddObserver(&animation_observer);
+    animation.SetTransitionDuration(base::TimeDelta::FromMilliseconds(1000));
+    l1->SetOpacity(0.f);
+  }
+
+  // Fails but no crash.
+  EXPECT_FALSE(l1->SwitchCCLayerForTest());
+}
+
 // Triggerring a OnDeviceScaleFactorChanged while a layer is undergoing
 // transform animation, may cause a crash. This is because an animation observer
 // may mutate the tree, e.g. deleting a layer, changing ancestor z-order etc,
@@ -2604,9 +2630,6 @@ TEST_P(LayerWithRealCompositorTest, TreeMutationDuringScaleFactorChange) {
   root->Add(layer_to_delete.get());
   layer_to_delete->Add(child.get());
 
-  long_duration_animation =
-      std::make_unique<ui::ScopedAnimationDurationScaleMode>(
-          ui::ScopedAnimationDurationScaleMode::SLOW_DURATION);
   {
     ui::ScopedLayerAnimationSettings animation(layer_to_delete->GetAnimator());
     animation.AddObserver(&animation_observer);
@@ -2629,9 +2652,6 @@ TEST_P(LayerWithRealCompositorTest, TreeMutationDuringScaleFactorChange) {
   layer_to_delete->Add(child.get());
   layer_to_delete->Add(child2.get());
 
-  long_duration_animation =
-      std::make_unique<ui::ScopedAnimationDurationScaleMode>(
-          ui::ScopedAnimationDurationScaleMode::SLOW_DURATION);
   {
     ui::ScopedLayerAnimationSettings animation(child->GetAnimator());
     animation.AddObserver(&animation_observer);
@@ -2666,6 +2686,60 @@ TEST_P(LayerWithRealCompositorTest, TreeMutationDuringScaleFactorChange) {
   root->OnDeviceScaleFactorChanged(1.5f);
 }
 
+// Tests that no crash when parent/child layer is released by an animation
+// observer of the child layer bounds animation.
+TEST_P(LayerWithRealCompositorTest, ParentOrChildGoneDuringRemove) {
+  std::unique_ptr<Layer> root = CreateLayer(LAYER_SOLID_COLOR);
+  GetCompositor()->SetRootLayer(root.get());
+  root->SetBounds(gfx::Rect(0, 0, 100, 100));
+
+  // Actions to taken on animation ends.
+  enum class Action {
+    kReleaseParent,
+    kReleaseChild,
+  };
+
+  for (auto action : {Action::kReleaseParent, Action::kReleaseChild}) {
+    SCOPED_TRACE(::testing::Message() << "action=" << static_cast<int>(action));
+
+    std::unique_ptr<Layer> parent = CreateLayer(LAYER_SOLID_COLOR);
+    parent->SetBounds(gfx::Rect(0, 0, 100, 100));
+
+    std::unique_ptr<Layer> child = CreateLayer(LAYER_SOLID_COLOR);
+    child->SetBounds(gfx::Rect(0, 0, 100, 100));
+    parent->Add(child.get());
+
+    root->Add(parent.get());
+
+    // An animation observer that takes action on animation ends.
+    TestCallbackAnimationObserver animation_observer;
+    animation_observer.SetCallback(base::BindLambdaForTesting([&]() {
+      switch (action) {
+        case Action::kReleaseParent:
+          parent.reset();
+          break;
+        case Action::kReleaseChild:
+          child.reset();
+          break;
+      }
+    }));
+
+    // Schedule a bounds animation on |child|.
+    auto long_duration_animation =
+        std::make_unique<ui::ScopedAnimationDurationScaleMode>(
+            ui::ScopedAnimationDurationScaleMode::SLOW_DURATION);
+    {
+      ui::ScopedLayerAnimationSettings animation(child->GetAnimator());
+      animation.AddObserver(&animation_observer);
+      animation.SetTransitionDuration(base::TimeDelta::FromMilliseconds(1000));
+      child->SetBounds(gfx::Rect(10, 20, 100, 100));
+    }
+
+    // No crash should happen when removing |child| with a  bounds animation.
+    parent->Remove(child.get());
+  }
+}
+
 // Tests that the animators in the layer tree is added to the
 // animator-collection when the root-layer is set to the compositor.
 TEST_F(LayerWithDelegateTest, RootLayerAnimatorsInCompositor) {