Fix crash in holding space when dragging items.

Previously we removed all holding space item views when closing the
holding space tray bubble. This was due to lifecycle concerns since it
is possible for holding space item views to outlive their delegate due
to the holding space bubble widget being asynchronously destroyed.

Removing child views during a destruction sequence can have unintended
side effects as it can trigger re-layout, as was the case in this crash.

This CL removes the logic which removed child views when closing the
bubble widget as item views only interact with the delegate in response
to UI events which should not occur when the widget is being closed.

Bug: 1143426
Change-Id: Ie0548401b8672c2c0c3b571e45c80900edd53b1d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2506513Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#821999}

ash/system/holding_space/holding_space_item_views_container.cc

@@ -17,7 +17,6 @@ HoldingSpaceItemViewsContainer::~HoldingSpaceItemViewsContainer() = default;
 void HoldingSpaceItemViewsContainer::Reset() {
   model_observer_.RemoveAll();
   controller_observer_.RemoveAll();
-  RemoveAllHoldingSpaceItemViews();
 }
 
 void HoldingSpaceItemViewsContainer::ChildPreferredSizeChanged(

ash/system/holding_space/holding_space_item_views_container.h

@@ -28,12 +28,9 @@ class HoldingSpaceItemViewsContainer : public views::View,
       const HoldingSpaceItemViewsContainer& other) = delete;
   ~HoldingSpaceItemViewsContainer() override;
 
-  // Resets the container. Called when the tray bubble starts closing - holding
-  // space item views depend on the `HoldingSpaceItemViewDelegate` which is
-  // reset when the `HoldingSpaceTrayBubble` is reset.
-  // This removes all children that depend on the delegate, and stops observing
-  // holding space model to ensure no new items are created while the bubble
-  // widget is being closed (which happens asynchronously).
+  // Resets the container. Called when the tray bubble starts closing to
+  // stop observing the holding space controller/model to ensure that no new
+  // items are created while the bubble widget is being asynchronously closed.
   void Reset();
 
   virtual void AddHoldingSpaceItemView(const HoldingSpaceItem* item,

ash/system/holding_space/holding_space_tray_bubble.cc

@@ -172,8 +172,9 @@ HoldingSpaceTrayBubble::HoldingSpaceTrayBubble(
 HoldingSpaceTrayBubble::~HoldingSpaceTrayBubble() {
   bubble_wrapper_->bubble_view()->ResetDelegate();
 
-  // Clear container state before `delegate_` is deleted, as container views
-  // depend on it.
+  // Explicitly reset holding space item view containers so that they will stop
+  // observing the holding space controller/model while they are asynchronously
+  // destroyed.
   pinned_files_container_->Reset();
   recent_files_container_->Reset();
 }

chrome/browser/ui/ash/holding_space/holding_space_ui_browsertest.cc

@@ -29,6 +29,14 @@ namespace {
 
 // Helpers ---------------------------------------------------------------------
 
+// Flushes the message loop by posting a task and waiting for it to run.
+void FlushMessageLoop() {
+  base::RunLoop run_loop;
+  base::SequencedTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                   run_loop.QuitClosure());
+  run_loop.Run();
+}
+
 // Performs a double click on `view`.
 void DoubleClick(const views::View* view) {
   auto* root_window = HoldingSpaceBrowserTestBase::GetRootWindowForNewWindows();
@@ -150,9 +158,12 @@ using HoldingSpaceUiBrowserTest = HoldingSpaceBrowserTestBase;
 
 // Verifies that drag-and-drop of holding space items works.
 IN_PROC_BROWSER_TEST_F(HoldingSpaceUiBrowserTest, DragAndDrop) {
+  auto* drop_target_view = DropTargetView::Create(GetRootWindowForNewWindows());
+  drop_target_view->GetWidget()->SetBounds(gfx::Rect(0, 0, 100, 100));
+  drop_target_view->GetWidget()->ShowInactive();
+
+  // Verify drag-and-drop of download items.
   HoldingSpaceItem* const download_file = AddDownloadFile();
-  HoldingSpaceItem* const pinned_file = AddPinnedFile();
-  HoldingSpaceItem* const screenshot_file = AddScreenshotFile();
 
   Show();
   ASSERT_TRUE(IsShowing());
@@ -160,25 +171,50 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceUiBrowserTest, DragAndDrop) {
   std::vector<views::View*> download_chips = GetDownloadChips();
   ASSERT_EQ(1u, download_chips.size());
 
-  std::vector<views::View*> pinned_file_chips = GetPinnedFileChips();
-  ASSERT_EQ(1u, pinned_file_chips.size());
+  MouseDrag(/*from=*/download_chips[0], /*to=*/drop_target_view);
+  EXPECT_EQ(download_file->file_path(), drop_target_view->copied_file_path());
 
-  std::vector<views::View*> screen_capture_views = GetScreenCaptureViews();
-  ASSERT_EQ(1u, screen_capture_views.size());
+  // Drag-and-drop should close holding space UI.
+  FlushMessageLoop();
+  ASSERT_FALSE(IsShowing());
 
-  auto* drop_target_view = DropTargetView::Create(GetRootWindowForNewWindows());
-  drop_target_view->GetWidget()->SetBounds(gfx::Rect(0, 0, 100, 100));
-  drop_target_view->GetWidget()->ShowInactive();
+  // Verify drag-and-drop of pinned file items.
+  // NOTE: Dragging a pinned file from a non-top row of the pinned files
+  // container grid previously resulted in a crash (crbug.com/1143426). To
+  // explicitly test against this case we will add and drag a second row item.
+  HoldingSpaceItem* const pinned_file = AddPinnedFile();
+  AddPinnedFile();
+  AddPinnedFile();
 
-  MouseDrag(/*from=*/download_chips[0], /*to=*/drop_target_view);
-  EXPECT_EQ(download_file->file_path(), drop_target_view->copied_file_path());
+  Show();
+  ASSERT_TRUE(IsShowing());
 
-  MouseDrag(/*from=*/pinned_file_chips[0], /*to=*/drop_target_view);
+  std::vector<views::View*> pinned_file_chips = GetPinnedFileChips();
+  ASSERT_EQ(3u, pinned_file_chips.size());
+
+  MouseDrag(/*from=*/pinned_file_chips.back(), /*to=*/drop_target_view);
   EXPECT_EQ(pinned_file->file_path(), drop_target_view->copied_file_path());
 
+  // Drag-and-drop should close holding space UI.
+  FlushMessageLoop();
+  ASSERT_FALSE(IsShowing());
+
+  // Verify drag-and-drop of screenshot items.
+  HoldingSpaceItem* const screenshot_file = AddScreenshotFile();
+
+  Show();
+  ASSERT_TRUE(IsShowing());
+
+  std::vector<views::View*> screen_capture_views = GetScreenCaptureViews();
+  ASSERT_EQ(1u, screen_capture_views.size());
+
   MouseDrag(/*from=*/screen_capture_views[0], /*to=*/drop_target_view);
   EXPECT_EQ(screenshot_file->file_path(), drop_target_view->copied_file_path());
 
+  // Drag-and-drop should close holding space UI.
+  FlushMessageLoop();
+  ASSERT_FALSE(IsShowing());
+
   drop_target_view->GetWidget()->Close();
 }