iwa: Wait for ongoing updates to complete before serving requests

If an update is in the process of being applied, pause all requests to
the IWA until the update is applied. There are two cases where this is
relevant:
- Normally, we wait for all windows of an IWA to close, and only then
  kick-off the update apply process. However, if the user were to
  re-open an IWA window during the update apply process, we need to
  delay loading of the window contents until after the update has
  finished applying.
- On startup, if updates from the previous browser session are still
  pending (possibly because the user never closed the windows of the
  IWA), then we queue the pending updates to be applied. However, if
  either the user or the browser opens an IWA window while we apply
  updates, we need to wait for that to finish, similar to the case
  above.

                     currently disabled due to crbug.com/1479463.

Low-Coverage-Reason: There is a test for Ash, but the Lacros test is
Bug: 1444407
Change-Id: I0fdf9579e72735b42dd3923ed6bfd03888b55488
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4831295
Reviewed-by: Robbie McElrath <rmcelrath@chromium.org>
Commit-Queue: Christian Flach <cmfcmf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1194079}

chrome/browser/ui/web_applications/test/isolated_web_app_builder.cc

@@ -25,7 +25,7 @@ constexpr base::StringPiece kTestManifest = R"({
       "version": "$2",
       "id": "/",
       "scope": "/",
-      "start_url": "/",
+      "start_url": "/index.html",
       "display": "standalone",
       "icons": [
         {

chrome/browser/ui/web_applications/test/isolated_web_app_test_utils.cc

@@ -67,8 +67,9 @@ Browser* IsolatedWebAppBrowserTestHarness::GetBrowserFromFrame(
 }
 
 content::RenderFrameHost* IsolatedWebAppBrowserTestHarness::OpenApp(
-    const AppId& app_id) {
-  return OpenIsolatedWebApp(profile(), app_id);
+    const AppId& app_id,
+    base::StringPiece path) {
+  return OpenIsolatedWebApp(profile(), app_id, path);
 }
 
 content::RenderFrameHost*
@@ -116,13 +117,14 @@ IsolatedWebAppUrlInfo InstallDevModeProxyIsolatedWebApp(
 }
 
 content::RenderFrameHost* OpenIsolatedWebApp(Profile* profile,
-                                             const AppId& app_id) {
+                                             const AppId& app_id,
+                                             base::StringPiece path) {
   WebAppRegistrar& registrar =
       WebAppProvider::GetForWebApps(profile)->registrar_unsafe();
   const WebApp* app = registrar.GetAppById(app_id);
   EXPECT_TRUE(app);
 
-  NavigateParams params(profile, app->start_url(),
+  NavigateParams params(profile, app->start_url().Resolve(path),
                         ui::PAGE_TRANSITION_GENERATED);
   params.app_id = app->app_id();
   params.window_action = NavigateParams::SHOW_WINDOW;

chrome/browser/ui/web_applications/test/isolated_web_app_test_utils.h

@@ -51,7 +51,8 @@ class IsolatedWebAppBrowserTestHarness : public WebAppControllerBrowserTest {
       const base::FilePath::StringPieceType& chrome_test_data_relative_root);
   IsolatedWebAppUrlInfo InstallDevModeProxyIsolatedWebApp(
       const url::Origin& origin);
-  content::RenderFrameHost* OpenApp(const AppId& app_id);
+  content::RenderFrameHost* OpenApp(const AppId& app_id,
+                                    base::StringPiece path = "");
   content::RenderFrameHost* NavigateToURLInNewTab(
       Browser* window,
       const GURL& url,
@@ -71,7 +72,8 @@ IsolatedWebAppUrlInfo InstallDevModeProxyIsolatedWebApp(
     const url::Origin& proxy_origin);
 
 content::RenderFrameHost* OpenIsolatedWebApp(Profile* profile,
-                                             const AppId& app_id);
+                                             const AppId& app_id,
+                                             base::StringPiece path = "");
 
 void CreateIframe(content::RenderFrameHost* parent_frame,
                   const std::string& iframe_id,

chrome/browser/web_applications/isolated_web_apps/isolated_web_app_update_manager.cc

@@ -7,20 +7,24 @@
 #include <memory>
 #include <type_traits>
 
+#include "base/callback_list.h"
 #include "base/check.h"
 #include "base/containers/circular_deque.h"
 #include "base/containers/cxx20_erase_map.h"
 #include "base/containers/unique_ptr_adapters.h"
 #include "base/feature_list.h"
 #include "base/functional/bind.h"
+#include "base/location.h"
 #include "base/memory/raw_ref.h"
 #include "base/memory/weak_ptr.h"
 #include "base/ranges/algorithm.h"
 #include "base/scoped_observation.h"
+#include "base/task/sequenced_task_runner.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "base/types/expected.h"
 #include "base/values.h"
+#include "chrome/browser/profiles/keep_alive/profile_keep_alive_types.h"
 #include "chrome/browser/profiles/keep_alive/scoped_profile_keep_alive.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/web_applications/isolated_web_apps/isolated_web_app_apply_update_command.h"
@@ -38,6 +42,7 @@
 #include "chrome/browser/web_applications/web_app_registrar.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/pref_names.h"
+#include "components/keep_alive_registry/keep_alive_types.h"
 #include "components/keep_alive_registry/scoped_keep_alive.h"
 #include "components/prefs/pref_service.h"
 #include "components/web_package/signed_web_bundles/signed_web_bundle_id.h"
@@ -94,7 +99,26 @@ void IsolatedWebAppUpdateManager::Start() {
                  << web_app.start_url();
       continue;
     }
-    CreateUpdateApplyWaiter(*url_info);
+    // During startup of the `IsolatedWebAppUpdateManager`, we do not use
+    // `IsolatedWebAppUpdateApplyWaiter`s to wait for all windows to close
+    // before applying the update. Instead, we schedule the update apply tasks
+    // directly (but do not start them yet). These tasks will be started one
+    // after the other eventually (see the `BEST_EFFORT` call below), or via
+    // `MaybeWaitUntilPendingUpdateIsApplied` if a window for an to-be-updated
+    // app is opened.
+    //
+    // At this point, it is guaranteed that no IWA window has loaded, since the
+    // `IsolatedWebAppURLLoaderFactory` can only create `URLLoader`s for IWAs
+    // after the `WebAppProvider` has fully started, and this code runs as part
+    // of the startup process of the `WebAppProvider`.
+    task_queue_.Push(std::make_unique<IsolatedWebAppUpdateApplyTask>(
+        *url_info,
+        std::make_unique<ScopedKeepAlive>(
+            KeepAliveOrigin::ISOLATED_WEB_APP_UPDATE,
+            KeepAliveRestartOption::DISABLED),
+        std::make_unique<ScopedProfileKeepAlive>(
+            &*profile_, ProfileKeepAliveOrigin::kIsolatedWebAppUpdate),
+        provider_->scheduler()));
   }
 
   content::GetUIThreadTaskRunner({base::TaskPriority::BEST_EFFORT})
@@ -149,6 +173,28 @@ base::Value IsolatedWebAppUpdateManager::AsDebugValue() const {
           .Set("update_apply_waiters", std::move(update_apply_waiters)));
 }
 
+bool IsolatedWebAppUpdateManager::IsUpdateBeingApplied(
+    base::PassKey<IsolatedWebAppURLLoaderFactory>,
+    const AppId app_id) const {
+  return task_queue_.IsUpdateApplyTaskQueued(app_id);
+}
+
+void IsolatedWebAppUpdateManager::PrioritizeUpdateAndWait(
+    base::PassKey<IsolatedWebAppURLLoaderFactory>,
+    const AppId& app_id,
+    base::OnceClosure callback) {
+  bool task_has_started =
+      task_queue_.EnsureQueuedUpdateApplyTaskHasStarted(app_id);
+  if (task_has_started) {
+    on_update_finished_callbacks_
+        .try_emplace(app_id, std::make_unique<base::OnceCallbackList<void()>>())
+        .first->second->AddUnsafe(std::move(callback));
+  } else {
+    base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
+        FROM_HERE, std::move(callback));
+  }
+}
+
 void IsolatedWebAppUpdateManager::SetEnableAutomaticUpdatesForTesting(
     bool automatic_updates_enabled) {
   CHECK(!has_started_);
@@ -302,6 +348,15 @@ void IsolatedWebAppUpdateManager::OnUpdateApplyWaiterFinished(
 void IsolatedWebAppUpdateManager::OnUpdateApplyTaskCompleted(
     std::unique_ptr<IsolatedWebAppUpdateApplyTask> task,
     IsolatedWebAppUpdateApplyTask::CompletionStatus status) {
+  auto callbacks_it =
+      on_update_finished_callbacks_.find(task->url_info().app_id());
+  if (callbacks_it != on_update_finished_callbacks_.end()) {
+    callbacks_it->second->Notify();
+    if (callbacks_it->second->empty()) {
+      on_update_finished_callbacks_.erase(callbacks_it);
+    }
+  }
+
   task_queue_.MaybeStartNextTask();
 }
 
@@ -349,6 +404,22 @@ void IsolatedWebAppUpdateManager::TaskQueue::Clear() {
   update_apply_tasks_.clear();
 }
 
+bool IsolatedWebAppUpdateManager::TaskQueue::
+    EnsureQueuedUpdateApplyTaskHasStarted(const AppId& app_id) {
+  auto task_it =
+      base::ranges::find_if(update_apply_tasks_, [&app_id](const auto& task) {
+        return task->url_info().app_id() == app_id;
+      });
+  if (task_it == update_apply_tasks_.end()) {
+    return false;
+  }
+
+  if (!task_it->get()->has_started()) {
+    StartUpdateApplyTask(task_it->get());
+  }
+  return true;
+}
+
 void IsolatedWebAppUpdateManager::TaskQueue::ClearNonStartedTasksOfApp(
     const AppId& app_id) {
   base::EraseIf(update_discovery_tasks_, [&app_id](const auto& task) {
@@ -377,6 +448,13 @@ void IsolatedWebAppUpdateManager::TaskQueue::MaybeStartNextTask() {
   }
 }
 
+bool IsolatedWebAppUpdateManager::TaskQueue::IsUpdateApplyTaskQueued(
+    const AppId& app_id) const {
+  return base::ranges::any_of(update_apply_tasks_, [&app_id](const auto& task) {
+    return task->url_info().app_id() == app_id;
+  });
+}
+
 void IsolatedWebAppUpdateManager::TaskQueue::StartUpdateDiscoveryTask(
     IsolatedWebAppUpdateDiscoveryTask* task_ptr) {
   task_ptr->Start(base::BindOnce(

chrome/browser/web_applications/isolated_web_apps/isolated_web_app_update_manager.h

@@ -7,16 +7,19 @@
 
 #include <memory>
 
+#include "base/callback_list.h"
 #include "base/containers/circular_deque.h"
 #include "base/containers/flat_map.h"
 #include "base/files/file_error_or.h"
 #include "base/files/file_path.h"
+#include "base/functional/callback_forward.h"
 #include "base/memory/raw_ptr.h"
 #include "base/memory/raw_ref.h"
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observation.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
+#include "base/types/pass_key.h"
 #include "base/values.h"
 #include "chrome/browser/web_applications/isolated_web_apps/isolated_web_app_update_apply_task.h"
 #include "chrome/browser/web_applications/isolated_web_apps/isolated_web_app_update_apply_waiter.h"
@@ -34,6 +37,7 @@ class SignedWebBundleId;
 namespace web_app {
 
 class IsolatedWebAppUrlInfo;
+class IsolatedWebAppURLLoaderFactory;
 class WebAppProvider;
 
 namespace {
@@ -69,6 +73,30 @@ class IsolatedWebAppUpdateManager : public WebAppInstallManagerObserver {
 
   base::Value AsDebugValue() const;
 
+  // Returns `true` if an update for the provided `app_id` is currently being
+  // applied or scheduled to be applied soon.
+  //
+  // Use of this method should be limited to the
+  // `IsolatedWebAppURLLoaderFactory`. If you have a different use case, please
+  // talk to iwa-dev@chromium.org first.
+  bool IsUpdateBeingApplied(base::PassKey<IsolatedWebAppURLLoaderFactory>,
+                            const AppId app_id) const;
+
+  // Starts an already scheduled update apply task for the provided `app_id`, if
+  // it is queued but not already running. This happens regardless of whether
+  // other update tasks are already running, and may therefore cause the task to
+  // run concurrently with another running update task.
+  //
+  // `callback` will be run once the update apply task for the provided `app_id`
+  // finishes.
+  //
+  // Use of this method should be limited to the
+  // `IsolatedWebAppURLLoaderFactory`. If you have a different use case, please
+  // talk to iwa-dev@chromium.org first.
+  void PrioritizeUpdateAndWait(base::PassKey<IsolatedWebAppURLLoaderFactory>,
+                               const AppId& app_id,
+                               base::OnceClosure callback);
+
   void SetEnableAutomaticUpdatesForTesting(bool automatic_updates_enabled);
 
   // `WebAppInstallManagerObserver`:
@@ -86,10 +114,13 @@ class IsolatedWebAppUpdateManager : public WebAppInstallManagerObserver {
  private:
   // This queue manages update discovery and apply tasks. Tasks can be added to
   // the queue via its `Push` methods. The queue will never start a new task on
-  // its own. Tasks can be started via `MaybeStartNextTask`; only one task is
-  // scheduled to run at the same time, with update apply tasks having
+  // its own. Tasks can be started via `MaybeStartNextTask`; normally, only one
+  // task is scheduled to run at the same time, with update apply tasks having
   // precedence over update discovery tasks. This is mainly to conserve
-  // resources (because each update task requires a `WebContents`).
+  // resources (because each update task requires a `WebContents`). However,
+  // queued update apply tasks that are explicitly started via
+  // `EnsureQueuedUpdateApplyTaskHasStarted` will run concurrently with other
+  // potentially running tasks.
   class TaskQueue {
    public:
     explicit TaskQueue(IsolatedWebAppUpdateManager& update_manager);
@@ -106,6 +137,12 @@ class IsolatedWebAppUpdateManager : public WebAppInstallManagerObserver {
     void Push(std::unique_ptr<IsolatedWebAppUpdateApplyTask> task);
     void Clear();
 
+    // If an `IsolatedWebAppUpdateApplyTask` for the `app_id` is queued, start
+    // it immediately, even if other tasks are currently running. Returns
+    // `false` if a task for this `app_id` is neither queued nor running,
+    // returns `true` otherwise.
+    bool EnsureQueuedUpdateApplyTaskHasStarted(const AppId& app_id);
+
     // Removes all tasks for the provided `app_id` that haven't yet started from
     // the queue.
     //
@@ -118,6 +155,8 @@ class IsolatedWebAppUpdateManager : public WebAppInstallManagerObserver {
     // update apply over update discovery tasks.
     void MaybeStartNextTask();
 
+    bool IsUpdateApplyTaskQueued(const AppId& app_id) const;
+
    private:
     void StartUpdateDiscoveryTask(IsolatedWebAppUpdateDiscoveryTask* task_ptr);
 
@@ -190,6 +229,11 @@ class IsolatedWebAppUpdateManager : public WebAppInstallManagerObserver {
   base::flat_map<AppId, std::unique_ptr<IsolatedWebAppUpdateApplyWaiter>>
       update_apply_waiters_;
 
+  // Callbacks that are run once an update apply task for a given app id has
+  // finished (successfully or unsuccessfully).
+  base::flat_map<AppId, std::unique_ptr<base::OnceCallbackList<void()>>>
+      on_update_finished_callbacks_;
+
   base::ScopedObservation<WebAppInstallManager, WebAppInstallManagerObserver>
       install_manager_observation_{this};
   base::WeakPtrFactory<IsolatedWebAppUpdateManager> weak_factory_{this};