M90: Speculative fix for crash in AppsGridView::EndDrag / MoveItemInList

See bug for detailed analysis. I suspect an item is being moved off
the end of the AppListItemList. This could happen if PagedViewStructure
is slightly out-of-sync with the AppListItemList.

I suspect some combination of page breaks, sync updates, and page flip
during dragging causes the crash, but I could not repro locally. I
added a test that triggers a crash with a similar stack (without my
fix to AppListItemList::MoveItem).

Also, added some more DVLOG logging and updated some CHECK/DCHECK.

(cherry picked from commit 7305175)

Bug: 1166011
Test: Added to ash_unittests and app_list_unittests
Change-Id: Ie4c2c2e3694bc075d69976a1d92ba63e0804e367
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2733005
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#859924}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2742995
Auto-Submit: James Cook <jamescook@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4430@{#238}
Cr-Branched-From: e5ce7dc-refs/heads/master@{#857950}

ash/app_list/model/app_list_item.cc

@@ -75,8 +75,8 @@ size_t AppListItem::ChildItemCount() const {
 }
 
 std::string AppListItem::ToDebugString() const {
-  return id().substr(0, 8) + " '" + name() + "'" + " [" +
-         position().ToDebugString() + "]";
+  return id().substr(0, 8) + " '" + (is_page_break() ? "page_break" : name()) +
+         "'" + " [" + position().ToDebugString() + "]";
 }
 
 // Protected methods

ash/app_list/model/app_list_item_list.cc

@@ -48,14 +48,24 @@ bool AppListItemList::FindItemIndex(const std::string& id, size_t* index) {
 
 void AppListItemList::MoveItem(size_t from_index, size_t to_index) {
   DCHECK_LT(from_index, item_count());
-  DCHECK_LT(to_index, item_count());
-  if (from_index == to_index || item_count() == 1)
+  // Speculative fix for crash, possibly due to single-item folders.
+  // https://crbug.com/937431
+  if (item_count() <= 1)
+    return;
+  // Speculative fix for crash, possibly due |to_index| == item_count().
+  // Make |to_index| point to the last item. https://crbug.com/1166011
+  if (to_index >= item_count()) {
+    DCHECK_GT(item_count(), 1u);
+    to_index = item_count() - 1;
+  }
+  if (from_index == to_index)
     return;
 
   auto target_item = std::move(app_list_items_[from_index]);
   DVLOG(2) << "MoveItem: " << from_index << " -> " << to_index << " ["
            << target_item->position().ToDebugString() << "]";
-  // Remove the target item
+  // Remove the target item. If |from_index| <= |to_index| this changes the
+  // item |to_index| points to, but that's OK.
   app_list_items_.erase(app_list_items_.begin() + from_index);
 
   // Update the position
@@ -162,6 +172,9 @@ AppListItem* AppListItemList::AddPageBreakItemAfter(
 
   AppListItem* item = page_break_item.get();
   size_t index = GetItemSortOrderIndex(item->position(), item->id());
+  DVLOG(2) << "AddPageBreakItemAfter: " << previous_item->id() << " prev index "
+           << previous_index << " next index " << next_index << " count "
+           << item_count() << " add index " << index;
   app_list_items_.insert(app_list_items_.begin() + index,
                          std::move(page_break_item));
   return item;

ash/app_list/model/app_list_item_list.h

@@ -19,6 +19,10 @@
 
 namespace ash {
 
+namespace test {
+class AppsGridViewTest;
+}  // namespace test
+
 class AppListItem;
 
 // Class to manage items in the app list. Used both by AppListModel and
@@ -67,6 +71,7 @@ class APP_LIST_MODEL_EXPORT AppListItemList {
  private:
   friend class AppListItemListTest;
   friend class AppListModel;
+  friend class test::AppsGridViewTest;
 
   // Returns a unique, valid StringOrdinal immediately before |position| or at
   // the end of the list if |position| is invalid.

ash/app_list/model/app_list_item_list_unittest.cc

@@ -23,9 +23,9 @@ namespace {
 
 class TestObserver : public AppListItemListObserver {
  public:
-  TestObserver() : items_added_(0), items_removed_(0), items_moved_(0) {}
+  TestObserver() = default;
 
-  ~TestObserver() override {}
+  ~TestObserver() override = default;
 
   // AppListItemListObserver overriden:
   void OnListItemAdded(size_t index, AppListItem* item) override {
@@ -53,9 +53,9 @@ class TestObserver : public AppListItemListObserver {
   }
 
  private:
-  size_t items_added_;
-  size_t items_removed_;
-  size_t items_moved_;
+  size_t items_added_ = 0;
+  size_t items_removed_ = 0;
+  size_t items_moved_ = 0;
 
   DISALLOW_COPY_AND_ASSIGN(TestObserver);
 };
@@ -68,8 +68,8 @@ std::string GetItemId(int id) {
 
 class AppListItemListTest : public testing::Test {
  public:
-  AppListItemListTest() {}
-  ~AppListItemListTest() override {}
+  AppListItemListTest() = default;
+  ~AppListItemListTest() override = default;
 
   // testing::Test overrides:
   void SetUp() override { item_list_.AddObserver(&observer_); }
@@ -402,4 +402,18 @@ TEST_F(AppListItemListTest, AddPageBreakItemWithSamePosition) {
   EXPECT_TRUE(page_break_item->position().LessThan(item_1->position()));
 }
 
+TEST_F(AppListItemListTest, MoveItemPastEnd) {
+  CreateAndAddItem(GetItemId(0));
+  CreateAndAddItem(GetItemId(1));
+  CreateAndAddItem(GetItemId(2));
+  CreateAndAddItem(GetItemId(3));
+  EXPECT_TRUE(VerifyItemOrder4(0, 1, 2, 3));
+
+  // Move the first item one step beyond the end of the list.
+  item_list_.MoveItem(0, 4);
+
+  // No crash, item moves to the end.
+  EXPECT_TRUE(VerifyItemOrder4(1, 2, 3, 0));
+}
+
 }  // namespace ash

ash/app_list/paged_view_structure.cc

@@ -139,9 +139,14 @@ void PagedViewStructure::Add(AppListItemView* view,
                              bool clear_overflow,
                              bool clear_empty_pages) {
   const int view_structure_size = total_pages();
-  CHECK((target_index.page < view_structure_size &&
-         target_index.slot <= items_on_page(target_index.page)) ||
-        (target_index.page == view_structure_size && target_index.slot == 0));
+  if (target_index.page < view_structure_size) {
+    // Adding to an existing page.
+    CHECK_LE(target_index.slot, items_on_page(target_index.page));
+  } else {
+    // Adding to a new page at the end.
+    CHECK_EQ(target_index.page, view_structure_size);
+    CHECK_EQ(target_index.slot, 0);
+  }
 
   if (target_index.page == view_structure_size)
     pages_.emplace_back();
@@ -273,7 +278,7 @@ int PagedViewStructure::GetTargetItemIndexForMove(
     ++current_index.page;
     current_index.slot = 0;
   }
-  DCHECK(current_index == index);
+  DCHECK_EQ(current_index, index);
   return current_item_index - offset;
 }
 

ash/app_list/views/app_list_item_view.cc

@@ -450,7 +450,7 @@ void AppListItemView::SetTouchDragging(bool touch_dragging) {
 
   // EndDrag may delete |this|.
   if (!touch_dragging)
-    apps_grid_view_->EndDrag(false);
+    apps_grid_view_->EndDrag(/*cancel=*/false);
 }
 
 void AppListItemView::SetMouseDragging(bool mouse_dragging) {

ash/app_list/views/apps_grid_view.cc

@@ -2555,9 +2555,11 @@ void AppsGridView::MoveItemInModel(AppListItemView* item_view,
                                    const GridIndex& target,
                                    bool clear_overflow) {
   int current_model_index = view_model_.GetIndexOfView(item_view);
-  size_t current_item_list_index;
-  item_list_->FindItemIndex(item_view->item()->id(), &current_item_list_index);
-  DCHECK_GE(current_model_index, 0);
+  CHECK_GE(current_model_index, 0);
+  size_t current_item_list_index = 0;
+  bool found = item_list_->FindItemIndex(item_view->item()->id(),
+                                         &current_item_list_index);
+  CHECK(found);
 
   int target_model_index = GetTargetModelIndexForMove(item_view, target);
   size_t target_item_list_index = GetTargetItemIndexForMove(item_view, target);
@@ -2566,6 +2568,10 @@ void AppsGridView::MoveItemInModel(AppListItemView* item_view,
   if (!folder_delegate_)
     view_structure_.Move(item_view, target, clear_overflow);
 
+  DVLOG(1) << "MoveItemInModel: view model: " << current_model_index << " -> "
+           << target_model_index << ", item list: " << current_item_list_index
+           << " -> " << target_item_list_index;
+
   // Reorder the app list item views in accordance with |view_model_|.
   items_container_->ReorderChildView(item_view, target_model_index);
   items_container_->NotifyAccessibilityEvent(ax::mojom::Event::kChildrenChanged,

ash/app_list/views/apps_grid_view_unittest.cc

@@ -426,6 +426,16 @@ class AppsGridViewTest : public views::ViewsTestBase,
     return drag_view_center;
   }
 
+  // Calls the private method.
+  static void DeleteItemAt(AppListItemList* item_list, size_t index) {
+    item_list->DeleteItemAt(index);
+  }
+
+  // Calls the private method.
+  void MoveItemInModel(AppListItemView* item_view, const GridIndex& target) {
+    apps_grid_view_->MoveItemInModel(item_view, target);
+  }
+
   AppListView* app_list_view_ = nullptr;    // Owned by native widget.
   AppsGridView* apps_grid_view_ = nullptr;  // Owned by |app_list_view_|.
   ContentsView* contents_view_ = nullptr;   // Owned by |app_list_view_|.
@@ -1338,6 +1348,30 @@ TEST_P(AppsGridViewTest, MouseDragWithCancelDeleteAddItem) {
   test_api_->LayoutToIdealBounds();
 }
 
+// Regression test for crash bug. https://crbug.com/1166011.
+TEST_F(AppsGridViewTest, MoveItemInModelPastEndOfList) {
+  model_->PopulateApps(20);
+
+  // I speculate that the item list is missing an item, but PagedViewStructure
+  // doesn't know about it. This could happen if an item was deleted during a
+  // period that AppsGridView was not observing the list.
+  AppListItemList* item_list = model_->top_level_item_list();
+  item_list->RemoveObserver(apps_grid_view_);
+  DeleteItemAt(item_list, 19);
+  item_list->AddObserver(apps_grid_view_);
+  ASSERT_EQ(19u, item_list->item_count());
+
+  // Try to move the first item to the end of the page. PagedViewStructure is
+  // out of sync with AppListItemList, so it will return a target item list
+  // index off the end of the list.
+  AppListItemView* first_item = GetItemViewInTopLevelGrid(0);
+  MoveItemInModel(first_item, GridIndex(0, 19));
+
+  // No crash. Item moved to the end.
+  ASSERT_EQ(19u, item_list->item_count());
+  EXPECT_EQ("Item 0", item_list->item_at(18)->id());
+}
+
 // Test that control+arrow swaps app within the same page.
 TEST_F(AppsGridViewTest, ControlArrowSwapsAppsWithinSamePage) {
   model_->PopulateApps(GetTilesPerPage(0));