Filter pinned extensions as actions change

This filters extensions when they are added/removed from the
ToolbarActionsModel. This happens not only on uninstall but also during
reload which is why we need to re-filter on adding extensions.

This leaves a TODO to actually remove pinned-extension IDs when
uninstalling an extension to keep traces of previously-installed
extensions.

For now it'll address a crash where entries in pinned_action_ids() could
refer to extensions that were no longer installed, resulting in a
null-pointer dereference when trying to update their position in the
toolbar.

Bug: chromium:1040856
Change-Id: I3928e931c12d6571c8eb5ad9bd255f0602cfa3d3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2005373
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Caroline Rising <corising@chromium.org>
Cr-Commit-Position: refs/heads/master@{#732706}

chrome/browser/ui/toolbar/toolbar_actions_model.cc

@@ -383,6 +383,8 @@ void ToolbarActionsModel::AddAction(const ActionId& action_id) {
     if (visible_count_delta)
       SetVisibleIconCount(visible_icon_count() + visible_count_delta);
   }
+
+  UpdatePinnedActionIds();
 }
 
 void ToolbarActionsModel::RemoveAction(const ActionId& action_id) {
@@ -397,6 +399,11 @@ void ToolbarActionsModel::RemoveAction(const ActionId& action_id) {
 
   action_ids_.erase(pos);
 
+  // TODO(pbos): Remove previously-pinned actions from ExtensionPrefs if this is
+  // an uninstall and not a disable. This might need to be handled in another
+  // place (ExtensionPrefs?) as we don't know the reason for RemoveAction here.
+  UpdatePinnedActionIds();
+
   // If we're in highlight mode, we also have to remove the action from
   // the highlighted list.
   if (is_highlighting()) {
@@ -485,8 +492,11 @@ void ToolbarActionsModel::InitializeActionList() {
   else
     Populate();
 
-  if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu))
+  if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu)) {
+    // Set |pinned_action_ids_| directly to avoid notifying observers that they
+    // have changed even though they haven't.
     pinned_action_ids_ = GetFilteredPinnedActionIds();
+  }
 }
 
 void ToolbarActionsModel::Populate() {
@@ -658,14 +668,7 @@ void ToolbarActionsModel::OnActionToolbarPrefChange() {
   if (!actions_initialized_)
     return;
 
-  if (base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu)) {
-    std::vector<ActionId> pinned_extensions = GetFilteredPinnedActionIds();
-    if (pinned_extensions != pinned_action_ids_) {
-      pinned_action_ids_ = pinned_extensions;
-      for (Observer& observer : observers_)
-        observer.OnToolbarPinnedActionsChanged();
-    }
-  }
+  UpdatePinnedActionIds();
 
   // Recalculate |last_known_positions_| to be |pref_positions| followed by
   // ones that are only in |last_known_positions_|.
@@ -784,6 +787,18 @@ bool ToolbarActionsModel::IsActionVisible(const ActionId& action_id) const {
   return index < visible_icon_count();
 }
 
+void ToolbarActionsModel::UpdatePinnedActionIds() {
+  if (!base::FeatureList::IsEnabled(features::kExtensionsToolbarMenu))
+    return;
+  std::vector<ActionId> pinned_extensions = GetFilteredPinnedActionIds();
+  if (pinned_extensions == pinned_action_ids_)
+    return;
+
+  pinned_action_ids_ = pinned_extensions;
+  for (Observer& observer : observers_)
+    observer.OnToolbarPinnedActionsChanged();
+}
+
 std::vector<ToolbarActionsModel::ActionId>
 ToolbarActionsModel::GetFilteredPinnedActionIds() const {
   // TODO(pbos): Make sure that the pinned IDs are pruned from ExtensionPrefs on

chrome/browser/ui/toolbar/toolbar_actions_model.h

@@ -266,6 +266,10 @@ class ToolbarActionsModel : public extensions::ExtensionActionAPI::Observer,
   // Returns true if the action is visible on the toolbar.
   bool IsActionVisible(const ActionId& action_id) const;
 
+  // Updates |pinned_action_ids_| per GetFilteredPinnedActionIds() and notifies
+  // observers if they have changed.
+  void UpdatePinnedActionIds();
+
   // Gets a list of pinned action ids that only contains that only contains IDs
   // with a corresponding action in the model.
   std::vector<ActionId> GetFilteredPinnedActionIds() const;

chrome/browser/ui/views/extensions/extensions_menu_view.cc

@@ -467,9 +467,8 @@ void ExtensionsMenuView::OnToolbarModelInitialized() {
 }
 
 void ExtensionsMenuView::OnToolbarPinnedActionsChanged() {
-  for (auto* menu_item : extensions_menu_items_) {
+  for (auto* menu_item : extensions_menu_items_)
     menu_item->UpdatePinButton();
-  }
 }
 
 // static