Messages: Wait for app registry to load before querying for PWA info.

Recent changes to the apps framework caused a delay in loading
app information during login.  This requires that we wait for
WebAppProvider::on_registry_ready() before querying the apps
database for information on installed PWAs.  This issue was resulting
in the Messages feature getting disabled after the app was perceived to
be uninstalled.

Bug: 1131140
Change-Id: Ie2448fed1d04a9525b5dd5a452cb0b000a29ce0b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429527
Commit-Queue: Jon Mann <jonmann@chromium.org>
Reviewed-by: Azeem Arshad <azeemarshad@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#811565}

chrome/browser/chromeos/android_sms/android_sms_app_manager_impl.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/chromeos/android_sms/android_sms_urls.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service.h"
+#include "chrome/browser/web_applications/web_app_provider.h"
 #include "chromeos/components/multidevice/logging/logging.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
@@ -65,23 +66,42 @@ bool AndroidSmsAppManagerImpl::PwaDelegate::TransferItemAttributes(
                                                            to_app_id);
 }
 
+bool AndroidSmsAppManagerImpl::PwaDelegate::IsAppRegistryReady(
+    Profile* profile) {
+  auto* provider = web_app::WebAppProvider::Get(profile);
+  if (!provider)
+    return false;
+
+  return provider->on_registry_ready().is_signaled();
+}
+
+void AndroidSmsAppManagerImpl::PwaDelegate::ExecuteOnAppRegistryReady(
+    Profile* profile,
+    base::OnceClosure task) {
+  auto* provider = web_app::WebAppProvider::Get(profile);
+  if (!provider)
+    return;
+
+  provider->on_registry_ready().Post(FROM_HERE, std::move(task));
+}
+
 AndroidSmsAppManagerImpl::AndroidSmsAppManagerImpl(
     Profile* profile,
     AndroidSmsAppSetupController* setup_controller,
     PrefService* pref_service,
     app_list::AppListSyncableService* app_list_syncable_service,
-    scoped_refptr<base::TaskRunner> task_runner)
+    std::unique_ptr<PwaDelegate> test_pwa_delegate)
     : profile_(profile),
       setup_controller_(setup_controller),
       app_list_syncable_service_(app_list_syncable_service),
