WebApps: Disable manifest updating for placeholder apps

Placeholder apps are pending installation completion. Attempting to
update them will collide with PendingAppManager attempting to finish
installing them.

Bug: 926083
Change-Id: I0c9e5f61dd8e60ccd0af190b0b8cfb63a1115a23
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1830250
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701942}

chrome/browser/web_applications/components/app_registrar.cc

@@ -24,6 +24,11 @@ bool AppRegistrar::IsLocallyInstalled(const GURL& start_url) const {
   return IsLocallyInstalled(GenerateAppIdFromURL(start_url));
 }
 
+bool AppRegistrar::IsPlaceholderApp(const AppId& app_id) const {
+  return ExternallyInstalledWebAppPrefs(profile_->GetPrefs())
+      .IsPlaceholderApp(app_id);
+}
+
 void AppRegistrar::AddObserver(AppRegistrarObserver* observer) {
   observers_.AddObserver(observer);
 }

chrome/browser/web_applications/components/app_registrar.h

@@ -93,6 +93,10 @@ class AppRegistrar {
   // that fall within the scope.
   bool IsLocallyInstalled(const GURL& start_url) const;
 
+  // Returns whether the app is pending successful navigation in order to
+  // complete installation via the PendingAppManager.
+  bool IsPlaceholderApp(const AppId& app_id) const;
+
   void AddObserver(AppRegistrarObserver* observer);
   void RemoveObserver(AppRegistrarObserver* observer);
 

chrome/browser/web_applications/components/externally_installed_web_app_prefs.cc

@@ -210,4 +210,14 @@ void ExternallyInstalledWebAppPrefs::SetIsPlaceholder(const GURL& url,
   app_entry->SetBoolKey(kIsPlaceholder, is_placeholder);
 }
 
+bool ExternallyInstalledWebAppPrefs::IsPlaceholderApp(
+    const AppId& app_id) const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  const base::Value* app_prefs = GetPreferenceValue(pref_service_, app_id);
+  if (!app_prefs || !app_prefs->is_dict())
+    return false;
+  return app_prefs->FindBoolKey(kIsPlaceholder).value_or(false);
+}
+
 }  // namespace web_app

chrome/browser/web_applications/components/externally_installed_web_app_prefs.h

