Fix potential crash when a fading view is removed.

Fixes a rare use-after-free crash that could occur when an animating
layout manager was already fading out a view and then that view was
removed from its parent.

Also adds a DCHECK and two regression tests to prevent the problem from
occurring again (the DCHECK covers ALL LayoutManagerBase-derived
layouts, not just AnimatingLayoutManager).

Bug: 1037625
Change-Id: I87f4611a081bad7bb4fcffa30ab60a81502aeb1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1981338Reviewed-by: Charlene Yan <cyan@chromium.org>
Commit-Queue: Dana Fried <dfried@chromium.org>
Cr-Commit-Position: refs/heads/master@{#727489}

ui/views/layout/animating_layout_manager.cc

@@ -332,6 +332,17 @@ std::vector<View*> AnimatingLayoutManager::GetChildViewsInPaintOrder(
   return result;
 }
 
+bool AnimatingLayoutManager::OnViewRemoved(View* host, View* view) {
+  // Remove any fade infos corresponding to the removed view.
+  fade_infos_.erase(std::remove_if(fade_infos_.begin(), fade_infos_.end(),
+                                   [view](const LayoutFadeInfo& fade_info) {
+                                     return fade_info.child_view == view;
+                                   }),
+                    fade_infos_.end());
+
+  return LayoutManagerBase::OnViewRemoved(host, view);
+}
+
 void AnimatingLayoutManager::PostOrQueueAction(base::OnceClosure action) {
   if (!is_animating()) {
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, std::move(action));

ui/views/layout/animating_layout_manager.h

@@ -158,6 +158,7 @@ class VIEWS_EXPORT AnimatingLayoutManager : public LayoutManagerBase {
   gfx::Size GetMinimumSize(const View* host) const override;
   int GetPreferredHeightForWidth(const View* host, int width) const override;
   std::vector<View*> GetChildViewsInPaintOrder(const View* host) const override;
+  bool OnViewRemoved(View* host, View* view) override;
 
   // Queues an action to take place after the current animation completes.
   // If |action| needs access to external resources, views, etc. then it must

ui/views/layout/animating_layout_manager_unittest.cc

@@ -1417,6 +1417,86 @@ TEST_F(AnimatingLayoutManagerTest,
   EnsureLayout(expected_end);
 }
 
+// Regression test for crbug.com/1037625: crash in SetViewVisibility() (1/2)
+TEST_F(AnimatingLayoutManagerTest, FlexLayout_RemoveFadingViewDoesNotCrash) {
+  constexpr gfx::Insets kChildMargins(5);
+  layout()->SetShouldAnimateBounds(false);
+  layout()->SetOrientation(LayoutOrientation::kHorizontal);
+  auto* const flex_layout =
+      layout()->SetTargetLayoutManager(std::make_unique<FlexLayout>());
+  flex_layout->SetOrientation(LayoutOrientation::kHorizontal);
+  flex_layout->SetCollapseMargins(true);
+  flex_layout->SetCrossAxisAlignment(LayoutAlignment::kStart);
+  flex_layout->SetDefault(kMarginsKey, kChildMargins);
+  flex_layout->SetDefault(kFlexBehaviorKey, kDropOut);
+
+  const ProposedLayout expected_start{
+      {50, 20},
+      {{child(0), true, {{5, 5}, kChildViewSize}},
+       {child(1), true, {{20, 5}, kChildViewSize}},
+       {child(2), true, {{35, 5}, kChildViewSize}}}};
+
+  // Set up the initial state of the host view and children.
+  SizeAndLayout();
+  EXPECT_FALSE(layout()->is_animating());
+  EnsureLayout(expected_start);
+
+  layout()->FadeOut(child(1));
+
+  animation_api()->IncrementTime(base::TimeDelta::FromMilliseconds(500));
+  view()->Layout();
+  EXPECT_TRUE(layout()->is_animating());
+
+  View* const child1 = child(1);
+  view()->RemoveChildView(child1);
+  delete child1;
+
+  animation_api()->IncrementTime(base::TimeDelta::FromMilliseconds(250));
+  view()->Layout();
+  EXPECT_TRUE(layout()->is_animating());
+
+  animation_api()->IncrementTime(base::TimeDelta::FromMilliseconds(250));
+  view()->Layout();
+  EXPECT_FALSE(layout()->is_animating());
+}
+
+// Regression test for crbug.com/1037625: crash in SetViewVisibility() (2/2)
+TEST_F(AnimatingLayoutManagerTest, FlexLayout_RemoveShowingViewDoesNotCrash) {
+  constexpr gfx::Insets kChildMargins(5);
+  layout()->SetShouldAnimateBounds(false);
+  layout()->SetOrientation(LayoutOrientation::kHorizontal);
+  child(1)->SetVisible(false);
+  auto* const flex_layout =
+      layout()->SetTargetLayoutManager(std::make_unique<FlexLayout>());
+  flex_layout->SetOrientation(LayoutOrientation::kHorizontal);
+  flex_layout->SetCollapseMargins(true);
+  flex_layout->SetCrossAxisAlignment(LayoutAlignment::kStart);
+  flex_layout->SetDefault(kMarginsKey, kChildMargins);
+  flex_layout->SetDefault(kFlexBehaviorKey, kDropOut);
+
+  // Set up the initial state of the host view and children.
+  SizeAndLayout();
+  EXPECT_FALSE(layout()->is_animating());
+
+  layout()->FadeIn(child(1));
+
+  animation_api()->IncrementTime(base::TimeDelta::FromMilliseconds(500));
+  view()->Layout();
+  EXPECT_TRUE(layout()->is_animating());
+
+  View* const child1 = child(1);
+  view()->RemoveChildView(child1);
+  delete child1;
+
+  animation_api()->IncrementTime(base::TimeDelta::FromMilliseconds(250));
+  view()->Layout();
+  EXPECT_TRUE(layout()->is_animating());
+
+  animation_api()->IncrementTime(base::TimeDelta::FromMilliseconds(250));
+  view()->Layout();
+  EXPECT_FALSE(layout()->is_animating());
+}
+
 TEST_F(AnimatingLayoutManagerTest, FlexLayout_FadeInOnAdded) {
   constexpr gfx::Insets kChildMargins(5);
   layout()->SetShouldAnimateBounds(false);

ui/views/layout/layout_manager_base.cc

@@ -120,6 +120,8 @@ void LayoutManagerBase::ApplyLayout(const ProposedLayout& layout) {
     // Since we have a non-const reference to the parent here, we can safely use
     // a non-const reference to the child.
     View* const child_view = child_layout.child_view;
+    // Should not be attempting to modify a child view that has been removed.
+    DCHECK(host_view()->GetIndexOf(child_view) >= 0);
     if (child_view->GetVisible() != child_layout.visible)
       SetViewVisibility(child_view, child_layout.visible);