Fix IWA orphaned bundle cleanup

The enumeration of orphaned bundles works by subracting the set
of installed IWAs from the set of directories in the profile/iwa
directory. This does not consider that IWA updates are also stored
in the profile/iwa folder, which means that updates would be deleted
by the orphaned bundle cleanup.

Further, this CL fixes a problem with the creation of the list of
installed IWAs (which returns the full paths of the bundles, instead
of their base directories).

Bug: b/357629298
Change-Id: I91a6a7848b9f9dc54829d443f1f70837ade65e70
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5756734
Reviewed-by: Andrew Rayskiy <[email protected]>
Commit-Queue: Simon Hangl <[email protected]>
Reviewed-by: Robbie McElrath <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1339086}

chrome/browser/web_applications/isolated_web_apps/cleanup_orphaned_isolated_web_apps_command.cc

@@ -43,7 +43,19 @@ std::set<base::FilePath> RetrieveAllInstalledIsolatedWebAppsPaths(
         continue;
       }
 
-      isolated_web_apps_paths.insert(owned_bundle->GetPath(profile.GetPath()));
+      isolated_web_apps_paths.insert(
+          owned_bundle->GetPath(profile.GetPath()).DirName());
+
+      if (const auto& pending_update_info =
+              isolation_data->pending_update_info()) {
+        const auto* pending_update_location =
+            absl::get_if<IsolatedWebAppStorageLocation::OwnedBundle>(
+                &pending_update_info->location.variant());
+        if (pending_update_location) {
+          isolated_web_apps_paths.insert(
+              pending_update_location->GetPath(profile.GetPath()).DirName());
+        }
+      }
     }
   }
   return isolated_web_apps_paths;
@@ -67,8 +79,9 @@ std::set<base::FilePath> RetrieveAllIsolatedWebAppsDirectories(
 }
 
 bool DeleteOrphanedIsolatedWebApps(std::vector<base::FilePath> paths) {
-  return base::ranges::all_of(
-      paths, [](const base::FilePath& path) { return base::DeleteFile(path); });
+  return base::ranges::all_of(paths, [](const base::FilePath& path) {
+    return base::DeletePathRecursively(path);
+  });
 }
 
 base::expected<CleanupOrphanedIsolatedWebAppsCommandSuccess,
@@ -200,6 +213,7 @@ void CleanupOrphanedIsolatedWebAppsCommand::
   base::ranges::set_difference(isolated_web_apps_directories,
                                installed_isolated_web_apps_paths,
                                std::back_inserter(directories_to_delete));
+
   number_of_deleted_directories_ = directories_to_delete.size();
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE,

chrome/browser/web_applications/isolated_web_apps/isolated_web_app_update_manager_browsertest.cc

@@ -8,8 +8,11 @@
 #include <string_view>
 
 #include "base/base64.h"
+#include "base/check_deref.h"
+#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/functional/callback_helpers.h"
 #include "base/scoped_observation.h"
 #include "base/strings/strcat.h"
 #include "base/strings/string_util.h"
@@ -107,6 +110,14 @@ constexpr std::string_view kServiceWorkerScript = R"(
   });
 )";
 
