Speculative ~AppListModel crash fix

Speculative fix. We now observe a folders item list on
creation, but it seems we forgot to stop observing in
one case. When the scoped observer list destructs, it tries
to stop observing things that no longer exist.

This should fix the crash we are seeing in M-86.

Bug: 1130901
Change-Id: I078e570a9beb67fda7efd48ba40ec52bda47aab7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429524Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Alex Newcomer <newcomer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810825}

ash/app_list/model/app_list_folder_item.cc

@@ -16,7 +16,7 @@ namespace ash {
 AppListFolderItem::AppListFolderItem(const std::string& id)
     : AppListItem(id),
       folder_type_(id == kOemFolderId ? FOLDER_TYPE_OEM : FOLDER_TYPE_NORMAL),
-      item_list_(new AppListItemList) {
+      item_list_(std::make_unique<AppListItemList>()) {
   EnsureIconsForAvailableConfigTypes(
       {AppListConfigType::kLarge, AppListConfigType::kMedium,
        AppListConfigType::kSmall},

ash/app_list/model/app_list_item_list.h

@@ -108,7 +108,7 @@ class APP_LIST_MODEL_EXPORT AppListItemList {
   void FixItemPosition(size_t index);
 
   std::vector<std::unique_ptr<AppListItem>> app_list_items_;
-  base::ObserverList<AppListItemListObserver, true>::Unchecked observers_;
+  base::ObserverList<AppListItemListObserver, true> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(AppListItemList);
 };

ash/app_list/model/app_list_item_list_observer.h

@@ -8,12 +8,14 @@
 #include <stddef.h>
 
 #include "ash/app_list/model/app_list_model_export.h"
+#include "base/observer_list_types.h"
 
 namespace ash {
 
 class AppListItem;
 
-class APP_LIST_MODEL_EXPORT AppListItemListObserver {
+class APP_LIST_MODEL_EXPORT AppListItemListObserver
+    : public base::CheckedObserver {
  public:
   // Triggered after |item| has been added to the list at |index|.
   virtual void OnListItemAdded(size_t index, AppListItem* item) {}
@@ -29,7 +31,7 @@ class APP_LIST_MODEL_EXPORT AppListItemListObserver {
                                AppListItem* item) {}
 
  protected:
-  virtual ~AppListItemListObserver() {}
+  ~AppListItemListObserver() override = default;
 };
 
 }  // namespace ash

ash/app_list/model/app_list_model.cc

@@ -19,7 +19,9 @@ AppListModel::AppListModel()
   item_list_scoped_observer_.Add(top_level_item_list_.get());
 }
 
-AppListModel::~AppListModel() = default;
+AppListModel::~AppListModel() {
+  item_list_scoped_observer_.RemoveAll();
+}
 
 void AppListModel::AddObserver(AppListModelObserver* observer) {
   observers_.AddObserver(observer);
@@ -368,9 +370,13 @@ AppListItem* AppListModel::AddItemToFolderItemAndNotify(
 }
 
 std::unique_ptr<AppListItem> AppListModel::RemoveItem(AppListItem* item) {
-  if (!item->IsInFolder())
+  if (!item->IsInFolder()) {
+    if (item->GetItemType() == AppListFolderItem::kItemType) {
+      item_list_scoped_observer_.Remove(
+          static_cast<AppListFolderItem*>(item)->item_list());
+    }
     return top_level_item_list_->RemoveItem(item->id());
-
+  }
   AppListFolderItem* folder = FindFolderItem(item->folder_id());
   return RemoveItemFromFolder(folder, item);
 }

ash/app_list/model/app_list_model_unittest.cc

@@ -226,6 +226,27 @@ TEST_F(AppListModelTest, AppOrder) {
 
 using AppListModelFolderTest = AppListModelTest;
 
+// Test that moving an item into a folder does not crash. (See
+// https://crbug.com/1130901)
+TEST_F(AppListModelFolderTest, MergeItemIntoFolder) {
+  model_.PopulateApps(1);
+
+  AppListItem* item0 = model_.top_level_item_list()->item_at(0);
+  AppListFolderItem* folder = new AppListFolderItem("folder1");
+  model_.AddItem(folder);
+  const size_t num_folder_apps = 2;
+  for (int i = 0; static_cast<size_t>(i) < num_folder_apps; ++i) {
+    std::string name = model_.GetItemName(i);
+    model_.AddItemToFolder(model_.CreateItem(name), folder->id());
+  }
+
+  // Calling MergeItems(AppListItem, AppListFolderItem) results in
+  // AppListFolderItem being deleted. Previously AppListModel did not remove
+  // itself from observing AppListFolderItem's AppListItemList, causing a crash
+  // on ~AppListModel when clearing observers.
+  model_.MergeItems(item0->id(), folder->id());
+}
+
 TEST_F(AppListModelFolderTest, NonSharedConfigIconGeneration) {
   // Ensure any configs set by previous tests are cleared.
   AppListConfigProvider::Get().ResetForTesting();