Fix crash in holding space view containers.

Previously the code which determined the index at which to insert a view
for a finalized item assumed that newer finalized items would have
already been added as views before older finalized items. This
assumption is not true when showing UI with a model already containing
finalized items.

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

ash/system/holding_space/holding_space_item_views_container.cc

@@ -35,7 +35,7 @@ void HoldingSpaceItemViewsContainer::OnHoldingSpaceModelAttached(
   model_observer_.Add(model);
   for (const auto& item : model->items()) {
     if (item->IsFinalized())
-      AddHoldingSpaceItemView(item.get());
+      AddHoldingSpaceItemView(item.get(), /*due_to_finalization=*/false);
   }
 }
 
@@ -50,7 +50,7 @@ void HoldingSpaceItemViewsContainer::OnHoldingSpaceItemAdded(
   if (!item->IsFinalized())
     return;
 
-  AddHoldingSpaceItemView(item);
+  AddHoldingSpaceItemView(item, /*due_to_finalization=*/false);
 }
 
 void HoldingSpaceItemViewsContainer::OnHoldingSpaceItemRemoved(
@@ -60,7 +60,7 @@ void HoldingSpaceItemViewsContainer::OnHoldingSpaceItemRemoved(
 
 void HoldingSpaceItemViewsContainer::OnHoldingSpaceItemFinalized(
     const HoldingSpaceItem* item) {
-  AddHoldingSpaceItemView(item);
+  AddHoldingSpaceItemView(item, /*due_to_finalization=*/true);
 }
 
 }  // namespace ash

ash/system/holding_space/holding_space_item_views_container.h

@@ -36,7 +36,8 @@ class HoldingSpaceItemViewsContainer : public views::View,
   // widget is being closed (which happens asynchronously).
   void Reset();
 
-  virtual void AddHoldingSpaceItemView(const HoldingSpaceItem* item) = 0;
+  virtual void AddHoldingSpaceItemView(const HoldingSpaceItem* item,
+                                       bool due_to_finalization) = 0;
   virtual void RemoveAllHoldingSpaceItemViews() = 0;
   virtual void RemoveHoldingSpaceItemView(const HoldingSpaceItem* item) = 0;
 

ash/system/holding_space/holding_space_tray_unittest.cc

@@ -4,7 +4,11 @@
 
 #include "ash/system/holding_space/holding_space_tray.h"
 
+#include <deque>
+#include <vector>
+
 #include "ash/public/cpp/ash_features.h"
+#include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_controller.h"
 #include "ash/public/cpp/holding_space/holding_space_image.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
@@ -310,6 +314,38 @@ TEST_F(HoldingSpaceTrayTest, DownloadsContainer) {
   test_api()->Close();
 }
 
+// Verifies the downloads container is shown and orders items as expected when
+// the model contains a number of finalized items prior to showing UI.
+TEST_F(HoldingSpaceTrayTest, DownloadsContainerWithFinalizedItemsOnly) {
+  MarkTimeOfFirstPin();
+  StartSession();
+
+  // Add a number of finalized download items.
+  std::deque<HoldingSpaceItem*> items;
+  for (size_t i = 0; i < kMaxDownloads; ++i) {
+    items.push_back(
+        AddItem(HoldingSpaceItem::Type::kDownload,
+                base::FilePath("/tmp/fake_" + base::NumberToString(i))));
+  }
+
+  test_api()->Show();
+  EXPECT_TRUE(test_api()->RecentFilesContainerShown());
+
+  std::vector<views::View*> download_files = test_api()->GetDownloadChips();
+  ASSERT_EQ(items.size(), download_files.size());
+
+  while (!items.empty()) {
+    // View order is expected to be reverse of item order.
+    auto* download_file = HoldingSpaceItemView::Cast(download_files.back());
+    EXPECT_EQ(download_file->item()->id(), items.front()->id());
+
+    items.pop_front();
+    download_files.pop_back();
+  }
+
+  test_api()->Close();
+}
+
 TEST_F(HoldingSpaceTrayTest, FinalizingDownloadItemThatShouldBeInvisible) {
   StartSession();
   test_api()->Show();
@@ -506,6 +542,38 @@ TEST_F(HoldingSpaceTrayTest, ScreenCaptureContainer) {
   test_api()->Close();
 }
 
+// Verifies the screen captures container is shown and orders items as expected
+// when the model contains a number of finalized items prior to showing UI.
+TEST_F(HoldingSpaceTrayTest, ScreenCapturesContainerWithFinalizedItemsOnly) {
+  MarkTimeOfFirstPin();
+  StartSession();
+
+  // Add a number of finalized screen capture items.
+  std::deque<HoldingSpaceItem*> items;
+  for (size_t i = 0; i < kMaxScreenCaptures; ++i) {
+    items.push_back(
+        AddItem(HoldingSpaceItem::Type::kScreenshot,
+                base::FilePath("/tmp/fake_" + base::NumberToString(i))));
+  }
+
+  test_api()->Show();
+  EXPECT_TRUE(test_api()->RecentFilesContainerShown());
+
+  std::vector<views::View*> screenshots = test_api()->GetScreenCaptureViews();
+  ASSERT_EQ(items.size(), screenshots.size());
+
+  while (!items.empty()) {
+    // View order is expected to be reverse of item order.
+    auto* screenshot = HoldingSpaceItemView::Cast(screenshots.back());
+    EXPECT_EQ(screenshot->item()->id(), items.front()->id());
+
+    items.pop_front();
+    screenshots.pop_back();
+  }
+
+  test_api()->Close();
+}
+
 TEST_F(HoldingSpaceTrayTest, FinalizingScreenCaptureItemThatShouldBeInvisible) {
   StartSession();
   test_api()->Show();
@@ -772,4 +840,36 @@ TEST_F(HoldingSpaceTrayTest,
   test_api()->Close();
 }
 
+// Verifies the pinned items container is shown and orders items as expected
+// when the model contains a number of finalized items prior to showing UI.
+TEST_F(HoldingSpaceTrayTest, PinnedFilesContainerWithFinalizedItemsOnly) {
+  MarkTimeOfFirstPin();
+  StartSession();
+
+  // Add a number of finalized pinned items.
+  std::deque<HoldingSpaceItem*> items;
+  for (int i = 0; i < 10; ++i) {
+    items.push_back(
+        AddItem(HoldingSpaceItem::Type::kPinnedFile,
+                base::FilePath("/tmp/fake_" + base::NumberToString(i))));
+  }
+
+  test_api()->Show();
+  EXPECT_TRUE(test_api()->PinnedFilesContainerShown());
+
+  std::vector<views::View*> pinned_files = test_api()->GetPinnedFileChips();
+  ASSERT_EQ(items.size(), pinned_files.size());
+
+  while (!items.empty()) {
+    // View order is expected to be reverse of item order.
+    auto* pinned_file = HoldingSpaceItemView::Cast(pinned_files.back());
+    EXPECT_EQ(pinned_file->item()->id(), items.front()->id());
+
+    items.pop_front();
+    pinned_files.pop_back();
+  }
+
+  test_api()->Close();
+}
+
 }  // namespace ash

ash/system/holding_space/pinned_files_container.cc

@@ -147,23 +147,26 @@ void PinnedFilesContainer::ViewHierarchyChanged(
   SetVisible(details.is_add);
 }
 
-void PinnedFilesContainer::AddHoldingSpaceItemView(
-    const HoldingSpaceItem* item) {
+void PinnedFilesContainer::AddHoldingSpaceItemView(const HoldingSpaceItem* item,
+                                                   bool due_to_finalization) {
   DCHECK(!base::Contains(views_by_item_id_, item->id()));
   DCHECK(item->IsFinalized());
 
   if (item->type() != HoldingSpaceItem::Type::kPinnedFile)
     return;
 
-  // Find the position to which the view should be added.
   size_t index = 0;
-  for (const auto& candidate :
-       base::Reversed(HoldingSpaceController::Get()->model()->items())) {
-    if (candidate->id() == item->id())
-      break;
-    if (candidate->IsFinalized() &&
-        candidate->type() == HoldingSpaceItem::Type::kPinnedFile) {
-      ++index;
+
+  if (due_to_finalization) {
+    // Find the position at which the view should be added.
+    for (const auto& candidate :
+         base::Reversed(HoldingSpaceController::Get()->model()->items())) {
+      if (candidate->id() == item->id())
+        break;
+      if (candidate->IsFinalized() &&
+          candidate->type() == HoldingSpaceItem::Type::kPinnedFile) {
+        ++index;
+      }
     }
   }
 

ash/system/holding_space/pinned_files_container.h

@@ -28,7 +28,8 @@ class PinnedFilesContainer : public HoldingSpaceItemViewsContainer {
 
   // HoldingSpaceItemViewsContainer:
   void ViewHierarchyChanged(const views::ViewHierarchyChangedDetails&) override;
-  void AddHoldingSpaceItemView(const HoldingSpaceItem* item) override;
+  void AddHoldingSpaceItemView(const HoldingSpaceItem* item,
+                               bool due_to_finalization) override;
   void RemoveAllHoldingSpaceItemViews() override;
   void RemoveHoldingSpaceItemView(const HoldingSpaceItem* item) override;
 

ash/system/holding_space/recent_files_container.cc

@@ -146,8 +146,8 @@ void RecentFilesContainer::ViewHierarchyChanged(
     OnDownloadsContainerViewHierarchyChanged(details);
 }
 
-void RecentFilesContainer::AddHoldingSpaceItemView(
-    const HoldingSpaceItem* item) {
+void RecentFilesContainer::AddHoldingSpaceItemView(const HoldingSpaceItem* item,
+                                                   bool due_to_finalization) {
   DCHECK(item->IsFinalized());
 
   if (item->type() != HoldingSpaceItem::Type::kScreenshot &&
@@ -156,12 +156,16 @@ void RecentFilesContainer::AddHoldingSpaceItemView(
   }
 
   size_t index = 0;
-  for (const auto& candidate :
-       base::Reversed(HoldingSpaceController::Get()->model()->items())) {
-    if (candidate->id() == item->id())
-      break;
-    if (candidate->IsFinalized() && candidate->type() == item->type())
-      ++index;
+
+  if (due_to_finalization) {
+    // Find the position at which the view should be added.
+    for (const auto& candidate :
+         base::Reversed(HoldingSpaceController::Get()->model()->items())) {
+      if (candidate->id() == item->id())
+        break;
+      if (candidate->IsFinalized() && candidate->type() == item->type())
+        ++index;
+    }
   }
 
   if (item->type() == HoldingSpaceItem::Type::kScreenshot)

ash/system/holding_space/recent_files_container.h

@@ -29,7 +29,8 @@ class RecentFilesContainer : public HoldingSpaceItemViewsContainer {
   // HoldingSpaceItemViewsContainer:
   void ChildVisibilityChanged(views::View* child) override;
   void ViewHierarchyChanged(const views::ViewHierarchyChangedDetails&) override;
-  void AddHoldingSpaceItemView(const HoldingSpaceItem* item) override;
+  void AddHoldingSpaceItemView(const HoldingSpaceItem* item,
+                               bool due_to_finalization) override;
   void RemoveAllHoldingSpaceItemViews() override;
   void RemoveHoldingSpaceItemView(const HoldingSpaceItem* item) override;