WebApp: Add WebAppInstallFinalizer is started check.

Return runtime error if finalizer is not started.

Unfortunately, WebAppInstallFinalizer::FinalizeInstall is
used directly by PendingAppManager (for now).

This is a speculative fix for the crash in the bug:
Do not even initiate the writing operation if web app provider
is not yet ready.
In next CLs: Add more started/shutdown checks in
WebAppInstallManager, ManifestUpdateManager. We should return
WebAppProviderNotReady error as early as possible (in
WebAppInstallManager API).

Bug: 1084939
Change-Id: I3f7a27f587da4d2c8676f9863f94650f7357d5ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2208889
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770958}

chrome/browser/web_applications/components/install_finalizer.h

@@ -103,6 +103,9 @@ class InstallFinalizer {
                            bool shortcut_created,
                            content::WebContents* web_contents);
 
+  virtual void Start() {}
+  virtual void Shutdown() {}
+
   void SetSubsystems(AppRegistrar* registrar,
                      WebAppUiManager* ui_manager,
                      AppRegistryController* registry_controller);

chrome/browser/web_applications/components/install_finalizer_unittest.cc

@@ -97,6 +97,7 @@ class InstallFinalizerUnitTest
     finalizer_->SetSubsystems(&registrar(), ui_manager_.get(),
                               &test_registry_controller_->sync_bridge());
     test_registry_controller_->Init();
