desktop-pwas: Add queue to PendingBookmarkAppManager

Adds a queue to process installations to PendingBookmarkAppManager.

Multiple calls to Install() will result in installations being
added to the queue. Once an installation finishes, the next one
will start.

Bug: 864904
Change-Id: I063c1dd16760183a1fbbde64ed7b43bf89078dfe
Reviewed-on: https://chromium-review.googlesource.com/1163403
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582781}

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.cc

@@ -9,10 +9,11 @@
 #include <utility>
 #include <vector>
 
+#include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_shortcut_installation_task.h"
-#include "content/public/browser/web_contents.h"
 #include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/web_contents.h"
 
 namespace extensions {
 
@@ -33,6 +34,15 @@ std::unique_ptr<BookmarkAppInstallationTask> InstallationTaskCreateWrapper(
 
 }  // namespace
 
+struct PendingBookmarkAppManager::Installation {
+  Installation(AppInfo info, InstallCallback callback)
+      : info(std::move(info)), callback(std::move(callback)) {}
+  ~Installation() = default;
+
+  AppInfo info;
+  InstallCallback callback;
+};
+
 PendingBookmarkAppManager::PendingBookmarkAppManager(Profile* profile)
     : profile_(profile),
       web_contents_factory_(base::BindRepeating(&WebContentsCreateWrapper)),
@@ -42,22 +52,25 @@ PendingBookmarkAppManager::~PendingBookmarkAppManager() = default;
 
 void PendingBookmarkAppManager::Install(AppInfo app_to_install,
                                         InstallCallback callback) {
-  // The app is already being installed.
-  if (current_install_info_ && *current_install_info_ == app_to_install) {
+  // Check that we are not already installing the same app.
+  if (current_installation_ && current_installation_->info == app_to_install) {
     std::move(callback).Run(std::string());
     return;
   }
+  for (const auto& installation : installation_queue_) {
+    if (installation->info == app_to_install) {
+      std::move(callback).Run(std::string());
+      return;
+    }
+  }
 
-  current_install_info_ = std::make_unique<AppInfo>(std::move(app_to_install));
-  current_install_callback_ = std::move(callback);
-
-  CreateWebContentsIfNecessary();
-  Observe(web_contents_.get());
+  installation_queue_.push_back(std::make_unique<Installation>(
+      std::move(app_to_install), std::move(callback)));
 
-  content::NavigationController::LoadURLParams load_params(
-      current_install_info_->url);
-  load_params.transition_type = ui::PAGE_TRANSITION_GENERATED;
-  web_contents_->GetController().LoadURLWithParams(load_params);
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&PendingBookmarkAppManager::MaybeStartNextInstallation,
+                     weak_ptr_factory_.GetWeakPtr()));
 }
 
 void PendingBookmarkAppManager::ProcessAppOperations(
@@ -70,11 +83,53 @@ void PendingBookmarkAppManager::SetFactoriesForTesting(
   task_factory_ = std::move(task_factory);
 }
 
+void PendingBookmarkAppManager::MaybeStartNextInstallation() {
+  if (current_installation_)
+    return;
+
+  if (installation_queue_.empty()) {
+    web_contents_.reset();
+    return;
+  }
+
+  current_installation_ = std::move(installation_queue_.front());
+  installation_queue_.pop_front();
+
+  CreateWebContentsIfNecessary();
+  Observe(web_contents_.get());
+
+  content::NavigationController::LoadURLParams load_params(
+      current_installation_->info.url);
+  load_params.transition_type = ui::PAGE_TRANSITION_GENERATED;
+  web_contents_->GetController().LoadURLWithParams(load_params);
+}
+
 void PendingBookmarkAppManager::CreateWebContentsIfNecessary() {
   if (!web_contents_)
     web_contents_ = web_contents_factory_.Run(profile_);
 }
 
+void PendingBookmarkAppManager::OnInstalled(
+    BookmarkAppInstallationTask::Result result) {
+  CurrentInstallationFinished(result.app_id);
+}
+
+void PendingBookmarkAppManager::CurrentInstallationFinished(
+    const std::string& app_id) {
+  // Post a task to avoid reentrancy issues e.g. adding a WebContentsObserver
+  // while a previous observer call is being executed. Post a task before
+  // running the callback in case the callback tries to install another
+  // app.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&PendingBookmarkAppManager::MaybeStartNextInstallation,
+                     weak_ptr_factory_.GetWeakPtr()));
+
+  std::unique_ptr<Installation> installation;
+  installation.swap(current_installation_);
+  std::move(installation->callback).Run(app_id);
+}
+
 void PendingBookmarkAppManager::DidFinishLoad(
     content::RenderFrameHost* render_frame_host,
     const GURL& validated_url) {
@@ -82,8 +137,8 @@ void PendingBookmarkAppManager::DidFinishLoad(
     return;
   }
 
-  if (validated_url != current_install_info_->url) {
-    std::move(current_install_callback_).Run(std::string());
+  if (validated_url != current_installation_->info.url) {
+    CurrentInstallationFinished(std::string());
     return;
   }
 
@@ -110,21 +165,7 @@ void PendingBookmarkAppManager::DidFailLoad(
   }
 
   Observe(nullptr);
-  // TODO(crbug.com/864904): Only destroy the WebContents if there are no
-  // queued installation requests.
-  web_contents_.reset();
-  current_install_info_.reset();
-
-  std::move(current_install_callback_).Run(std::string());
-}
-
-void PendingBookmarkAppManager::OnInstalled(
-    BookmarkAppInstallationTask::Result result) {
-  // TODO(crbug.com/864904): Only destroy the WebContents if there are no
-  // queued installation requests.
-  web_contents_.reset();
-  current_install_info_.reset();
-  std::move(current_install_callback_).Run(result.app_id);
+  CurrentInstallationFinished(std::string());
 }
 
 }  // namespace extensions

