Fix crash caused by adding page break item

Background:
A crash will happen when a page break item is added between two items
with the same position.

Changes:
Fix the position of items before adding page break item.

AppListItemListTest.AddPageBreakItem
AppListItemListTest.AddPageBreakItemWithSamePosition

Bug: 874619
Test: 
Change-Id: I7e5c9eac51db0fa7a563cb0097c13ab1777218fa
Reviewed-on: https://chromium-review.googlesource.com/1176560Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Weidong Guo <weidongg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583466}

ash/app_list/model/app_list_item_list.cc

@@ -129,11 +129,23 @@ AppListItem* AppListItemList::AddPageBreakItemAfter(
   size_t previous_index;
   CHECK(FindItemIndex(previous_item->id(), &previous_index));
   CHECK(!previous_item->IsInFolder());
-  syncer::StringOrdinal position =
-      previous_index == item_count() - 1
-          ? previous_item->position().CreateAfter()
-          : previous_item->position().CreateBetween(
-                item_at(previous_index + 1)->position());
+
+  const size_t next_index = previous_index + 1;
+  const AppListItem* next_item =
+      next_index < item_count() ? item_at(next_index) : nullptr;
+  syncer::StringOrdinal position;
+  if (!next_item) {
+    position = previous_item->position().CreateAfter();
+  } else {
+    // It is possible that items were added with the same ordinal. To
+    // successfully add the page break item we need to fix this. We do not try
+    // to fix this when an item is added in order to avoid possible edge cases
+    // with sync.
+    if (previous_item->position().Equals(next_item->position()))
+      FixItemPosition(next_index);
+    position = previous_item->position().CreateBetween(next_item->position());
+  }
+
   auto page_break_item = std::make_unique<AppListItem>(base::GenerateGUID());
   page_break_item->set_position(position);
   page_break_item->set_is_page_break(true);

ash/app_list/model/app_list_item_list_unittest.cc

@@ -366,4 +366,39 @@ TEST_F(AppListItemListTest, SetItemPosition) {
   EXPECT_TRUE(VerifyItemOrder4(2, 0, 3, 1));
 }
 
+// Test adding a page break item between two items with different position.
+TEST_F(AppListItemListTest, AddPageBreakItem) {
+  AppListItem* item_0 = CreateAndAddItem(GetItemId(0));
+  AppListItem* item_1 = CreateAndAddItem(GetItemId(1));
+  EXPECT_EQ(item_0, item_list_.item_at(0));
+  EXPECT_EQ(item_1, item_list_.item_at(1));
+  EXPECT_TRUE(item_0->position().LessThan(item_1->position()));
+
+  AppListItem* page_break_item = item_list_.AddPageBreakItemAfter(item_0);
+  EXPECT_EQ(item_0, item_list_.item_at(0));
+  EXPECT_EQ(page_break_item, item_list_.item_at(1));
+  EXPECT_EQ(item_1, item_list_.item_at(2));
+  EXPECT_TRUE(item_0->position().LessThan(page_break_item->position()));
+  EXPECT_TRUE(page_break_item->position().LessThan(item_1->position()));
+}
+
+// Test adding a page break item between two items with the same position.
+TEST_F(AppListItemListTest, AddPageBreakItemWithSamePosition) {
+  AppListItem* item_0 = CreateAndAddItem(GetItemId(0));
+  AppListItem* item_1 = CreateAndAddItem(GetItemId(1));
+  item_list_.SetItemPosition(item_list_.item_at(1),
+                             item_list_.item_at(0)->position());
+  EXPECT_EQ(item_0, item_list_.item_at(0));
+  EXPECT_EQ(item_1, item_list_.item_at(1));
+  EXPECT_TRUE(item_0->position().Equals(item_1->position()));
+
+  // Position of items should be fixed.
+  AppListItem* page_break_item = item_list_.AddPageBreakItemAfter(item_0);
+  EXPECT_EQ(item_0, item_list_.item_at(0));
+  EXPECT_EQ(page_break_item, item_list_.item_at(1));
+  EXPECT_EQ(item_1, item_list_.item_at(2));
+  EXPECT_TRUE(item_0->position().LessThan(page_break_item->position()));
+  EXPECT_TRUE(page_break_item->position().LessThan(item_1->position()));
+}
+
 }  // namespace app_list