+    finalizer_->Start();
   }
 
   void TearDown() override {

chrome/browser/web_applications/components/web_app_constants.h

@@ -83,8 +83,11 @@ enum class InstallResultCode {
   // App managers are shutting down. For example, when user logs out immediately
   // after login.
   kCancelledOnWebAppProviderShuttingDown = 21,
+  // The Web Apps system is not ready: registry is not yet opened or already
+  // closed.
+  kWebAppProviderNotReady = 22,
 
-  kMaxValue = kCancelledOnWebAppProviderShuttingDown
+  kMaxValue = kWebAppProviderNotReady
 };
 
 // Checks if InstallResultCode is not a failure.

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -140,6 +140,14 @@ void WebAppInstallFinalizer::FinalizeInstall(
     const FinalizeOptions& options,
     InstallFinalizedCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  // TODO(crbug.com/1084939): Implement a before-start queue in
+  // WebAppInstallManager and replace this runtime error in
+  // WebAppInstallFinalizer with DCHECK(started_).
+  if (!started_) {
+    std::move(callback).Run(AppId(),
+                            InstallResultCode::kWebAppProviderNotReady);
+    return;
+  }
 
   // TODO(loyso): Expose Source argument as a field of AppTraits struct.
   const auto source =
@@ -191,6 +199,8 @@ void WebAppInstallFinalizer::FinalizeInstall(
 void WebAppInstallFinalizer::FinalizeFallbackInstallAfterSync(
     const AppId& app_id,
     InstallFinalizedCallback callback) {
+  DCHECK(started_);
+
   const WebApp* app_in_sync_install = GetWebAppRegistrar().GetAppById(app_id);
   DCHECK(app_in_sync_install);
 
@@ -242,6 +252,7 @@ void WebAppInstallFinalizer::FinalizeFallbackInstallAfterSync(
 void WebAppInstallFinalizer::FinalizeUninstallAfterSync(
     const AppId& app_id,
     UninstallWebAppCallback callback) {
+  DCHECK(started_);
   // WebAppSyncBridge::ApplySyncChangesToRegistrar does the actual
   // NotifyWebAppUninstalled and unregistration of the app from the registry.
   DCHECK(!GetWebAppRegistrar().GetAppById(app_id));
@@ -256,6 +267,7 @@ void WebAppInstallFinalizer::UninstallExternalWebApp(
     const AppId& app_id,
     ExternalInstallSource external_install_source,
     UninstallWebAppCallback callback) {
+  DCHECK(started_);
   Source::Type source =
       InferSourceFromExternalInstallSource(external_install_source);
   UninstallWebAppOrRemoveSource(app_id, source, std::move(callback));
@@ -263,6 +275,7 @@ void WebAppInstallFinalizer::UninstallExternalWebApp(
 
 bool WebAppInstallFinalizer::CanUserUninstallFromSync(
     const AppId& app_id) const {
+  DCHECK(started_);
   const WebApp* app = GetWebAppRegistrar().GetAppById(app_id);
   return app ? app->IsSynced() : false;
 }
@@ -276,6 +289,7 @@ void WebAppInstallFinalizer::UninstallWebAppFromSyncByUser(
 
 bool WebAppInstallFinalizer::CanUserUninstallExternalApp(
     const AppId& app_id) const {
+  DCHECK(started_);
   // TODO(loyso): Policy Apps: Implement web_app::ManagementPolicy taking
   // extensions::ManagementPolicy::UserMayModifySettings as inspiration.
   const WebApp* app = GetWebAppRegistrar().GetAppById(app_id);
@@ -285,6 +299,8 @@ bool WebAppInstallFinalizer::CanUserUninstallExternalApp(
 void WebAppInstallFinalizer::UninstallExternalAppByUser(
     const AppId& app_id,
     UninstallWebAppCallback callback) {
+  DCHECK(started_);
+
   const WebApp* app = GetWebAppRegistrar().GetAppById(app_id);
   DCHECK(app);
   DCHECK(app->CanUserUninstallExternalApp());
@@ -313,6 +329,12 @@ bool WebAppInstallFinalizer::WasExternalAppUninstalledByUser(
 void WebAppInstallFinalizer::FinalizeUpdate(
     const WebApplicationInfo& web_app_info,
     InstallFinalizedCallback callback) {
+  if (!started_) {
+    std::move(callback).Run(AppId(),
+                            InstallResultCode::kWebAppProviderNotReady);
+    return;
+  }
+
   const AppId app_id = GenerateAppIdFromURL(web_app_info.app_url);
   const WebApp* existing_web_app = GetWebAppRegistrar().GetAppById(app_id);
 
@@ -332,6 +354,15 @@ void WebAppInstallFinalizer::FinalizeUpdate(
                                       std::move(callback));
 }
 
+void WebAppInstallFinalizer::Start() {
+  DCHECK(!started_);
+  started_ = true;
+}
+
+void WebAppInstallFinalizer::Shutdown() {
+  started_ = false;
+}
+
 void WebAppInstallFinalizer::UninstallWebApp(const AppId& app_id,
                                              UninstallWebAppCallback callback) {
   registrar().NotifyWebAppUninstalled(app_id);

chrome/browser/web_applications/web_app_install_finalizer.h

@@ -48,6 +48,8 @@ class WebAppInstallFinalizer final : public InstallFinalizer {
   void UninstallExternalAppByUser(const AppId& app_id,
                                   UninstallWebAppCallback callback) override;
   bool WasExternalAppUninstalledByUser(const AppId& app_id) const override;
+  void Start() override;
+  void Shutdown() override;
 
  private:
   void UninstallWebApp(const AppId& app_id, UninstallWebAppCallback callback);
@@ -81,6 +83,7 @@ class WebAppInstallFinalizer final : public InstallFinalizer {
 
   Profile* const profile_;
   WebAppIconManager* const icon_manager_;
+  bool started_ = false;
 
   base::WeakPtrFactory<WebAppInstallFinalizer> weak_ptr_factory_{this};
 

chrome/browser/web_applications/web_app_install_manager_unittest.cc

@@ -166,6 +166,8 @@ class WebAppInstallManagerTest : public WebAppTest {
     install_finalizer_->SetSubsystems(
         &registrar(), ui_manager_.get(),
         &test_registry_controller_->sync_bridge());
+
+    install_finalizer_->Start();
   }
 
   void TearDown() override {

chrome/browser/web_applications/web_app_install_task_unittest.cc

@@ -97,6 +97,7 @@ class WebAppInstallTaskTest : public WebAppTest {
 
     install_finalizer_ = std::make_unique<WebAppInstallFinalizer>(
         profile(), icon_manager_.get());
+
     shortcut_manager_ = std::make_unique<TestAppShortcutManager>(profile());
     file_handler_manager_ = std::make_unique<TestFileHandlerManager>(profile());
 
@@ -116,6 +117,7 @@ class WebAppInstallTaskTest : public WebAppTest {
 
     url_loader_ = std::make_unique<TestWebAppUrlLoader>();
     controller().Init();
+    install_finalizer_->Start();
 
 #if defined(OS_CHROMEOS)
     arc_test_.SetUp(profile());

chrome/browser/web_applications/web_app_provider.cc

@@ -154,6 +154,7 @@ void WebAppProvider::Shutdown() {
   install_manager_->Shutdown();
   manifest_update_manager_->Shutdown();
   system_web_app_manager_->Shutdown();
+  install_finalizer_->Shutdown();
 }
 
 void WebAppProvider::StartImpl() {
@@ -269,6 +270,7 @@ void WebAppProvider::StartRegistryController() {
 void WebAppProvider::OnRegistryControllerReady() {
   DCHECK(!on_registry_ready_.is_signaled());
 
+  install_finalizer_->Start();
   external_web_app_manager_->Start();
   web_app_policy_manager_->Start();
   system_web_app_manager_->Start();

tools/metrics/histograms/enums.xml

@@ -70117,6 +70117,7 @@ Full version information for the fingerprint enum values:
   <int value="19" label="BookmarkExtensionInstallError"/>
   <int value="20" label="ApkWebInstallFailed"/>
   <int value="21" label="CancelledOnWebAppProviderShuttingDown"/>
+  <int value="22" label="WebAppProviderNotReady"/>
 </enum>
 
 <enum name="WebAppInstallSource">