[WebAppProvider] Enforce that WebAppInstallTask may only be used once

While reading the code it isn't clear the WebAppInstallTask may only be
used once. Adding DCHECKs and a comment to clarify this for future
readers.

R=loyso@chromium.org

Change-Id: I08c188ac9eaef53ab7b9a82f0bbd93f383a5e936
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2300830
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Auto-Submit: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797429}

chrome/browser/web_applications/web_app_install_task.cc

@@ -97,6 +97,7 @@ void WebAppInstallTask::LoadWebAppAndCheckManifest(
     WebAppUrlLoader* url_loader,
     LoadWebAppAndCheckManifestCallback callback) {
   DCHECK(url_loader);
+  CheckInstallPreconditions();
   // Create a WebContents instead of reusing a shared one because we will pass
   // it back to be used for opening the web app.
   // TODO(loyso): Implement stealing of shared web_contents in upcoming
@@ -233,6 +234,7 @@ void WebAppInstallTask::UpdateWebAppFromInfo(
     const AppId& app_id,
     std::unique_ptr<WebApplicationInfo> web_application_info,
     InstallManager::OnceInstallCallback callback) {
+  CheckInstallPreconditions();
   Observe(web_contents);
   install_callback_ = std::move(callback);
   background_installation_ = true;
@@ -297,6 +299,9 @@ void WebAppInstallTask::CheckInstallPreconditions() {
   DCHECK(!web_contents());
   CHECK(!install_callback_);
   CHECK(!retrieve_info_callback_);
+
+  DCHECK(!initiated_);
+  initiated_ = true;
 }
 
 void WebAppInstallTask::RecordInstallEvent(

chrome/browser/web_applications/web_app_install_task.h

@@ -42,6 +42,10 @@ class InstallFinalizer;
 class WebAppDataRetriever;
 class WebAppUrlLoader;
 
+// Used to do a variety of tasks involving installing web applications. Only one
+// of the public Load*, Update*, or Install* methods can be called on a single
+// object. WebAppInstallManager is a queue of WebAppInstallTask jobs. Basically,
+// WebAppInstallTask is an implementation detail of WebAppInstallManager.
 class WebAppInstallTask : content::WebContentsObserver {
  public:
   using RetrieveWebApplicationInfoWithIconsCallback =
@@ -230,6 +234,10 @@ class WebAppInstallTask : content::WebContentsObserver {
                         const AppId& app_id,
                         const OsHooksResults os_hooks_results);
 
+  // Whether the install task has been 'initiated' by calling one of the public
+  // methods.
+  bool initiated_ = false;
+
   // Whether we should just obtain WebApplicationInfo instead of the actual
   // installation.
   bool only_retrieve_web_application_info_ = false;

chrome/browser/web_applications/web_app_install_task_unittest.cc

@@ -181,6 +181,15 @@ class WebAppInstallTaskTest : public WebAppTest {
     return base::NullableString16(base::UTF8ToUTF16(str), false);
   }
 
+  void ResetInstallTask() {
+    auto data_retriever = std::make_unique<TestDataRetriever>();
+    data_retriever_ = static_cast<TestDataRetriever*>(data_retriever.get());
+
+    install_task_ = std::make_unique<WebAppInstallTask>(
+        profile(), os_integration_manager_.get(), install_finalizer_.get(),
+        std::move(data_retriever));
+  }
+
   void SetInstallFinalizerForTesting() {
     auto test_install_finalizer = std::make_unique<TestInstallFinalizer>();
     test_install_finalizer_ = test_install_finalizer.get();
@@ -332,8 +341,9 @@ class WebAppInstallTaskTest : public WebAppTest {
   std::unique_ptr<InstallFinalizer> install_finalizer_;
   std::unique_ptr<TestOsIntegrationManager> os_integration_manager_;
 
-  // Owned by install_task_:
+  // Owned by icon_manager_:
   TestFileUtils* file_utils_ = nullptr;
+  // Owned by install_task_:
   TestDataRetriever* data_retriever_ = nullptr;
 
 #if defined(OS_CHROMEOS)
@@ -444,6 +454,7 @@ TEST_F(WebAppInstallTaskTest, ForceReinstall) {
 
   const AppId installed_web_app = InstallWebAppFromManifestWithFallback();
   EXPECT_EQ(app_id, installed_web_app);
+  ResetInstallTask();
 
   // Force reinstall:
   CreateRendererAppInfo(url, "Renderer Name2", "Renderer Description2");
@@ -1035,6 +1046,7 @@ TEST_F(WebAppInstallTaskTest, InstallWebAppWithParams_DisplayMode) {
     EXPECT_EQ(DisplayMode::kBrowser,
               registrar().GetAppById(app_id)->user_display_mode());
   }
+  ResetInstallTask();
   {
     CreateDataToRetrieve(GURL("https://example.org/"), /*open_as_window*/ true);
 
@@ -1043,6 +1055,7 @@ TEST_F(WebAppInstallTaskTest, InstallWebAppWithParams_DisplayMode) {
     EXPECT_EQ(DisplayMode::kStandalone,
               registrar().GetAppById(app_id)->user_display_mode());
   }
+  ResetInstallTask();
   {
     CreateDataToRetrieve(GURL("https://example.au/"), /*open_as_window*/ true);
 
@@ -1051,6 +1064,7 @@ TEST_F(WebAppInstallTaskTest, InstallWebAppWithParams_DisplayMode) {
     EXPECT_EQ(DisplayMode::kBrowser,
               registrar().GetAppById(app_id)->user_display_mode());
   }
+  ResetInstallTask();
   {
     CreateDataToRetrieve(GURL("https://example.app/"),
                          /*open_as_window*/ false);
@@ -1076,6 +1090,7 @@ TEST_F(WebAppInstallTaskTest, InstallWebAppFromManifest_ExpectAppId) {
     EXPECT_EQ(app_id1, result.app_id);
     EXPECT_TRUE(registrar().GetAppById(app_id1));
   }
+  ResetInstallTask();
   {
     CreateDefaultDataToRetrieve(url2);
     install_task().ExpectAppId(app_id1);
@@ -1099,6 +1114,7 @@ TEST_F(WebAppInstallTaskTest, LoadAndInstallWebAppFromManifestWithFallback) {
     EXPECT_TRUE(result.app_id.empty());
     EXPECT_FALSE(registrar().GetAppById(app_id));
   }
+  ResetInstallTask();
   {
     CreateDefaultDataToRetrieve(url);
     url_loader().SetNextLoadUrlResult(
@@ -1109,6 +1125,7 @@ TEST_F(WebAppInstallTaskTest, LoadAndInstallWebAppFromManifestWithFallback) {
     EXPECT_TRUE(result.app_id.empty());
     EXPECT_FALSE(registrar().GetAppById(app_id));
   }
+  ResetInstallTask();
   {
     CreateDefaultDataToRetrieve(url);
     url_loader().SetNextLoadUrlResult(url, WebAppUrlLoader::Result::kUrlLoaded);
@@ -1118,6 +1135,7 @@ TEST_F(WebAppInstallTaskTest, LoadAndInstallWebAppFromManifestWithFallback) {
     EXPECT_EQ(app_id, result.app_id);
     EXPECT_TRUE(registrar().GetAppById(app_id));
   }
+  ResetInstallTask();
   {
     CreateDefaultDataToRetrieve(url);
     url_loader().SetNextLoadUrlResult(url, WebAppUrlLoader::Result::kUrlLoaded);
@@ -1144,6 +1162,7 @@ TEST_F(WebAppInstallTaskTest, LoadAndRetrieveWebApplicationInfoWithIcons) {
         LoadAndRetrieveWebApplicationInfoWithIcons(url);
     EXPECT_FALSE(result);
   }
+  ResetInstallTask();
   {
     CreateDefaultDataToRetrieve(url);
     url_loader().SetNextLoadUrlResult(
@@ -1153,6 +1172,7 @@ TEST_F(WebAppInstallTaskTest, LoadAndRetrieveWebApplicationInfoWithIcons) {
         LoadAndRetrieveWebApplicationInfoWithIcons(url);
     EXPECT_FALSE(result);
   }
+  ResetInstallTask();
   {
     CreateDefaultDataToRetrieve(start_url);
     CreateRendererAppInfo(url, name, description);
@@ -1165,6 +1185,7 @@ TEST_F(WebAppInstallTaskTest, LoadAndRetrieveWebApplicationInfoWithIcons) {
     EXPECT_TRUE(result->icon_infos.empty());
     EXPECT_FALSE(result->icon_bitmaps_any.empty());
   }
+  ResetInstallTask();
   {
     // Verify the callback is always called.
     base::RunLoop run_loop;
@@ -1474,6 +1495,7 @@ TEST_F(WebAppInstallTaskTestWithShortcutsMenu,
     EXPECT_EQ(InstallResultCode::kSuccessNewInstall, result.code);
     EXPECT_EQ(app_id, result.app_id);
   }
+  ResetInstallTask();
 
   // Update the installed app, adding a Shortcuts Menu in the process.
   {
@@ -1499,6 +1521,7 @@ TEST_F(WebAppInstallTaskTestWithShortcutsMenu,
     EXPECT_EQ(InstallResultCode::kSuccessNewInstall, result.code);
     EXPECT_EQ(app_id, result.app_id);
   }
+  ResetInstallTask();
 
   // Update the installed app, Shortcuts Menu has changed.
   {
@@ -1523,6 +1546,7 @@ TEST_F(WebAppInstallTaskTestWithShortcutsMenu,
     EXPECT_EQ(InstallResultCode::kSuccessNewInstall, result.code);
     EXPECT_EQ(app_id, result.app_id);
   }
+  ResetInstallTask();
 
   // Update the installed app. Only theme color changed, so Shortcuts Menu
   // should stay the same.