chrome/browser/web_applications/extensions/pending_bookmark_app_manager.h

@@ -5,11 +5,14 @@
 #ifndef CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_PENDING_BOOKMARK_APP_MANAGER_H_
 #define CHROME_BROWSER_WEB_APPLICATIONS_EXTENSIONS_PENDING_BOOKMARK_APP_MANAGER_H_
 
+#include <deque>
 #include <memory>
+#include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_installation_task.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -50,6 +53,16 @@ class PendingBookmarkAppManager final : public web_app::PendingAppManager,
                               TaskFactory task_factory);
 
  private:
+  struct Installation;
+
+  void MaybeStartNextInstallation();
+
+  void CreateWebContentsIfNecessary();
+
+  void OnInstalled(BookmarkAppInstallationTask::Result result);
+
+  void CurrentInstallationFinished(const std::string& app_id);
+
   // WebContentsObserver
   void DidFinishLoad(content::RenderFrameHost* render_frame_host,
                      const GURL& validated_url) override;
@@ -58,10 +71,6 @@ class PendingBookmarkAppManager final : public web_app::PendingAppManager,
                    int error_code,
                    const base::string16& error_description) override;
 
-  void OnInstalled(BookmarkAppInstallationTask::Result result);
-
-  void CreateWebContentsIfNecessary();
-
   Profile* profile_;
 
   WebContentsFactory web_contents_factory_;
@@ -69,10 +78,13 @@ class PendingBookmarkAppManager final : public web_app::PendingAppManager,
 
   std::unique_ptr<content::WebContents> web_contents_;
 
-  InstallCallback current_install_callback_;
-  std::unique_ptr<AppInfo> current_install_info_;
+  std::unique_ptr<Installation> current_installation_;
   std::unique_ptr<BookmarkAppInstallationTask> current_installation_task_;
 
+  std::deque<std::unique_ptr<Installation>> installation_queue_;
+
+  base::WeakPtrFactory<PendingBookmarkAppManager> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(PendingBookmarkAppManager);
 };
 

chrome/browser/web_applications/extensions/pending_bookmark_app_manager_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "base/test/bind_test_util.h"
 #include "chrome/browser/extensions/test_extension_system.h"
 #include "chrome/browser/web_applications/components/pending_app_manager.h"
 #include "chrome/browser/web_applications/extensions/bookmark_app_shortcut_installation_task.h"
