Fix crash for installing an app

Changes:
If the first available position for the newly installed app does not
exist when we are inserting between an app and a page break item with
the same position, we use the next available position.

Bug: 907637
Test: AppListSyncableServiceTest.InvalidFirstAvailablePosition
Change-Id: I8f1a7abad58ea838b452ac8a270c4a030df57a1e
Reviewed-on: https://chromium-review.googlesource.com/c/1347208
Commit-Queue: Weidong Guo <weidongg@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Reviewed-by: Yury Khmel <khmel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610286}

chrome/browser/ui/app_list/app_list_model_updater.cc

@@ -34,9 +34,16 @@ syncer::StringOrdinal AppListModelUpdater::GetFirstAvailablePositionInternal(
     const int max_items_in_page =
         app_list::AppListConfig::instance().GetMaxNumOfItemsPerPage(page);
     if (items_in_page > 0 && items_in_page < max_items_in_page) {
+      // Sometimes two continuous items may have the same position, so skip to
+      // the next available position.
+      // |i| should always be larger than 0 here because |items_in_page| is
+      // larger than 0.
+      if (sorted_items[i - 1]->position().LessThan(
+              sorted_items[i]->position())) {
         return sorted_items[i - 1]->position().CreateBetween(
             sorted_items[i]->position());
       }
+    }
     if (items_in_page > 0)
       ++page;
     items_in_page = 0;

chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc

@@ -781,3 +781,37 @@ TEST_F(AppListSyncableServiceTest, FirstAvailablePosition) {
   EXPECT_TRUE(page_break_position.CreateAfter().Equals(
       model_updater()->GetFirstAvailablePosition()));
 }
+
+// Test that installing an app between two items with the same position will put
+// that app at next available position. The test also ensures that no crash
+// occurs (See https://crbug.com/907637).
+TEST_F(AppListSyncableServiceTest, FirstAvailablePositionNotExist) {
+  RemoveAllExistingItems();
+
+  // Populate the first page with items and leave 1 empty slot at the end.
+  const int max_items_in_first_page =
+      app_list::AppListConfig::instance().GetMaxNumOfItemsPerPage(0);
+  syncer::StringOrdinal last_app_position =
+      syncer::StringOrdinal::CreateInitialOrdinal();
+  for (int i = 0; i < max_items_in_first_page - 1; ++i) {
+    std::unique_ptr<ChromeAppListItem> item =
+        std::make_unique<ChromeAppListItem>(
+            profile_.get(), GenerateId("item_id" + base::IntToString(i)),
+            model_updater());
+    item->SetPosition(last_app_position);
+    model_updater()->AddItem(std::move(item));
+    if (i < max_items_in_first_page - 2)
+      last_app_position = last_app_position.CreateAfter();
+  }
+
+  // Add a "page break" item at the end of first page with the same position as
+  // last app item.
+  std::unique_ptr<ChromeAppListItem> page_break_item =
+      std::make_unique<ChromeAppListItem>(
+          profile_.get(), GenerateId("page_break_item_id"), model_updater());
+  page_break_item->SetPosition(last_app_position);
+  page_break_item->SetIsPageBreak(true);
+  model_updater()->AddItem((std::move(page_break_item)));
+  EXPECT_TRUE(last_app_position.CreateAfter().Equals(
+      model_updater()->GetFirstAvailablePosition()));
+}