Fixing crasher upon divergence between ChromLauncherController and ShelfItemDelegateManager's controller lists

Since we haven't found out how the two lists are getting into this state, this fix (for M39) is adding a logic
to synchronize the controller list "the hard way".

We should follow up later to get this properly fixed.

BUG=429870
TEST=none since no way of getting into the state is known (but it exists)

Review URL: https://codereview.chromium.org/701303002

Cr-Commit-Position: refs/heads/master@{#303380}

6 files changed, 154 insertions, 68 deletions

diff --git a/ash/shelf/shelf_item_delegate_manager.cc b/ash/shelf/shelf_item_delegate_manager.cc
index d5b3d04..256ee67 100644
--- a/ash/shelf/shelf_item_delegate_manager.cc
+++ b/ash/shelf/shelf_item_delegate_manager.cc
@@ -24,6 +24,16 @@ ShelfItemDelegateManager::~ShelfItemDelegateManager() {
                                        id_to_item_delegate_map_.end());
 }
 
+void ShelfItemDelegateManager::AddObserver(
+    ShelfItemDelegateManagerObserver* observer) {
+  observers_.AddObserver(observer);
+}
+
+void ShelfItemDelegateManager::RemoveObserver(
+    ShelfItemDelegateManagerObserver* observer) {
+  observers_.RemoveObserver(observer);
+}
+
 void ShelfItemDelegateManager::SetShelfItemDelegate(
     ShelfID id,
     scoped_ptr<ShelfItemDelegate> item_delegate) {
@@ -31,6 +41,11 @@ void ShelfItemDelegateManager::SetShelfItemDelegate(
   // that this request is replacing ShelfItemDelegate for |id| with
   // |item_delegate|.
   RemoveShelfItemDelegate(id);
+
+  FOR_EACH_OBSERVER(ShelfItemDelegateManagerObserver,
+                    observers_,
+                    OnSetShelfItemDelegate(id, item_delegate.get()));
+
   id_to_item_delegate_map_[id] = item_delegate.release();
 }
 
@@ -48,6 +63,9 @@ void ShelfItemDelegateManager::ShelfItemAdded(int index) {
 
 void ShelfItemDelegateManager::ShelfItemRemoved(int index, ShelfID id) {
   RemoveShelfItemDelegate(id);
+  FOR_EACH_OBSERVER(ShelfItemDelegateManagerObserver,
+                    observers_,
+                    OnSetShelfItemDelegate(id, nullptr));
 }
 
 void ShelfItemDelegateManager::ShelfItemMoved(int start_index,
diff --git a/ash/shelf/shelf_item_delegate_manager.h b/ash/shelf/shelf_item_delegate_manager.h
index 95f8f8a..d2e77b9 100644
--- a/ash/shelf/shelf_item_delegate_manager.h
+++ b/ash/shelf/shelf_item_delegate_manager.h
@@ -12,6 +12,7 @@
 #include "ash/shelf/shelf_model_observer.h"
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/observer_list.h"
 
 namespace ash {
 class ShelfItemDelegate;
@@ -21,6 +22,19 @@ namespace test {
 class ShelfItemDelegateManagerTestAPI;
 }
 
+// This class is added as a temporary fix for M39 to fix crbug.com/329597.
+// TODO(skuhne): Find the real reason for this problem and remove this fix.
+class ASH_EXPORT ShelfItemDelegateManagerObserver {
+ public:
+  ShelfItemDelegateManagerObserver() {}
+  virtual ~ShelfItemDelegateManagerObserver() {}
+
+  // Gets called when a ShelfItemDelegate gets changed. Note that
+  // |item_delegate| can be NULL.
+  virtual void OnSetShelfItemDelegate(ShelfID id,
+                                      ShelfItemDelegate* item_delegate) = 0;
+};
+
 // ShelfItemDelegateManager manages the set of ShelfItemDelegates for the
 // launcher. ShelfItemDelegateManager does not create ShelfItemDelegates,
 // rather it is expected that someone else invokes SetShelfItemDelegate
@@ -31,6 +45,9 @@ class ASH_EXPORT ShelfItemDelegateManager : public ShelfModelObserver {
   explicit ShelfItemDelegateManager(ShelfModel* model);
   ~ShelfItemDelegateManager() override;
 
+  void AddObserver(ShelfItemDelegateManagerObserver* observer);
+  void RemoveObserver(ShelfItemDelegateManagerObserver* observer);
+
   // Set |item_delegate| for |id| and take an ownership.
   void SetShelfItemDelegate(ShelfID id,
                             scoped_ptr<ShelfItemDelegate> item_delegate);
@@ -58,6 +75,8 @@ class ASH_EXPORT ShelfItemDelegateManager : public ShelfModelObserver {
 
   ShelfIDToItemDelegateMap id_to_item_delegate_map_;
 
+  ObserverList<ShelfItemDelegateManagerObserver> observers_;
+
   DISALLOW_COPY_AND_ASSIGN(ShelfItemDelegateManager);
 };
 
diff --git a/chrome/browser/ui/ash/launcher/browser_status_monitor.cc b/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
index 2178f9b..08f96c0 100644
--- a/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
+++ b/chrome/browser/ui/ash/launcher/browser_status_monitor.cc
@@ -130,6 +130,9 @@ BrowserStatusMonitor::~BrowserStatusMonitor() {
   if (ash::Shell::HasInstance())
     ash::Shell::GetInstance()->GetScreen()->RemoveObserver(this);
 
+  chrome::SettingsWindowManager::GetInstance()->RemoveObserver(
+      settings_window_observer_.get());
+
   BrowserList::RemoveObserver(this);
 
   BrowserList* browser_list =
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
index 86ef0d2..0d81c8f 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
@@ -421,12 +421,20 @@ ChromeLauncherController::ChromeLauncherController(Profile* profile,
   // TODO(mukai): Allows it to observe display change and write tests.
   if (ash::Shell::HasInstance()) {
     ash::Shell::GetInstance()->display_controller()->AddObserver(this);
+    // If it got already set, we remove the observer first again and swap the
+    // ItemDelegateManager.
+    if (item_delegate_manager_)
+      item_delegate_manager_->RemoveObserver(this);
     item_delegate_manager_ =
         ash::Shell::GetInstance()->shelf_item_delegate_manager();
+    item_delegate_manager_->AddObserver(this);
   }
 }
 
 ChromeLauncherController::~ChromeLauncherController() {
+  if (item_delegate_manager_)
+    item_delegate_manager_->RemoveObserver(this);
+
   // Reset the BrowserStatusMonitor as it has a weak pointer to this.
   browser_status_monitor_.reset();
 
@@ -576,9 +584,9 @@ void ChromeLauncherController::Pin(ash::ShelfID id) {
 }
 
 void ChromeLauncherController::Unpin(ash::ShelfID id) {
-  DCHECK(HasItemController(id));
+  LauncherItemController* controller = GetLauncherItemController(id);
+  CHECK(controller);
 
-  LauncherItemController* controller = id_to_item_controller_map_[id];
   if (controller->type() == LauncherItemController::TYPE_APP ||
       controller->locked()) {
     UnpinRunningAppInternal(model_->ItemIndexByID(id));
@@ -620,14 +628,14 @@ bool ChromeLauncherController::IsPinnable(ash::ShelfID id) const {
 }
 
 void ChromeLauncherController::Install(ash::ShelfID id) {
-  if (!HasItemController(id))
+  LauncherItemController* controller = GetLauncherItemController(id);
+  if (!controller)
     return;
 
   std::string app_id = GetAppIDForShelfID(id);
   if (extensions::util::IsExtensionInstalledPermanently(app_id, profile_))
     return;
 
-  LauncherItemController* controller = id_to_item_controller_map_[id];
   if (controller->type() == LauncherItemController::TYPE_APP) {
     AppWindowLauncherItemController* app_window_controller =
         static_cast<AppWindowLauncherItemController*>(controller);
@@ -647,8 +655,7 @@ bool ChromeLauncherController::CanInstall(ash::ShelfID id) {
   return extensions::util::IsEphemeralApp(GetAppIDForShelfID(id), profile_);
 }
 
-void ChromeLauncherController::LockV1AppWithID(
-    const std::string& app_id) {
+void ChromeLauncherController::LockV1AppWithID(const std::string& app_id) {
   ash::ShelfID id = GetShelfIDForAppID(app_id);
   if (!IsPinned(id) && !IsWindowedAppInLauncher(app_id)) {
     CreateAppShortcutLauncherItemWithType(app_id,
@@ -662,8 +669,8 @@ void ChromeLauncherController::LockV1AppWithID(
 
 void ChromeLauncherController::UnlockV1AppWithID(const std::string& app_id) {
   ash::ShelfID id = GetShelfIDForAppID(app_id);
-  CHECK(IsPinned(id) || IsWindowedAppInLauncher(app_id));
   CHECK(id);
+  CHECK(IsPinned(id) || IsWindowedAppInLauncher(app_id));
   LauncherItemController* controller = id_to_item_controller_map_[id];
   controller->unlock();
   if (!controller->locked() && !IsPinned(id))
@@ -671,21 +678,24 @@ void ChromeLauncherController::UnlockV1AppWithID(const std::string& app_id) {
 }
 
 void ChromeLauncherController::Launch(ash::ShelfID id, int event_flags) {
-  if (!HasItemController(id))
+  LauncherItemController* controller = GetLauncherItemController(id);
+  if (!controller)
     return;  // In case invoked from menu and item closed while menu up.
-  id_to_item_controller_map_[id]->Launch(ash::LAUNCH_FROM_UNKNOWN, event_flags);
+  controller->Launch(ash::LAUNCH_FROM_UNKNOWN, event_flags);
 }
 
 void ChromeLauncherController::Close(ash::ShelfID id) {
-  if (!HasItemController(id))
+  LauncherItemController* controller = GetLauncherItemController(id);
+  if (!controller)
     return;  // May happen if menu closed.
-  id_to_item_controller_map_[id]->Close();
+  controller->Close();
 }
 
 bool ChromeLauncherController::IsOpen(ash::ShelfID id) {
-  if (!HasItemController(id))
+  LauncherItemController* controller = GetLauncherItemController(id);
+  if (!controller)
     return false;
-  return id_to_item_controller_map_[id]->IsOpen();
+  return controller->IsOpen();
 }
 
 bool ChromeLauncherController::IsPlatformApp(ash::ShelfID id) {
@@ -752,7 +762,7 @@ void ChromeLauncherController::ActivateApp(const std::string& app_id,
   // If there is an existing non-shortcut controller for this app, open it.
   ash::ShelfID id = GetShelfIDForAppID(app_id);
   if (id) {
-    LauncherItemController* controller = id_to_item_controller_map_[id];
+    LauncherItemController* controller = GetLauncherItemController(id);
     controller->Activate(source);
     return;
   }
@@ -769,10 +779,7 @@ void ChromeLauncherController::ActivateApp(const std::string& app_id,
 
 extensions::LaunchType ChromeLauncherController::GetLaunchType(
     ash::ShelfID id) {
-  DCHECK(HasItemController(id));
-
-  const Extension* extension = GetExtensionForAppID(
-      id_to_item_controller_map_[id]->app_id());
+  const Extension* extension = GetExtensionForAppID(GetAppIDForShelfID(id));
 
   // An extension can be unloaded/updated/unavailable at any time.
   if (!extension)
@@ -797,8 +804,9 @@ ash::ShelfID ChromeLauncherController::GetShelfIDForAppID(
 
 const std::string& ChromeLauncherController::GetAppIDForShelfID(
     ash::ShelfID id) {
-  CHECK(HasItemController(id));
-  return id_to_item_controller_map_[id]->app_id();
+  LauncherItemController* controller = GetLauncherItemController(id);
+  CHECK(controller);
+  return controller->app_id();
 }
 
 void ChromeLauncherController::SetAppImage(const std::string& id,
@@ -875,12 +883,13 @@ void ChromeLauncherController::PinAppWithID(const std::string& app_id) {
 void ChromeLauncherController::SetLaunchType(
     ash::ShelfID id,
     extensions::LaunchType launch_type) {
-  if (!HasItemController(id))
+  LauncherItemController* controller = GetLauncherItemController(id);
+  if (!controller)
     return;
 
   extensions::SetLaunchType(
       extensions::ExtensionSystem::Get(profile_)->extension_service(),
-      id_to_item_controller_map_[id]->app_id(),
+      controller->app_id(),
       launch_type);
 }
 
@@ -891,6 +900,18 @@ void ChromeLauncherController::UnpinAppWithID(const std::string& app_id) {
     NOTREACHED();
 }
 
+void ChromeLauncherController::OnSetShelfItemDelegate(
+    ash::ShelfID id,
+    ash::ShelfItemDelegate* item_delegate) {
+  // TODO(skuhne): This fixes crbug.com/429870, but it does not answer why we
+  // get into this state in the first place.
+  IDToItemControllerMap::iterator iter = id_to_item_controller_map_.find(id);
+  if (iter == id_to_item_controller_map_.end() || item_delegate == iter->second)
+    return;
+  LOG(ERROR) << "Unexpected change of shelf item id: " << id;
+  id_to_item_controller_map_.erase(iter);
+}
+
 bool ChromeLauncherController::IsLoggedInAsGuest() {
   return profile_->IsGuestSession();
 }
@@ -926,9 +947,10 @@ void ChromeLauncherController::PersistPinnedState() {
     for (size_t i = 0; i < model_->items().size(); ++i) {
       if (model_->items()[i].type == ash::TYPE_APP_SHORTCUT) {
         ash::ShelfID id = model_->items()[i].id;
-        if (HasItemController(id) && IsPinned(id)) {
+        LauncherItemController* controller = GetLauncherItemController(id);
+        if (controller && IsPinned(id)) {
           base::DictionaryValue* app_value = ash::CreateAppDict(
-              id_to_item_controller_map_[id]->app_id());
+              controller->app_id());
           if (app_value)
             updater->Append(app_value);
         }
@@ -1035,8 +1057,8 @@ ash::ShelfID ChromeLauncherController::GetShelfIDForWebContents(
 
 void ChromeLauncherController::SetRefocusURLPatternForTest(ash::ShelfID id,
                                                            const GURL& url) {
-  DCHECK(HasItemController(id));
-  LauncherItemController* controller = id_to_item_controller_map_[id];
+  LauncherItemController* controller = GetLauncherItemController(id);
+  DCHECK(controller);
 
   int index = model_->ItemIndexByID(id);
   if (index == -1) {
@@ -1115,6 +1137,15 @@ void ChromeLauncherController::ShelfItemAdded(int index) {
 }
 
 void ChromeLauncherController::ShelfItemRemoved(int index, ash::ShelfID id) {
+  // TODO(skuhne): This fixes crbug.com/429870, but it does not answer why we
+  // get into this state in the first place.
+  IDToItemControllerMap::iterator iter = id_to_item_controller_map_.find(id);
+  if (iter == id_to_item_controller_map_.end())
+    return;
+
+  LOG(ERROR) << "Unexpected change of shelf item id: " << id;
+
+  id_to_item_controller_map_.erase(iter);
 }
 
 void ChromeLauncherController::ShelfItemMoved(int start_index,
@@ -1274,11 +1305,11 @@ ChromeLauncherAppMenuItems ChromeLauncherController::GetApplicationList(
     int event_flags) {
   // Make sure that there is a controller associated with the id and that the
   // extension itself is a valid application and not a panel.
-  if (!HasItemController(item.id) ||
-      !GetShelfIDForAppID(id_to_item_controller_map_[item.id]->app_id()))
+  LauncherItemController* controller = GetLauncherItemController(item.id);
+  if (!controller || !GetShelfIDForAppID(controller->app_id()))
     return ChromeLauncherAppMenuItems().Pass();
 
-  return id_to_item_controller_map_[item.id]->GetApplicationList(event_flags);
+  return controller->GetApplicationList(event_flags);
 }
 
 std::vector<content::WebContents*>
@@ -1287,7 +1318,7 @@ ChromeLauncherController::GetV1ApplicationsFromAppId(std::string app_id) {
 
   // If there is no such an item pinned to the launcher, no menu gets created.
   if (id) {
-    LauncherItemController* controller = id_to_item_controller_map_[id];
+    LauncherItemController* controller = GetLauncherItemController(id);
     DCHECK(controller);
     if (controller->type() == LauncherItemController::TYPE_SHORTCUT)
       return GetV1ApplicationsFromController(controller);
@@ -1299,8 +1330,8 @@ void ChromeLauncherController::ActivateShellApp(const std::string& app_id,
                                                 int index) {
   ash::ShelfID id = GetShelfIDForAppID(app_id);
   if (id) {
-    LauncherItemController* controller = id_to_item_controller_map_[id];
-    if (controller->type() == LauncherItemController::TYPE_APP) {
+    LauncherItemController* controller = GetLauncherItemController(id);
+    if (controller && controller->type() == LauncherItemController::TYPE_APP) {
       AppWindowLauncherItemController* app_window_controller =
           static_cast<AppWindowLauncherItemController*>(controller);
       app_window_controller->ActivateIndexedApp(index);
@@ -1388,7 +1419,13 @@ const std::string& ChromeLauncherController::GetAppIdFromShelfIdForTest(
 
 void ChromeLauncherController::SetShelfItemDelegateManagerForTest(
     ash::ShelfItemDelegateManager* manager) {
+  if (item_delegate_manager_)
+    item_delegate_manager_->RemoveObserver(this);
+
   item_delegate_manager_ = manager;
+
+  if (item_delegate_manager_)
+    item_delegate_manager_->AddObserver(this);
 }
 
 void ChromeLauncherController::RememberUnpinnedRunningApplicationOrder() {
@@ -1534,9 +1571,8 @@ void ChromeLauncherController::UnpinRunningAppInternal(int index) {
   item.type = ash::TYPE_WINDOWED_APP;
   // A platform app and a windowed app are sharing TYPE_APP_SHORTCUT. As such
   // we have to check here what this was before it got a shortcut.
-  if (HasItemController(item.id) &&
-      id_to_item_controller_map_[item.id]->type() ==
-          LauncherItemController::TYPE_APP)
+  LauncherItemController* controller = GetLauncherItemController(item.id);
+  if (controller && controller->type() == LauncherItemController::TYPE_APP)
     item.type = ash::TYPE_PLATFORM_APP;
   model_->Set(index, item);
 }
@@ -1575,12 +1611,10 @@ void ChromeLauncherController::UpdateAppLaunchersFromPref() {
         bool is_chrome = item.type == ash::TYPE_BROWSER_SHORTCUT;
         if (item.type != ash::TYPE_APP_SHORTCUT && !is_app_list && !is_chrome)
           continue;
-        IDToItemControllerMap::const_iterator entry =
-            id_to_item_controller_map_.find(item.id);
+        LauncherItemController* controller = GetLauncherItemController(item.id);
         if ((kAppShelfIdPlaceholder == *pref_app_id && is_app_list) ||
             (extension_misc::kChromeAppId == *pref_app_id && is_chrome) ||
-            (entry != id_to_item_controller_map_.end() &&
-             entry->second->app_id() == *pref_app_id)) {
+            (controller && controller->app_id() == *pref_app_id)) {
           // Check if an item needs to be moved here.
           MoveChromeOrApplistToFinalPosition(
               is_chrome, is_app_list, index, &chrome_index, &app_list_index);
@@ -1603,15 +1637,16 @@ void ChromeLauncherController::UpdateAppLaunchersFromPref() {
           } else {
             // Check if this is a platform or a windowed app.
             if (item.type == ash::TYPE_APP_SHORTCUT &&
-                (id_to_item_controller_map_[item.id]->locked() ||
-                 id_to_item_controller_map_[item.id]->type() ==
-                     LauncherItemController::TYPE_APP)) {
+                controller &&
+                (controller->locked() ||
+                 controller->type() == LauncherItemController::TYPE_APP)) {
               // Note: This will not change the amount of items (|max_index|).
               // Even changes to the actual |index| due to item weighting
               // changes should be fine.
               UnpinRunningAppInternal(index);
             } else {
-              LauncherItemClosed(item.id);
+              if (controller)
+                LauncherItemClosed(item.id);
               --max_index;
             }
           }
@@ -1641,12 +1676,15 @@ void ChromeLauncherController::UpdateAppLaunchersFromPref() {
   while (index < model_->item_count()) {
     const ash::ShelfItem& item(model_->items()[index]);
     if (item.type == ash::TYPE_APP_SHORTCUT) {
-      if (id_to_item_controller_map_[item.id]->locked() ||
-          id_to_item_controller_map_[item.id]->type() ==
-              LauncherItemController::TYPE_APP)
-        UnpinRunningAppInternal(index);
-      else
-        LauncherItemClosed(item.id);
+      LauncherItemController* controller = GetLauncherItemController(item.id);
+      if (controller) {
+        if (controller->locked() ||
+            controller->type() == LauncherItemController::TYPE_APP) {
+          UnpinRunningAppInternal(index);
+        } else {
+          LauncherItemClosed(item.id);
+        }
+      }
     } else {
       if (item.type == ash::TYPE_BROWSER_SHORTCUT)
         chrome_index = index;
@@ -1840,12 +1878,8 @@ ChromeLauncherController::GetBrowserShortcutLauncherItemController() {
     if (item.type == ash::TYPE_BROWSER_SHORTCUT)
       return static_cast<BrowserShortcutLauncherItemController*>(i->second);
   }
-  // Create a LauncherItemController for the Browser shortcut if it does not
-  // exist yet.
-  ash::ShelfID id = CreateBrowserShortcutLauncherItem();
-  DCHECK(id_to_item_controller_map_[id]);
-  return static_cast<BrowserShortcutLauncherItemController*>(
-      id_to_item_controller_map_[id]);
+  NOTREACHED() << "There should be always a BrowserLauncherItemController.";
+  return nullptr;
 }
 
 ash::ShelfID ChromeLauncherController::CreateBrowserShortcutLauncherItem() {
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.h b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.h
index e3b234e..94d762d 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller.h
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller.h
@@ -14,6 +14,7 @@
 #include "ash/display/display_controller.h"
 #include "ash/shelf/shelf_delegate.h"
 #include "ash/shelf/shelf_item_delegate.h"
+#include "ash/shelf/shelf_item_delegate_manager.h"
 #include "ash/shelf/shelf_item_types.h"
 #include "ash/shelf/shelf_layout_manager_observer.h"
 #include "ash/shelf/shelf_model_observer.h"
@@ -98,7 +99,8 @@ class ChromeLauncherController : public ash::ShelfDelegate,
                                  public PrefServiceSyncableObserver,
                                  public AppSyncUIStateObserver,
                                  public ExtensionEnableFlowDelegate,
-                                 public ash::ShelfLayoutManagerObserver {
+                                 public ash::ShelfLayoutManagerObserver,
+                                 public ash::ShelfItemDelegateManagerObserver {
  public:
   // Indicates if a shelf item is incognito or not.
   enum IncognitoState {
@@ -156,7 +158,9 @@ class ChromeLauncherController : public ash::ShelfDelegate,
   void SetItemStatus(ash::ShelfID id, ash::ShelfItemStatus status);
 
   // Updates the controller associated with id (which should be a shortcut).
-  // |controller| remains owned by caller.
+  // |controller| will be owned by the |ChromeLauncherController| and then
+  // passed on to |ShelfItemDelegateManager|.
+  // TODO(skuhne): Pass in scoped_ptr to make ownership clear.
   void SetItemController(ash::ShelfID id, LauncherItemController* controller);
 
   // Closes or unpins the shelf item.
@@ -290,7 +294,7 @@ class ChromeLauncherController : public ash::ShelfDelegate,
   void ActivateWindowOrMinimizeIfActive(ui::BaseWindow* window,
                                         bool allow_minimize);
 
-  // ash::ShelfDelegate overrides:
+  // ash::ShelfDelegate:
   void OnShelfCreated(ash::Shelf* shelf) override;
   void OnShelfDestroyed(ash::Shelf* shelf) override;
   ash::ShelfID GetShelfIDForAppID(const std::string& app_id) override;
@@ -300,20 +304,24 @@ class ChromeLauncherController : public ash::ShelfDelegate,
   bool CanPin() const override;
   void UnpinAppWithID(const std::string& app_id) override;
 
-  // ash::ShelfModelObserver overrides:
+  // ash::ShelfItemDelegateManagerObserver:
+  void OnSetShelfItemDelegate(ash::ShelfID id,
+                              ash::ShelfItemDelegate* item_delegate) override;
+
+  // ash::ShelfModelObserver:
   void ShelfItemAdded(int index) override;
   void ShelfItemRemoved(int index, ash::ShelfID id) override;
   void ShelfItemMoved(int start_index, int target_index) override;
   void ShelfItemChanged(int index, const ash::ShelfItem& old_item) override;
   void ShelfStatusChanged() override;
 
-  // ash::ShellObserver overrides:
+  // ash::ShellObserver:
   void OnShelfAlignmentChanged(aura::Window* root_window) override;
 
-  // ash::DisplayController::Observer overrides:
+  // ash::DisplayController::Observer:
   void OnDisplayConfigurationChanged() override;
 
-  // ExtensionRegistryObserver overrides:
+  // ExtensionRegistryObserver:
   void OnExtensionLoaded(content::BrowserContext* browser_context,
                          const extensions::Extension* extension) override;
   void OnExtensionUnloaded(
@@ -321,21 +329,21 @@ class ChromeLauncherController : public ash::ShelfDelegate,
       const extensions::Extension* extension,
       extensions::UnloadedExtensionInfo::Reason reason) override;
 
-  // PrefServiceSyncableObserver overrides:
+  // PrefServiceSyncableObserver:
   void OnIsSyncingChanged() override;
 
-  // AppSyncUIStateObserver overrides:
+  // AppSyncUIStateObserver:
   void OnAppSyncUIStatusChanged() override;
 
-  // ExtensionEnableFlowDelegate overrides:
+  // ExtensionEnableFlowDelegate:
   void ExtensionEnableFlowFinished() override;
   void ExtensionEnableFlowAborted(bool user_initiated) override;
 
-  // extensions::AppIconLoader overrides:
+  // extensions::AppIconLoader:
   void SetAppImage(const std::string& app_id,
                    const gfx::ImageSkia& image) override;
 
-  // ash::ShelfLayoutManagerObserver overrides:
+  // ash::ShelfLayoutManagerObserver:
   void OnAutoHideBehaviorChanged(
       aura::Window* root_window,
       ash::ShelfAutoHideBehavior new_behavior) override;
@@ -564,6 +572,7 @@ class ChromeLauncherController : public ash::ShelfDelegate,
   // profile new windows are created with.
   Profile* profile_;
 
+  // Controller items in this map are owned by |ShelfItemDelegateManager|.
   IDToItemControllerMap id_to_item_controller_map_;
 
   // Direct access to app_id for a web contents.
diff --git a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
index cb23247..c13cbc2 100644
--- a/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
+++ b/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
@@ -402,6 +402,7 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
   }
 
   void TearDown() override {
+    launcher_controller_->SetShelfItemDelegateManagerForTest(nullptr);
     if (!ash::Shell::HasInstance())
       delete item_delegate_manager_;
     model_->RemoveObserver(model_observer_.get());
@@ -428,9 +429,9 @@ class ChromeLauncherControllerTest : public BrowserWithTestWindowTest {
   }
 
   void InitLauncherControllerWithBrowser() {
+    InitLauncherController();
     chrome::NewTab(browser());
     BrowserList::SetLastActive(browser());
-    InitLauncherController();
   }
 
   void SetAppIconLoader(extensions::AppIconLoader* loader) {
@@ -2595,6 +2596,7 @@ TEST_F(ChromeLauncherControllerTest, PersistLauncherItemPositions) {
   EXPECT_EQ(ash::TYPE_APP_SHORTCUT, model_->items()[2].type);
   EXPECT_EQ(ash::TYPE_BROWSER_SHORTCUT, model_->items()[3].type);
 
+  SetShelfItemDelegateManager(nullptr);
   launcher_controller_.reset();
   if (!ash::Shell::HasInstance()) {
     delete item_delegate_manager_;
@@ -2650,6 +2652,7 @@ TEST_F(ChromeLauncherControllerTest, PersistPinned) {
   EXPECT_FALSE(launcher_controller_->IsAppPinned("0"));
   EXPECT_EQ(initial_size + 1, model_->items().size());
 
+  SetShelfItemDelegateManager(nullptr);
   launcher_controller_.reset();
   if (!ash::Shell::HasInstance()) {
     delete item_delegate_manager_;