@@ -57,6 +57,7 @@ class ExternallyInstalledWebAppPrefs {
   // *placeholder app*.
   base::Optional<AppId> LookupPlaceholderAppId(const GURL& url) const;
   void SetIsPlaceholder(const GURL& url, bool is_placeholder);
+  bool IsPlaceholderApp(const AppId& app_id) const;
 
  private:
   PrefService* pref_service_;

chrome/browser/web_applications/components/manifest_update_manager.cc

@@ -40,8 +40,12 @@ void ManifestUpdateManager::MaybeUpdate(const GURL& url,
     return;
   }
 
-  std::unique_ptr<ManifestUpdateTask>& current_task = tasks_[app_id];
-  if (current_task)
+  if (registrar_->IsPlaceholderApp(app_id)) {
+    NotifyResult(url, ManifestUpdateResult::kAppIsPlaceholder);
+    return;
+  }
+
+  if (base::Contains(tasks_, app_id))
     return;
 
   if (!MaybeConsumeUpdateCheck(app_id)) {
@@ -49,12 +53,13 @@ void ManifestUpdateManager::MaybeUpdate(const GURL& url,
     return;
   }
 
-  current_task = std::make_unique<ManifestUpdateTask>(
-      url, app_id, web_contents,
-      base::Bind(&ManifestUpdateManager::OnUpdateStopped,
-                 base::Unretained(this)),
-      hang_update_checks_for_testing_, *registrar_, ui_manager_,
-      install_manager_);
+  tasks_.insert_or_assign(
+      app_id, std::make_unique<ManifestUpdateTask>(
+                  url, app_id, web_contents,
+                  base::Bind(&ManifestUpdateManager::OnUpdateStopped,
+                             base::Unretained(this)),
+                  hang_update_checks_for_testing_, *registrar_, ui_manager_,
+                  install_manager_));
 }
 
 // AppRegistrarObserver:

chrome/browser/web_applications/components/manifest_update_manager_browsertest.cc

@@ -17,6 +17,7 @@
 #include "chrome/browser/web_applications/components/app_registry_controller.h"
 #include "chrome/browser/web_applications/components/install_finalizer.h"
 #include "chrome/browser/web_applications/components/install_manager.h"
+#include "chrome/browser/web_applications/components/pending_app_manager.h"
 #include "chrome/browser/web_applications/components/web_app_provider_base.h"
 #include "chrome/browser/web_applications/components/web_app_tab_helper.h"
 #include "chrome/common/chrome_features.h"
@@ -111,20 +112,27 @@ class ManifestUpdateManagerBrowserTest : public InProcessBrowserTest {
 
   std::unique_ptr<net::test_server::HttpResponse> RequestHandlerOverride(
       const net::test_server::HttpRequest& request) {
-    if (request.GetURL() != override_url_)
-      return nullptr;
-    auto http_response =
-        std::make_unique<net::test_server::BasicHttpResponse>();
-    http_response->set_code(net::HTTP_FOUND);
-    http_response->set_content(override_content_);
-    return std::move(http_response);
+    if (request_override_)
+      return request_override_.Run(request);
+    return nullptr;
   }
 
   void OverrideManifest(const char* manifest_template,
                         const std::vector<std::string>& substitutions) {
-    override_url_ = GetManifestURL();
-    override_content_ = base::ReplaceStringPlaceholders(manifest_template,
-                                                        substitutions, nullptr);
+    std::string content = base::ReplaceStringPlaceholders(
+        manifest_template, substitutions, nullptr);
+    request_override_ = base::BindLambdaForTesting(
+        [this, content = std::move(content)](
+            const net::test_server::HttpRequest& request)
+            -> std::unique_ptr<net::test_server::HttpResponse> {
+          if (request.GetURL() != GetManifestURL())
+            return nullptr;
+          auto http_response =
+              std::make_unique<net::test_server::BasicHttpResponse>();
+          http_response->set_code(net::HTTP_FOUND);
+          http_response->set_content(content);
+          return std::move(http_response);
+        });
   }
 
   GURL GetAppURL() const {
@@ -176,12 +184,12 @@ class ManifestUpdateManagerBrowserTest : public InProcessBrowserTest {
     return *WebAppProviderBase::GetProviderBase(browser()->profile());
   }
 
+  net::EmbeddedTestServer::HandleRequestCallback request_override_;
+
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
 
   net::EmbeddedTestServer http_server_;
-  GURL override_url_;
-  std::string override_content_;
 
   DISALLOW_COPY_AND_ASSIGN(ManifestUpdateManagerBrowserTest);
 };
@@ -395,6 +403,61 @@ IN_PROC_BROWSER_TEST_F(ManifestUpdateManagerBrowserTest,
             ManifestUpdateResult::kNoAppInScope);
 }
 
+IN_PROC_BROWSER_TEST_F(ManifestUpdateManagerBrowserTest,
+                       CheckIgnoresPlaceholderApps) {
+  // Set up app URL to redirect to force placeholder app to install.
+  const GURL app_url = GetAppURL();
+  request_override_ = base::BindLambdaForTesting(
+      [&app_url](const net::test_server::HttpRequest& request)
+          -> std::unique_ptr<net::test_server::HttpResponse> {
+        if (request.GetURL() != app_url)
+          return nullptr;
+        auto http_response =
+            std::make_unique<net::test_server::BasicHttpResponse>();
+        http_response->set_code(net::HTTP_TEMPORARY_REDIRECT);
+        http_response->AddCustomHeader("Location", "/defaultresponse");
+        http_response->set_content("redirect page");
+        return std::move(http_response);
+      });
+
+  // Install via PendingAppManager, the redirect should cause it to install a
+  // placeholder app.
+  base::RunLoop run_loop;
+  ExternalInstallOptions install_options(
+      app_url, LaunchContainer::kWindow,
+      ExternalInstallSource::kExternalPolicy);
+  install_options.add_to_applications_menu = false;
+  install_options.add_to_desktop = false;
+  install_options.add_to_quick_launch_bar = false;
+  install_options.install_placeholder = true;
+  GetProvider().pending_app_manager().Install(
+      std::move(install_options),
+      base::BindLambdaForTesting(
+          [&](const GURL& installed_app_url, InstallResultCode code) {
+            EXPECT_EQ(installed_app_url, app_url);
+            EXPECT_EQ(code, InstallResultCode::kSuccessNewInstall);
+            run_loop.Quit();
+          }));
+  run_loop.Run();
+  AppId app_id = GetProvider().registrar().LookupExternalAppId(app_url).value();
+  EXPECT_TRUE(GetProvider().registrar().IsPlaceholderApp(app_id));
+
+  // Manifest updating should ignore non-redirect loads for placeholder apps
+  // because the PendingAppManager will handle these.
+  const char* manifest_template = R"(
+    {
+      "name": "Test app name",
+      "start_url": ".",
+      "scope": "/",
+      "display": "standalone",
+      "icons": $1
+    }
+  )";
+  OverrideManifest(manifest_template, {kInstallableIconList});
+  EXPECT_EQ(GetResultAfterPageLoad(app_url, &app_id),
+            ManifestUpdateResult::kAppIsPlaceholder);
+}
+
 IN_PROC_BROWSER_TEST_F(ManifestUpdateManagerBrowserTest,
                        CheckFindsThemeColorChange) {
   const char* manifest_template = R"(

chrome/browser/web_applications/components/manifest_update_task.h

@@ -25,6 +25,7 @@ enum ManifestUpdateResult {
   kThrottled,
   kWebContentsDestroyed,
   kAppUninstalled,
+  kAppIsPlaceholder,
   kAppUpToDate,
   kAppDataInvalid,
   kAppUpdateFailed,

chrome/browser/web_applications/extensions/externally_installed_web_app_prefs_unittest.cc

@@ -176,4 +176,26 @@ TEST_F(ExternallyInstalledWebAppPrefsTest, BasicOps) {
             GetAppUrls(ExternalInstallSource::kExternalPolicy));
 }
 
+TEST_F(ExternallyInstalledWebAppPrefsTest, IsPlaceholderApp) {
+  const GURL url("https://example.com");
+  const AppId app_id = "app_id_string";
+  ExternallyInstalledWebAppPrefs prefs(profile()->GetPrefs());
+  prefs.Insert(url, app_id, ExternalInstallSource::kExternalPolicy);
+  EXPECT_FALSE(ExternallyInstalledWebAppPrefs(profile()->GetPrefs())
+                   .IsPlaceholderApp(app_id));
+  prefs.SetIsPlaceholder(url, true);
+  EXPECT_TRUE(ExternallyInstalledWebAppPrefs(profile()->GetPrefs())
+                  .IsPlaceholderApp(app_id));
+}
+
+TEST_F(ExternallyInstalledWebAppPrefsTest, OldPrefFormat) {
+  // Set up the old format for this pref {url -> app_id}.
+  DictionaryPrefUpdate update(profile()->GetPrefs(),
+                              prefs::kWebAppsExtensionIDs);
+  update->SetKey("https://example.com", base::Value("add_id_string"));
+  // This should not crash on invalid pref data.
+  EXPECT_FALSE(ExternallyInstalledWebAppPrefs(profile()->GetPrefs())
+                   .IsPlaceholderApp("app_id_string"));
+}
+
 }  // namespace web_app