+#if BUILDFLAG(IS_CHROMEOS_ASH)
+void CheckBundleExists(Profile* profile, const base::FilePath& directory) {
+  base::ScopedAllowBlockingForTesting allow_blocking;
+  EXPECT_TRUE(base::DirectoryExists(
+      CHECK_DEREF(profile).GetPath().Append(kIwaDirName).Append(directory)));
+}
+#endif  // BUILDFLAG(IS_CHROMEOS_ASH)
+
 class ServiceWorkerVersionStartedRunningWaiter
     : public content::ServiceWorkerContextObserver {
  public:
@@ -351,6 +362,68 @@ IN_PROC_BROWSER_TEST_F(IsolatedWebAppUpdateManagerBrowserTest,
   title_watcher.AlsoWaitForTitle(u"3.0.4");
   EXPECT_THAT(title_watcher.WaitAndGetTitle(), Eq(u"7.0.6"));
 }
+
+IN_PROC_BROWSER_TEST_F(IsolatedWebAppUpdateManagerBrowserTest,
+                       PendingUpdateDoesNotGetCleanedUp) {
+  profile()->GetPrefs()->SetList(
+      prefs::kIsolatedWebAppInstallForceList,
+      base::Value::List().Append(
+          update_server_mixin_.CreateForceInstallPolicyEntry(
+              GetWebBundleId())));
+
+  SessionStartupPref pref(SessionStartupPref::LAST);
+  SessionStartupPref::SetStartupPref(profile(), pref);
+
+  profile()->GetPrefs()->CommitPendingWrite();
+
+  web_app::WebAppTestInstallObserver(browser()->profile())
+      .BeginListeningAndWait({GetAppId()});
+
+  // Open the app to prevent the update from being applied.
+  OpenApp(GetAppId());
+  EXPECT_THAT(provider().ui_manager().GetNumWindowsForApp(GetAppId()), Eq(1ul));
+
+  AddUpdate();
+  EXPECT_THAT(provider().iwa_update_manager().DiscoverUpdatesNow(), Eq(1ul));
+
+  ASSERT_TRUE(base::test::RunUntil([this]() {
+    const WebApp* app = GetIsolatedWebApp(GetAppId());
+    return app->isolation_data()->pending_update_info().has_value();
+  }));
+
+  const auto& isolation_data = GetIsolatedWebApp(GetAppId())->isolation_data();
+  const auto& app_location =
+      base::FilePath(absl::get_if<IsolatedWebAppStorageLocation::OwnedBundle>(
+                         &isolation_data->location.variant())
+                         ->dir_name_ascii());
+  const auto& app_update_location = base::FilePath(
+      absl::get_if<IsolatedWebAppStorageLocation::OwnedBundle>(
+          &isolation_data->pending_update_info()->location.variant())
+          ->dir_name_ascii());
+
+  // Check that both IWA directories (currently running instance and the update)
+  // are there.
+  CheckBundleExists(profile(), app_location);
+  CheckBundleExists(profile(), app_update_location);
+
+  // Run the cleanup while both bundles are there.
+  base::test::TestFuture<
+      base::expected<CleanupOrphanedIsolatedWebAppsCommandSuccess,
+                     CleanupOrphanedIsolatedWebAppsCommandError>>
+      future;
+  provider().scheduler().CleanupOrphanedIsolatedApps(future.GetCallback());
+  const bool command_successful =
+      future
+          .Get<base::expected<CleanupOrphanedIsolatedWebAppsCommandSuccess,
+                              CleanupOrphanedIsolatedWebAppsCommandError>>()
+          .has_value();
+  ASSERT_TRUE(command_successful);
+
+  // Neither of the bundles should be deleted.
+  CheckBundleExists(profile(), app_location);
+  CheckBundleExists(profile(), app_update_location);
+}
+
 #endif  // !BUILDFLAG(IS_CHROMEOS_LACROS)
 
 class IsolatedWebAppUpdateManagerWithKeyRotationBrowserTest

chrome/browser/web_applications/isolated_web_apps/policy/isolated_web_app_policy_manager_ash_browsertest.cc

@@ -582,10 +582,13 @@ class CleanupOrphanedBundlesTest
   void SimulateOrphanedBundle(Profile* profile,
                               const std::string& bundle_directory) {
     base::ScopedAllowBlockingForTesting allow_blocking;
-    ASSERT_TRUE(base::CreateDirectory(CHECK_DEREF(profile)
-                                          .GetPath()
-                                          .Append(kIwaDirName)
-                                          .Append(bundle_directory)));
+    auto base_path = CHECK_DEREF(profile)
+                         .GetPath()
+                         .Append(kIwaDirName)
+                         .Append(bundle_directory);
+    ASSERT_TRUE(base::CreateDirectory(base_path));
+    ASSERT_TRUE(
+        base::WriteFile(base_path.Append("main.swbn"), "Sample content"));
   }
 
   bool CheckBundleDirectoryExists(Profile* profile,