@@ -24,8 +25,21 @@ namespace extensions {
 
 namespace {
 
-const char kWebAppUrl[] = "https://foo.example";
-const char kWrongUrl[] = "https://bar.example";
+const char kFooWebAppUrl[] = "https://foo.example";
+const char kBarWebAppUrl[] = "https://bar.example";
+
+const char kWrongUrl[] = "https://foobar.example";
+
+web_app::PendingAppManager::AppInfo GetFooAppInfo() {
+  return web_app::PendingAppManager::AppInfo(
+      GURL(kFooWebAppUrl), web_app::PendingAppManager::LaunchContainer::kTab);
+}
+
+web_app::PendingAppManager::AppInfo GetBarAppInfo() {
+  return web_app::PendingAppManager::AppInfo(
+      GURL(kBarWebAppUrl),
+      web_app::PendingAppManager::LaunchContainer::kWindow);
+}
 
 }  // namespace
 
@@ -56,7 +70,17 @@ class TestBookmarkAppShortcutInstallationTask
 
 class PendingBookmarkAppManagerTest : public ChromeRenderViewHostTestHarness {
  public:
-  PendingBookmarkAppManagerTest() = default;
+  PendingBookmarkAppManagerTest()
+      : test_web_contents_creator_(base::BindRepeating(
+            &PendingBookmarkAppManagerTest::CreateTestWebContents,
+            base::Unretained(this))),
+        successful_installation_task_creator_(base::BindRepeating(
+            &PendingBookmarkAppManagerTest::CreateSuccessfulInstallationTask,
+            base::Unretained(this))),
+        failing_installation_task_creator_(base::BindRepeating(
+            &PendingBookmarkAppManagerTest::CreateFailingInstallationTask,
+            base::Unretained(this))) {}
+
   ~PendingBookmarkAppManagerTest() override = default;
 
   void SetUp() override {
@@ -97,6 +121,21 @@ class PendingBookmarkAppManagerTest : public ChromeRenderViewHostTestHarness {
  protected:
   void ResetResults() { install_succeeded_ = base::nullopt; }
 
+  const PendingBookmarkAppManager::WebContentsFactory&
+  test_web_contents_creator() {
+    return test_web_contents_creator_;
+  }
+
+  const PendingBookmarkAppManager::TaskFactory&
+  successful_installation_task_creator() {
+    return successful_installation_task_creator_;
+  }
+
+  const PendingBookmarkAppManager::TaskFactory&
+  failing_installation_task_creator() {
+    return failing_installation_task_creator_;
+  }
+
   content::WebContentsTester* web_contents_tester() {
     return web_contents_tester_;
   }
@@ -107,75 +146,152 @@ class PendingBookmarkAppManagerTest : public ChromeRenderViewHostTestHarness {
   content::WebContentsTester* web_contents_tester_ = nullptr;
   base::Optional<bool> install_succeeded_;
 
+  PendingBookmarkAppManager::WebContentsFactory test_web_contents_creator_;
+  PendingBookmarkAppManager::TaskFactory successful_installation_task_creator_;
+  PendingBookmarkAppManager::TaskFactory failing_installation_task_creator_;
+
   DISALLOW_COPY_AND_ASSIGN(PendingBookmarkAppManagerTest);
 };
 
 TEST_F(PendingBookmarkAppManagerTest, Install_Succeeds) {
   PendingBookmarkAppManager pending_app_manager(profile());
   pending_app_manager.SetFactoriesForTesting(
-      base::BindRepeating(&PendingBookmarkAppManagerTest::CreateTestWebContents,
-                          base::Unretained(this)),
-      base::BindRepeating(
-          &PendingBookmarkAppManagerTest::CreateSuccessfulInstallationTask,
-          base::Unretained(this)));
+      test_web_contents_creator(), successful_installation_task_creator());
 
   pending_app_manager.Install(
-      web_app::PendingAppManager::AppInfo(
-          GURL(kWebAppUrl), web_app::PendingAppManager::LaunchContainer::kTab),
+      GetFooAppInfo(),
       base::BindOnce(&PendingBookmarkAppManagerTest::InstallCallback,
                      base::Unretained(this)));
-  web_contents_tester()->NavigateAndCommit(GURL(kWebAppUrl));
-  web_contents_tester()->TestDidFinishLoad(GURL(kWebAppUrl));
+
+  base::RunLoop().RunUntilIdle();
+  web_contents_tester()->NavigateAndCommit(GURL(kFooWebAppUrl));
+  web_contents_tester()->TestDidFinishLoad(GURL(kFooWebAppUrl));
   EXPECT_TRUE(install_succeeded());
 }
 
 TEST_F(PendingBookmarkAppManagerTest, Install_SucceedsTwice) {
   PendingBookmarkAppManager pending_app_manager(profile());
   pending_app_manager.SetFactoriesForTesting(
-      base::BindRepeating(&PendingBookmarkAppManagerTest::CreateTestWebContents,
-                          base::Unretained(this)),
-      base::BindRepeating(
-          &PendingBookmarkAppManagerTest::CreateSuccessfulInstallationTask,
-          base::Unretained(this)));
+      test_web_contents_creator(), successful_installation_task_creator());
+
+  pending_app_manager.Install(
+      GetFooAppInfo(),
+      base::BindOnce(&PendingBookmarkAppManagerTest::InstallCallback,
+                     base::Unretained(this)));
+
+  base::RunLoop().RunUntilIdle();
+  web_contents_tester()->NavigateAndCommit(GURL(kFooWebAppUrl));
+  web_contents_tester()->TestDidFinishLoad(GURL(kFooWebAppUrl));
+  EXPECT_TRUE(install_succeeded());
+  ResetResults();
+
+  pending_app_manager.Install(
+      GetBarAppInfo(),
+      base::BindOnce(&PendingBookmarkAppManagerTest::InstallCallback,
+                     base::Unretained(this)));
+
+  base::RunLoop().RunUntilIdle();
+  web_contents_tester()->NavigateAndCommit(GURL(kBarWebAppUrl));
+  web_contents_tester()->TestDidFinishLoad(GURL(kBarWebAppUrl));
+  EXPECT_TRUE(install_succeeded());
+}
+
+TEST_F(PendingBookmarkAppManagerTest, Install_PendingSuccessfulTask) {
+  PendingBookmarkAppManager pending_app_manager(profile());
+  pending_app_manager.SetFactoriesForTesting(
+      test_web_contents_creator(), successful_installation_task_creator());
 
   pending_app_manager.Install(
-      web_app::PendingAppManager::AppInfo(
-          GURL(kWebAppUrl), web_app::PendingAppManager::LaunchContainer::kTab),
+      GetFooAppInfo(),
+      base::BindOnce(&PendingBookmarkAppManagerTest::InstallCallback,
+                     base::Unretained(this)));
+  pending_app_manager.Install(
+      GetBarAppInfo(),
       base::BindOnce(&PendingBookmarkAppManagerTest::InstallCallback,
                      base::Unretained(this)));
-  web_contents_tester()->NavigateAndCommit(GURL(kWebAppUrl));
-  web_contents_tester()->TestDidFinishLoad(GURL(kWebAppUrl));
+
+  // Finish the first install.
+  base::RunLoop().RunUntilIdle();
+  web_contents_tester()->NavigateAndCommit(GURL(kFooWebAppUrl));
+  web_contents_tester()->TestDidFinishLoad(GURL(kFooWebAppUrl));
   EXPECT_TRUE(install_succeeded());
   ResetResults();
 
+  // Finish the second install.
+  base::RunLoop().RunUntilIdle();
+  web_contents_tester()->NavigateAndCommit(GURL(kBarWebAppUrl));
+  web_contents_tester()->TestDidFinishLoad(GURL(kBarWebAppUrl));
+  EXPECT_TRUE(install_succeeded());
+}
+
+TEST_F(PendingBookmarkAppManagerTest, Install_PendingFailingTask) {
+  PendingBookmarkAppManager pending_app_manager(profile());
+  pending_app_manager.SetFactoriesForTesting(
+      test_web_contents_creator(), successful_installation_task_creator());
+
+  pending_app_manager.Install(
+      GetFooAppInfo(),
+      base::BindOnce(&PendingBookmarkAppManagerTest::InstallCallback,
+                     base::Unretained(this)));
   pending_app_manager.Install(
-      web_app::PendingAppManager::AppInfo(
-          GURL(kWebAppUrl), web_app::PendingAppManager::LaunchContainer::kTab),
+      GetBarAppInfo(),
       base::BindOnce(&PendingBookmarkAppManagerTest::InstallCallback,
                      base::Unretained(this)));
-  web_contents_tester()->NavigateAndCommit(GURL(kWebAppUrl));
-  web_contents_tester()->TestDidFinishLoad(GURL(kWebAppUrl));
+
+  // Fail the first install.
+  base::RunLoop().RunUntilIdle();
+  web_contents_tester()->NavigateAndCommit(GURL(kBarWebAppUrl));
+  web_contents_tester()->TestDidFinishLoad(GURL(kBarWebAppUrl));
+  EXPECT_FALSE(install_succeeded());
+  ResetResults();
+
+  // Finish the second install.
+  base::RunLoop().RunUntilIdle();
+  web_contents_tester()->NavigateAndCommit(GURL(kBarWebAppUrl));
+  web_contents_tester()->TestDidFinishLoad(GURL(kBarWebAppUrl));
+  EXPECT_TRUE(install_succeeded());
+}
+
+TEST_F(PendingBookmarkAppManagerTest, Install_ReentrantCallback) {
+  PendingBookmarkAppManager pending_app_manager(profile());
+  pending_app_manager.SetFactoriesForTesting(
+      test_web_contents_creator(), successful_installation_task_creator());
+
+  // Call install with a callback that tries to install another app.
+  pending_app_manager.Install(
+      GetFooAppInfo(),
+      base::BindLambdaForTesting([&](const std::string& app_id) {
+        InstallCallback(app_id);
+        pending_app_manager.Install(
+            GetBarAppInfo(),
+            base::BindOnce(&PendingBookmarkAppManagerTest::InstallCallback,
+                           base::Unretained(this)));
+      }));
+  // Finish the first install.
+  base::RunLoop().RunUntilIdle();
+  web_contents_tester()->NavigateAndCommit(GURL(kFooWebAppUrl));
+  web_contents_tester()->TestDidFinishLoad(GURL(kFooWebAppUrl));
+  EXPECT_TRUE(install_succeeded());
+  ResetResults();
+
+  base::RunLoop().RunUntilIdle();
+  web_contents_tester()->NavigateAndCommit(GURL(kBarWebAppUrl));
+  web_contents_tester()->TestDidFinishLoad(GURL(kBarWebAppUrl));
   EXPECT_TRUE(install_succeeded());
 }
 
 TEST_F(PendingBookmarkAppManagerTest, Install_FailsSameInstallPending) {
   PendingBookmarkAppManager pending_app_manager(profile());
   pending_app_manager.SetFactoriesForTesting(
-      base::BindRepeating(&PendingBookmarkAppManagerTest::CreateTestWebContents,
-                          base::Unretained(this)),
-      base::BindRepeating(
-          &PendingBookmarkAppManagerTest::CreateSuccessfulInstallationTask,
-          base::Unretained(this)));
+      test_web_contents_creator(), successful_installation_task_creator());
 
   pending_app_manager.Install(
-      web_app::PendingAppManager::AppInfo(
-          GURL(kWebAppUrl), web_app::PendingAppManager::LaunchContainer::kTab),
+      GetFooAppInfo(),
       base::BindOnce(&PendingBookmarkAppManagerTest::InstallCallback,
                      base::Unretained(this)));
 
   pending_app_manager.Install(
-      web_app::PendingAppManager::AppInfo(
-          GURL(kWebAppUrl), web_app::PendingAppManager::LaunchContainer::kTab),
+      GetFooAppInfo(),
       base::BindOnce(&PendingBookmarkAppManagerTest::InstallCallback,
                      base::Unretained(this)));
 
@@ -184,26 +300,23 @@ TEST_F(PendingBookmarkAppManagerTest, Install_FailsSameInstallPending) {
   ResetResults();
 
   // The original install should still be able to succeed.
-  web_contents_tester()->NavigateAndCommit(GURL(kWebAppUrl));
-  web_contents_tester()->TestDidFinishLoad(GURL(kWebAppUrl));
+  base::RunLoop().RunUntilIdle();
+  web_contents_tester()->NavigateAndCommit(GURL(kFooWebAppUrl));
+  web_contents_tester()->TestDidFinishLoad(GURL(kFooWebAppUrl));
   EXPECT_TRUE(install_succeeded());
 }
 
 TEST_F(PendingBookmarkAppManagerTest, Install_FailsLoadIncorrectURL) {
   PendingBookmarkAppManager pending_app_manager(profile());
   pending_app_manager.SetFactoriesForTesting(
-      base::BindRepeating(&PendingBookmarkAppManagerTest::CreateTestWebContents,
-                          base::Unretained(this)),
-      base::BindRepeating(
-          &PendingBookmarkAppManagerTest::CreateSuccessfulInstallationTask,
-          base::Unretained(this)));
+      test_web_contents_creator(), successful_installation_task_creator());
 
   pending_app_manager.Install(
-      web_app::PendingAppManager::AppInfo(
-          GURL(kWebAppUrl), web_app::PendingAppManager::LaunchContainer::kTab),
+      GetFooAppInfo(),
       base::BindOnce(&PendingBookmarkAppManagerTest::InstallCallback,
                      base::Unretained(this)));
 
+  base::RunLoop().RunUntilIdle();
   web_contents_tester()->NavigateAndCommit(GURL(kWrongUrl));
   web_contents_tester()->TestDidFinishLoad(GURL(kWrongUrl));
   EXPECT_FALSE(install_succeeded());