Fix crash in OnAndroidAppListRefreshed

The crash happened if the Android app list was updated while the
Assistant was stopped.
To solve this we now stop observing Android app list updates when the
Assistant is stopped.

Notes:
   - I also renamed |AddAppListEventSubscriber| to
     |AddAndFireAppListEventSubscriber| to better describe its behavior.
   - I also changed |ScopedObserver| to |ScopedObservation|, as
     |ScopedObserver| is deprecated.
   - I looked into adding unittests for this but decided against it as
     it won't be trivial and the code will be reworked in the near
     future during the Libassistant V2 migration.

Bug: 1136387
Test: chromeos_unittests
Change-Id: Icf536c9aad54af3e9ab306464c2a47075076e60d
Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2600448Reviewed-by: Yue Li <updowndota@chromium.org>
Reviewed-by: Meilin Wang <meilinw@chromium.org>
Commit-Queue: Yue Li <updowndota@chromium.org>
Commit-Queue: Jeroen Dhollander <jeroendh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839486}

chrome/browser/ui/ash/assistant/device_actions.cc

@@ -208,7 +208,7 @@ void DeviceActions::LaunchAndroidIntent(const std::string& intent) {
   app->LaunchIntent(intent, display::kDefaultDisplayId);
 }
 
-void DeviceActions::AddAppListEventSubscriber(
+void DeviceActions::AddAndFireAppListEventSubscriber(
     chromeos::assistant::AppListEventSubscriber* subscriber) {
   auto* prefs = ArcAppListPrefs::Get(ProfileManager::GetActiveUserProfile());
   if (prefs && prefs->package_list_initial_refreshed()) {

chrome/browser/ui/ash/assistant/device_actions.h

@@ -39,7 +39,7 @@ class DeviceActions : public ash::AndroidIntentHelper,
   chromeos::assistant::AppStatus GetAndroidAppStatus(
       const chromeos::assistant::AndroidAppInfo& app_info) override;
   void LaunchAndroidIntent(const std::string& intent) override;
-  void AddAppListEventSubscriber(
+  void AddAndFireAppListEventSubscriber(
       chromeos::assistant::AppListEventSubscriber* subscriber) override;
   void RemoveAppListEventSubscriber(
       chromeos::assistant::AppListEventSubscriber* subscriber) override;

chromeos/services/assistant/assistant_device_settings_delegate_unittest.cc

@@ -65,7 +65,7 @@ class ScopedDeviceActionsMock : public ScopedDeviceActions {
               (const chromeos::assistant::AndroidAppInfo& app_info));
   MOCK_METHOD(void, LaunchAndroidIntent, (const std::string& intent));
   MOCK_METHOD(void,
-              AddAppListEventSubscriber,
+              AddAndFireAppListEventSubscriber,
               (chromeos::assistant::AppListEventSubscriber * subscriber));
   MOCK_METHOD(void,
               RemoveAppListEventSubscriber,

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -253,7 +253,7 @@ void AssistantManagerServiceImpl::Stop() {
     assistant_manager()->ResetAllDataAndShutdown();
 
   media_controller_observer_receiver_.reset();
-
+  scoped_app_list_event_subscriber_.Reset();
   service_controller().Stop();
 }
 
@@ -1023,10 +1023,8 @@ void AssistantManagerServiceImpl::PostInitAssistant() {
 
   assistant_settings_->UpdateServerDeviceSettings();
 
-  if (base::FeatureList::IsEnabled(assistant::features::kAssistantAppSupport) &&
-      !scoped_app_list_event_subscriber_.IsObservingSources()) {
-    scoped_app_list_event_subscriber_.Add(device_actions());
-  }
+  if (base::FeatureList::IsEnabled(assistant::features::kAssistantAppSupport))
+    scoped_app_list_event_subscriber_.Observe(device_actions());
 }
 
 bool AssistantManagerServiceImpl::IsServiceStarted() const {

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -14,7 +14,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "base/observer_list.h"
 #include "base/optional.h"
-#include "base/scoped_observer.h"
+#include "base/scoped_observation.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread.h"
 #include "chromeos/assistant/internal/action/cros_action_module.h"
@@ -353,10 +353,10 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
   // Configuration passed to libassistant.
   std::string libassistant_config_;
 
-  ScopedObserver<DeviceActions,
-                 AppListEventSubscriber,
-                 &DeviceActions::AddAppListEventSubscriber,
-                 &DeviceActions::RemoveAppListEventSubscriber>
+  base::ScopedObservation<DeviceActions,
+                          AppListEventSubscriber,
+                          &DeviceActions::AddAndFireAppListEventSubscriber,
+                          &DeviceActions::RemoveAppListEventSubscriber>
       scoped_app_list_event_subscriber_{this};
   base::ObserverList<CommunicationErrorObserver> error_observers_;
   base::ObserverList<StateObserver> state_observers_;

chromeos/services/assistant/public/cpp/device_actions.h

@@ -70,8 +70,9 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE_PUBLIC) DeviceActions {
   // See Intent.toUri().
   virtual void LaunchAndroidIntent(const std::string& intent) = 0;
 
-  // Register App list event subscriber.
-  virtual void AddAppListEventSubscriber(
+  // Register App list event subscriber. The subscriber will be immediately
+  // called with the current App list, and then for every change.
+  virtual void AddAndFireAppListEventSubscriber(
       AppListEventSubscriber* subscriber) = 0;
   virtual void RemoveAppListEventSubscriber(
       AppListEventSubscriber* subscriber) = 0;

chromeos/services/assistant/test_support/scoped_device_actions.h

@@ -29,7 +29,8 @@ class ScopedDeviceActions : DeviceActions {
   bool OpenAndroidApp(const AndroidAppInfo& app_info) override;
   AppStatus GetAndroidAppStatus(const AndroidAppInfo& app_info) override;
   void LaunchAndroidIntent(const std::string& intent) override {}
-  void AddAppListEventSubscriber(AppListEventSubscriber* subscriber) override {}
+  void AddAndFireAppListEventSubscriber(
+      AppListEventSubscriber* subscriber) override {}
   void RemoveAppListEventSubscriber(
       AppListEventSubscriber* subscriber) override {}