-      pref_service_(pref_service),
-      installed_url_at_last_notify_(GetCurrentAppUrl()),
-      pwa_delegate_(std::make_unique<PwaDelegate>()) {
+      pref_service_(pref_service) {
+  pwa_delegate_ = test_pwa_delegate ? std::move(test_pwa_delegate)
+                                    : std::make_unique<PwaDelegate>();
   // Post a task to complete initialization. This portion of the flow must be
-  // posted asynchronously because it accesses the networking stack, which is
-  // not completely loaded until after this class is instantiated.
-  task_runner->PostTask(
-      FROM_HERE,
+  // posted asynchronously because it accesses the networking stack and apps
+  // registry, which might not be loaded until later.
+  pwa_delegate_->ExecuteOnAppRegistryReady(
+      profile_,
       base::BindOnce(&AndroidSmsAppManagerImpl::CompleteAsyncInitialization,
                      weak_ptr_factory_.GetWeakPtr()));
 }
@@ -140,6 +160,15 @@ bool AndroidSmsAppManagerImpl::HasAppBeenManuallyUninstalledByUser() {
          !setup_controller_->GetPwa(url);
 }
 
+bool AndroidSmsAppManagerImpl::IsAppRegistryReady() {
+  return pwa_delegate_->IsAppRegistryReady(profile_);
+}
+
+void AndroidSmsAppManagerImpl::ExecuteOnAppRegistryReady(
+    base::OnceClosure task) {
+  pwa_delegate_->ExecuteOnAppRegistryReady(profile_, std::move(task));
+}
+
 base::Optional<PwaDomain> AndroidSmsAppManagerImpl::GetInstalledPwaDomain() {
   for (auto* it = std::begin(kDomains); it != std::end(kDomains); ++it) {
     if (setup_controller_->GetPwa(
@@ -152,6 +181,9 @@ base::Optional<PwaDomain> AndroidSmsAppManagerImpl::GetInstalledPwaDomain() {
 }
 
 void AndroidSmsAppManagerImpl::CompleteAsyncInitialization() {
+  // Must wait until the app registry is ready before querying the current url.
+  last_installed_url_ = GetCurrentAppUrl();
+
   base::Optional<PwaDomain> domain = GetInstalledPwaDomain();
 
   // If no app was installed before this object was created, there is nothing
@@ -173,10 +205,10 @@ void AndroidSmsAppManagerImpl::CompleteAsyncInitialization() {
 
 void AndroidSmsAppManagerImpl::NotifyInstalledAppUrlChangedIfNecessary() {
   base::Optional<GURL> installed_app_url = GetCurrentAppUrl();
-  if (installed_url_at_last_notify_ == installed_app_url)
+  if (last_installed_url_ == installed_app_url)
     return;
 
-  installed_url_at_last_notify_ = installed_app_url;
+  last_installed_url_ = installed_app_url;
   NotifyInstalledAppUrlChanged();
 }
 
@@ -265,11 +297,6 @@ void AndroidSmsAppManagerImpl::HandleAppSetupFinished() {
                              true /* use_install_url */, *domain)));
 }
 
-void AndroidSmsAppManagerImpl::SetPwaDelegateForTesting(
-    std::unique_ptr<PwaDelegate> test_pwa_delegate) {
-  pwa_delegate_ = std::move(test_pwa_delegate);
-}
-
 }  // namespace android_sms
 
 }  // namespace chromeos

chrome/browser/chromeos/android_sms/android_sms_app_manager_impl.h

@@ -35,19 +35,6 @@ enum class PwaDomain;
 
 class AndroidSmsAppManagerImpl : public AndroidSmsAppManager {
  public:
-  AndroidSmsAppManagerImpl(
-      Profile* profile,
-      AndroidSmsAppSetupController* setup_controller,
-      PrefService* pref_service,
-      app_list::AppListSyncableService* app_list_syncable_service,
-      scoped_refptr<base::TaskRunner> task_runner =
-          base::ThreadTaskRunnerHandle::Get());
-  ~AndroidSmsAppManagerImpl() override;
-  static void RegisterProfilePrefs(PrefRegistrySimple* registry);
-
- private:
-  friend class AndroidSmsAppManagerImplTest;
-
   // Thin wrapper around static PWA functions which is stubbed out for tests.
   class PwaDelegate {
    public:
@@ -58,8 +45,24 @@ class AndroidSmsAppManagerImpl : public AndroidSmsAppManager {
         const std::string& from_app_id,
         const std::string& to_app_id,
         app_list::AppListSyncableService* app_list_syncable_service);
+    virtual bool IsAppRegistryReady(Profile* profile);
+    virtual void ExecuteOnAppRegistryReady(Profile* profile,
+                                           base::OnceClosure task);
   };
 
+  AndroidSmsAppManagerImpl(
+      Profile* profile,
+      AndroidSmsAppSetupController* setup_controller,
+      PrefService* pref_service,
+      app_list::AppListSyncableService* app_list_syncable_service,
+      std::unique_ptr<PwaDelegate> test_pwa_delegate = nullptr);
+  ~AndroidSmsAppManagerImpl() override;
+
+  static void RegisterProfilePrefs(PrefRegistrySimple* registry);
+
+ private:
+  friend class AndroidSmsAppManagerImplTest;
+
   // AndroidSmsAppManager:
   base::Optional<GURL> GetCurrentAppUrl() override;
 
@@ -68,6 +71,8 @@ class AndroidSmsAppManagerImpl : public AndroidSmsAppManager {
   void SetUpAndLaunchAndroidSmsApp() override;
   void TearDownAndroidSmsApp() override;
   bool HasAppBeenManuallyUninstalledByUser() override;
+  bool IsAppRegistryReady() override;
+  void ExecuteOnAppRegistryReady(base::OnceClosure task) override;
 
   base::Optional<PwaDomain> GetInstalledPwaDomain();
   void CompleteAsyncInitialization();
@@ -79,8 +84,6 @@ class AndroidSmsAppManagerImpl : public AndroidSmsAppManager {
                             bool success);
   void HandleAppSetupFinished();
 
-  void SetPwaDelegateForTesting(std::unique_ptr<PwaDelegate> test_pwa_delegate);
-
   Profile* profile_;
   AndroidSmsAppSetupController* setup_controller_;
   app_list::AppListSyncableService* app_list_syncable_service_;
@@ -93,9 +96,9 @@ class AndroidSmsAppManagerImpl : public AndroidSmsAppManager {
   // successfully.
   bool is_app_launch_pending_ = false;
 
-  // The installed app URL during the last time that
-  // NotifyInstalledAppUrlChanged() was invoked.
-  base::Optional<GURL> installed_url_at_last_notify_;
+  // The installed app URL, initialized when app registry is ready and updated
+  // any time NotifyInstalledAppUrlChanged() is invoked.
+  base::Optional<GURL> last_installed_url_;
 
   std::unique_ptr<PwaDelegate> pwa_delegate_;
   base::WeakPtrFactory<AndroidSmsAppManagerImpl> weak_ptr_factory_{this};

chrome/browser/chromeos/android_sms/android_sms_app_manager_impl_unittest.cc

@@ -83,10 +83,21 @@ class AndroidSmsAppManagerImplTest : public testing::Test {
       return true;
     }
 
+    bool IsAppRegistryReady(Profile* profile) override { return true; }
+
+    void ExecuteOnAppRegistryReady(Profile* profile,
+                                   base::OnceClosure task) override {
+      test_task_runner_->PostTask(FROM_HERE, std::move(task));
+    }
+
+    void RunPendingTasks() { test_task_runner_->RunUntilIdle(); }
+
    private:
     std::vector<std::string> opened_app_ids_;
     std::vector<std::pair<std::string, std::string>>
         transfer_item_attribute_params_;
+    scoped_refptr<base::TestSimpleTaskRunner> test_task_runner_ =
+        base::MakeRefCounted<base::TestSimpleTaskRunner>();
 
     DISALLOW_COPY_AND_ASSIGN(TestPwaDelegate);
   };
@@ -99,20 +110,16 @@ class AndroidSmsAppManagerImplTest : public testing::Test {
     fake_android_sms_app_setup_controller_ =
         std::make_unique<FakeAndroidSmsAppSetupController>();
 
-    test_task_runner_ = base::MakeRefCounted<base::TestSimpleTaskRunner>();
     test_pref_service_ =
         std::make_unique<sync_preferences::TestingPrefServiceSyncable>();
     AndroidSmsAppManagerImpl::RegisterProfilePrefs(
         test_pref_service_->registry());
 
+    auto test_pwa_delegate = std::make_unique<TestPwaDelegate>();
+    test_pwa_delegate_ = test_pwa_delegate.get();
     android_sms_app_manager_ = std::make_unique<AndroidSmsAppManagerImpl>(
         &profile_, fake_android_sms_app_setup_controller_.get(),
         test_pref_service_.get(), nullptr /* app_list_syncable_service */,
-        test_task_runner_);
-
-    auto test_pwa_delegate = std::make_unique<TestPwaDelegate>();
-    test_pwa_delegate_ = test_pwa_delegate.get();
-    android_sms_app_manager_->SetPwaDelegateForTesting(
         std::move(test_pwa_delegate));
 
     test_observer_ = std::make_unique<TestObserver>();
@@ -123,7 +130,7 @@ class AndroidSmsAppManagerImplTest : public testing::Test {
     android_sms_app_manager_->RemoveObserver(test_observer_.get());
   }
 
-  void CompleteAsyncInitialization() { test_task_runner_->RunUntilIdle(); }
+  void CompleteAsyncInitialization() { test_pwa_delegate_->RunPendingTasks(); }
 
   TestPwaDelegate* test_pwa_delegate() { return test_pwa_delegate_; }
 
@@ -149,7 +156,6 @@ class AndroidSmsAppManagerImplTest : public testing::Test {
       test_pref_service_;
   std::unique_ptr<FakeAndroidSmsAppSetupController>
       fake_android_sms_app_setup_controller_;
-  scoped_refptr<base::TestSimpleTaskRunner> test_task_runner_;
 
   TestPwaDelegate* test_pwa_delegate_;
   std::unique_ptr<TestObserver> test_observer_;

chrome/browser/chromeos/android_sms/android_sms_pairing_state_tracker_impl.cc

@@ -96,11 +96,17 @@ void AndroidSmsPairingStateTrackerImpl::OnInstalledAppUrlChanged() {
 }
 
 GURL AndroidSmsPairingStateTrackerImpl::GetPairingUrl() {
+  // If the app registry is not ready, we can't see check what is currently
+  // installed.
+  if (android_sms_app_manager_->IsAppRegistryReady()) {
     base::Optional<GURL> app_url = android_sms_app_manager_->GetCurrentAppUrl();
     if (app_url)
       return *app_url;
+  }
 
-  // If no app is installed, default to the normal messages URL.
+  // If no app is installed or the app registry is not ready, default to the
+  // expected messages URL.  This will only be incorrect if a migration must
+  // happen.
   return GetAndroidMessagesURL();
 }
 

chrome/browser/chromeos/android_sms/pairing_lost_notifier.cc

@@ -47,7 +47,12 @@ PairingLostNotifier::PairingLostNotifier(
       pref_service_(pref_service),
       android_sms_app_helper_delegate_(android_sms_app_helper_delegate) {
   multidevice_setup_client_->AddObserver(this);
-  HandleMessagesFeatureState();
+
+  // Wait until the app registry is loaded before querying for installed PWA
+  // info.
+  android_sms_app_helper_delegate_->ExecuteOnAppRegistryReady(
+      base::BindOnce(&PairingLostNotifier::HandleMessagesFeatureState,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 PairingLostNotifier::~PairingLostNotifier() {
@@ -61,6 +66,10 @@ void PairingLostNotifier::OnFeatureStatesChanged(
 }
 
 void PairingLostNotifier::HandleMessagesFeatureState() {
+  if (!android_sms_app_helper_delegate_->IsAppRegistryReady()) {
+    return;
+  }
+
   multidevice_setup::mojom::FeatureState state =
       multidevice_setup_client_->GetFeatureStates()
           .find(multidevice_setup::mojom::Feature::kMessages)

chrome/browser/chromeos/android_sms/pairing_lost_notifier_unittest.cc

@@ -39,6 +39,7 @@ class PairingLostNotifierTest : public BrowserWithTestWindowTest {
     PairingLostNotifier::RegisterProfilePrefs(test_pref_service_->registry());
     fake_android_sms_app_helper_delegate_ =
         std::make_unique<multidevice_setup::FakeAndroidSmsAppHelperDelegate>();
+    fake_android_sms_app_helper_delegate_->set_is_app_registry_ready(true);
 
     pairing_lost_notifier_ = std::make_unique<PairingLostNotifier>(
         profile(), fake_multidevice_setup_client_.get(),

chromeos/services/multidevice_setup/android_sms_app_installing_status_observer.cc

@@ -56,7 +56,12 @@ AndroidSmsAppInstallingStatusObserver::AndroidSmsAppInstallingStatusObserver(
       android_sms_app_helper_delegate_(android_sms_app_helper_delegate) {
   host_status_provider_->AddObserver(this);
   feature_state_manager_->AddObserver(this);
-  UpdatePwaInstallationState();
+
+  // Wait until the app registry has been loaded before updating installation
+  // status.
+  android_sms_app_helper_delegate_->ExecuteOnAppRegistryReady(base::BindOnce(
+      &AndroidSmsAppInstallingStatusObserver::UpdatePwaInstallationState,
+      weak_ptr_factory_.GetWeakPtr()));
 }
 
 bool AndroidSmsAppInstallingStatusObserver::
@@ -80,6 +85,11 @@ bool AndroidSmsAppInstallingStatusObserver::
 }
 
 void AndroidSmsAppInstallingStatusObserver::UpdatePwaInstallationState() {
+  if (!android_sms_app_helper_delegate_->IsAppRegistryReady()) {
+    PA_LOG(INFO) << "App registry is not ready.";
+    return;
+  }
+
   if (!DoesFeatureStateAllowInstallation()) {
     PA_LOG(INFO)
         << "Feature state does not allow installation, tearing down App.";

chromeos/services/multidevice_setup/android_sms_app_installing_status_observer.h

@@ -64,6 +64,8 @@ class AndroidSmsAppInstallingStatusObserver
   HostStatusProvider* host_status_provider_;
   FeatureStateManager* feature_state_manager_;
   AndroidSmsAppHelperDelegate* android_sms_app_helper_delegate_;
+  base::WeakPtrFactory<AndroidSmsAppInstallingStatusObserver> weak_ptr_factory_{
+      this};
 
   DISALLOW_COPY_AND_ASSIGN(AndroidSmsAppInstallingStatusObserver);
 };

chromeos/services/multidevice_setup/android_sms_app_installing_status_observer_unittest.cc

@@ -41,6 +41,7 @@ class MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest
         AndroidSmsAppInstallingStatusObserver::Factory::Create(
             fake_host_status_provider_.get(), fake_feature_state_manager_.get(),
             fake_android_sms_app_helper_delegate_.get());
+    fake_android_sms_app_helper_delegate_->set_is_app_registry_ready(true);
 
     SetMessagesFeatureState(mojom::FeatureState::kEnabledByUser);
     SetHostWithStatus(mojom::HostStatus::kHostVerified, GetFakePhone());
@@ -70,6 +71,11 @@ class MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest
                                                  feature_state);
   }
 
+  mojom::FeatureState GetMessagesFeatureState() {
+    return fake_feature_state_manager_->GetFeatureState(
+        mojom::Feature::kMessages);
+  }
+
  private:
   std::unique_ptr<FakeHostStatusProvider> fake_host_status_provider_;
   std::unique_ptr<FakeFeatureStateManager> fake_feature_state_manager_;
@@ -134,6 +140,19 @@ TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
   EXPECT_FALSE(fake_app_helper_delegate()->has_installed_app());
 }
 
+TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
+       DoesNotDisableFeatureIfAppRegistryNotReady) {
+  SetHostWithStatus(mojom::HostStatus::kNoEligibleHosts,
+                    base::nullopt /* host_device */);
+  fake_app_helper_delegate()->Reset();
+  fake_app_helper_delegate()->set_has_app_been_manually_uninstalled(true);
+  fake_app_helper_delegate()->set_is_app_registry_ready(false);
+  SetMessagesFeatureState(mojom::FeatureState::kEnabledByUser);
+
+  SetHostWithStatus(mojom::HostStatus::kHostVerified, GetFakePhone());
+  EXPECT_EQ(GetMessagesFeatureState(), mojom::FeatureState::kEnabledByUser);
+}
+
 TEST_F(MultiDeviceSetupAndroidSmsAppInstallingStatusObserverTest,
        DoesNotInstallsAfterHostVerifiedIfNotSupportedByPhone) {
   SetMessagesFeatureState(mojom::FeatureState::kNotSupportedByPhone);

chromeos/services/multidevice_setup/fake_feature_state_manager.cc

@@ -31,6 +31,11 @@ FakeFeatureStateManager::FakeFeatureStateManager()
 
 FakeFeatureStateManager::~FakeFeatureStateManager() = default;
 
+mojom::FeatureState FakeFeatureStateManager::GetFeatureState(
+    mojom::Feature feature) {
+  return feature_states_map_[feature];
+}
+
 void FakeFeatureStateManager::SetFeatureState(mojom::Feature feature,
                                               mojom::FeatureState state) {
   if (feature_states_map_[feature] == state)

chromeos/services/multidevice_setup/fake_feature_state_manager.h

@@ -21,6 +21,7 @@ class FakeFeatureStateManager : public FeatureStateManager {
   FakeFeatureStateManager();
   ~FakeFeatureStateManager() override;
 
+  mojom::FeatureState GetFeatureState(mojom::Feature feature);
   void SetFeatureState(mojom::Feature feature, mojom::FeatureState state);
   void SetFeatureStates(const FeatureStatesMap& feature_states_map);
 

chromeos/services/multidevice_setup/public/cpp/android_sms_app_helper_delegate.h

@@ -28,6 +28,11 @@ class AndroidSmsAppHelperDelegate {
   // Returns true if the app was ever installed successfully since the feature
   // was enabled and then been manually uninstalled by the user.
   virtual bool HasAppBeenManuallyUninstalledByUser() = 0;
+  // Returns true when details about installed PWAs is available to query.
+  virtual bool IsAppRegistryReady() = 0;
+  // Takes a task to run when the app registry is available.  If already
+  // available it will execute asynchronously.
+  virtual void ExecuteOnAppRegistryReady(base::OnceClosure task) = 0;
 
  protected:
   AndroidSmsAppHelperDelegate() = default;

chromeos/services/multidevice_setup/public/cpp/fake_android_sms_app_helper_delegate.cc

@@ -39,6 +39,15 @@ bool FakeAndroidSmsAppHelperDelegate::HasAppBeenManuallyUninstalledByUser() {
   return has_app_been_manually_uninstalled_;
 }
 
+bool FakeAndroidSmsAppHelperDelegate::IsAppRegistryReady() {
+  return is_app_registry_ready_;
+}
+
+void FakeAndroidSmsAppHelperDelegate::ExecuteOnAppRegistryReady(
+    base::OnceClosure task) {
+  std::move(task).Run();
+}
+
 }  // namespace multidevice_setup
 
 }  // namespace chromeos

chromeos/services/multidevice_setup/public/cpp/fake_android_sms_app_helper_delegate.h

@@ -29,6 +29,10 @@ class FakeAndroidSmsAppHelperDelegate
     has_app_been_manually_uninstalled_ = has_app_been_manually_uninstalled;
   }
 
+  void set_is_app_registry_ready(bool is_app_registry_ready) {
+    is_app_registry_ready_ = is_app_registry_ready;
+  }
+
   // Sets all booleans representing recorded actions to false.
   void Reset();
 
@@ -38,11 +42,14 @@ class FakeAndroidSmsAppHelperDelegate
   void SetUpAndLaunchAndroidSmsApp() override;
   void TearDownAndroidSmsApp() override;
   bool HasAppBeenManuallyUninstalledByUser() override;
+  bool IsAppRegistryReady() override;
+  void ExecuteOnAppRegistryReady(base::OnceClosure task) override;
 
   bool has_installed_app_ = false;
   bool has_launched_app_ = false;
   bool is_default_to_persist_cookie_set_ = false;
   bool has_app_been_manually_uninstalled_ = false;
+  bool is_app_registry_ready_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(FakeAndroidSmsAppHelperDelegate);
 };