Revert "[Extensions] Change install directory of .zip file installs."

This reverts commit 1a7cebd.

Reason for revert: This causes unexpected .zip file deletion due to incorrect extension_garbage_collector.cc logic. See https://crbug.com/1378775#c12 for details.

Original change's description:
> [Extensions] Change install directory of .zip file installs.
>
> Change the install directory to "UnpackedExtensions" subdir of the
> profile directory.
>
> I've put it behind a default enabled flag to give us the later option of
> an emergency rollback due to the complexity of the change and the fact
> that the change modifies files.
>
> Before this change we installed .zip file installed extensions to a
> unique subdir of base::DIR_TEMP. This could cause confusion when the
> temporary directory used for installation would be deleted by the OS
> (e.g. /tmp on Linux, `NSTemporaryDirectory` on Mac, and etc.) which are
> commonly cleared on varied timelines). This caused issues like the html
> file for a badge popup failing to be found at runtime and the extension
> disappearing from chromium on restart when it no longer could find the
> directory.
>
> After this change we install them to a non-temporary
> "UnpackedExtensions" subdir of the profile dir (at the same level as the
> "Extensions" dir where we install packed .crx and webstore installs).
>
> This allows the extension to persist until the user chooses to uninstall
> the (from .zip) extension.
>
> I kept the original install folder format of (example good.zip)
> ../<profile_dir>/UnpackedExtensions/good_XXXXXX/ where the Xs are a
> unique combination (for that folder) of alphanumeric characters but that
> was to be consistent more than anything else.
>
> Extra work had to be done with extension_garbage_collector.cc to ensure
> that if the directory deletion failed it would eventually come around
> and delete/cleanup the directory so it doesn't linger (because there
> is now a new install location).
>
> This does not cause the Extension to be synced.
>
> Low-Coverage-Reason: extension_garbage_collector.cc we don't seem to
> cover NOTREACHED() in unit tests.
>
> Bug: 1378775
> Change-Id: If793f363ec76bf6c36c7a77f8ebd8c6ad8c8e1d5
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4313024
> Auto-Submit: Justin Lulejian <jlulejian@chromium.org>
> Reviewed-by: David Bertoni <dbertoni@chromium.org>
> Commit-Queue: Justin Lulejian <jlulejian@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1124983}

Bug: 1378775
Change-Id: Ic60fb1694375b5eb302d2f6498f04ddd691acd26
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4477804
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Commit-Queue: Justin Lulejian <jlulejian@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1136022}

chrome/browser/extensions/api/developer_private/developer_private_api.cc

@@ -98,7 +98,6 @@
 #include "extensions/browser/warning_service.h"
 #include "extensions/browser/warning_service_factory.h"
 #include "extensions/browser/zipfile_installer.h"
-#include "extensions/common/extension_features.h"
 #include "extensions/common/extension_set.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/features/feature_developer_mode_only.h"
@@ -1456,18 +1455,9 @@ DeveloperPrivateInstallDroppedFileFunction::Run() {
 
   ExtensionService* service = GetExtensionService(browser_context());
   if (path.MatchesExtension(FILE_PATH_LITERAL(".zip"))) {
-    if (base::FeatureList::IsEnabled(
-            extensions_features::kExtensionsZipFileInstalledInProfileDir)) {
-      ZipFileInstaller::Create(GetExtensionFileTaskRunner(),
-                               MakeRegisterInExtensionServiceCallback(service))
-          ->InstallZipFileToUnpackedExtensionsDir(
-              path, service->unpacked_install_directory());
-    } else {
-      ZipFileInstaller::Create(GetExtensionFileTaskRunner(),
-                               MakeRegisterInExtensionServiceCallback(service))
-          ->InstallZipFileToTempDir(path);
-    }
-
+    ZipFileInstaller::Create(GetExtensionFileTaskRunner(),
+                             MakeRegisterInExtensionServiceCallback(service))
+        ->LoadFromZipFile(path);
   } else {
     auto prompt = std::make_unique<ExtensionInstallPrompt>(web_contents);
     scoped_refptr<CrxInstaller> crx_installer =

chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc

@@ -7,13 +7,11 @@
 #include <memory>
 #include <utility>
 
-#include "base/base_paths.h"
 #include "base/files/file_util.h"
 #include "base/functional/bind.h"
 #include "base/json/json_writer.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_refptr.h"
-#include "base/path_service.h"
 #include "base/scoped_observation.h"
 #include "base/stl_util.h"
 #include "base/strings/strcat.h"
@@ -273,10 +271,6 @@ class DeveloperPrivateApiUnitTest : public ExtensionServiceTestWithInstall {
   DeveloperPrivateApiUnitTest& operator=(const DeveloperPrivateApiUnitTest&) =
       delete;
 
-  // ExtensionServiceTestBase:
-  void SetUp() override;
-  void TearDown() override;
-
  protected:
   DeveloperPrivateApiUnitTest() {}
   ~DeveloperPrivateApiUnitTest() override {}
@@ -336,6 +330,10 @@ class DeveloperPrivateApiUnitTest : public ExtensionServiceTestWithInstall {
   }
 
  private:
+  // ExtensionServiceTestBase:
+  void SetUp() override;
+  void TearDown() override;
+
   // The browser (and accompanying window).
   std::unique_ptr<TestBrowserWindow> browser_window_;
   std::unique_ptr<Browser> browser_;
@@ -1962,42 +1960,7 @@ TEST_F(DeveloperPrivateApiUnitTest, ExtensionUpdatedEventOnPermissionsChange) {
       api::developer_private::EVENT_TYPE_PERMISSIONS_CHANGED));
 }
 
-class DeveloperPrivateApiZipFileUnitTest
-    : public DeveloperPrivateApiUnitTest,
-      public testing::WithParamInterface<bool> {
- public:
-  void SetUp() override {
-    DeveloperPrivateApiUnitTest::SetUp();
-    const bool kFeatureEnabled = GetParam();
-    feature_list_.InitWithFeatureState(
-        extensions_features::kExtensionsZipFileInstalledInProfileDir,
-        kFeatureEnabled);
-    if (kFeatureEnabled) {
-      expected_extension_install_directory_ =
-          service()->unpacked_install_directory();
-    } else {
-      base::FilePath dir_temp;
-      ASSERT_TRUE(base::PathService::Get(base::DIR_TEMP, &dir_temp));
-      expected_extension_install_directory_ = dir_temp;
-    }
-  }
-
- protected:
-  base::test::ScopedFeatureList scoped_feature_list_;
-  base::FilePath expected_extension_install_directory_;
-};
-
-INSTANTIATE_TEST_SUITE_P(
-    ,
-    DeveloperPrivateApiZipFileUnitTest,
-    // extensions_features::kExtensionsZipFileInstalledInProfileDir enabled.
-    testing::Bool(),
-    [](const testing::TestParamInfo<
-        DeveloperPrivateApiZipFileUnitTest::ParamType>& info) {
-      return info.param ? "ProfileDir" : "TempDir";
-    });
-
-TEST_P(DeveloperPrivateApiZipFileUnitTest, InstallDroppedFileZip) {
+TEST_F(DeveloperPrivateApiUnitTest, InstallDroppedFileZip) {
   base::FilePath zip_path = data_dir().AppendASCII("simple_empty.zip");
   extensions::ExtensionInstallUI::set_disable_ui_for_tests();
   ScopedTestDialogAutoConfirm auto_confirm(ScopedTestDialogAutoConfirm::ACCEPT);
@@ -2018,17 +1981,6 @@ TEST_P(DeveloperPrivateApiZipFileUnitTest, InstallDroppedFileZip) {
       observer.WaitForExtensionInstalled();
   ASSERT_TRUE(extension);
   EXPECT_EQ("Simple Empty Extension", extension->name());
-
-  // Expect extension install directory to be immediate subdir of expected
-  // unpacked install directory. E.g. /a/b/c/d == /a/b/c + /d.
-  EXPECT_EQ(extension->path(), expected_extension_install_directory_.Append(
-                                   extension->path().BaseName()));
-
-  // Expect extension install directory to exist and be named with the right
-  // prefix.
-  EXPECT_TRUE(base::PathExists(extension->path()));
-  EXPECT_TRUE(
-      extension->path().BaseName().AsUTF8Unsafe().starts_with("simple_empty"));
 }
 
 // Test developerPrivate.getUserSiteSettings.

chrome/browser/extensions/extension_assets_manager.cc

@@ -42,11 +42,10 @@ class ExtensionAssetsManagerImpl :  public ExtensionAssetsManager {
 
   void UninstallExtension(const std::string& id,
                           const std::string& profile_user_name,
-                          const base::FilePath& extensions_install_dir,
-                          const base::FilePath& extension_dir_to_delete,
+                          const base::FilePath& local_install_dir,
+                          const base::FilePath& extension_root,
                           const base::FilePath& profile_dir) override {
-    file_util::UninstallExtension(profile_dir, extensions_install_dir,
-                                  extension_dir_to_delete);
+    file_util::UninstallExtension(profile_dir, local_install_dir, id);
   }
 
  private:

chrome/browser/extensions/extension_assets_manager.h

@@ -41,14 +41,10 @@ class ExtensionAssetsManager {
       bool updates_from_webstore_or_empty_update_url) = 0;
 
   // Remove extension assets if it is not used by anyone else.
-  // `extensions_install_dir` is the path to where extensions of this type are
-  // being installed. E.g. "/path/to/Profile/Extensions".
-  // `extension_dir_to_delete` is the directory that should be deleted to
-  // uninstall the extension.
   virtual void UninstallExtension(const std::string& id,
                                   const std::string& profile_user_name,
-                                  const base::FilePath& extensions_install_dir,
-                                  const base::FilePath& extension_dir_to_delete,
+                                  const base::FilePath& local_install_dir,
+                                  const base::FilePath& extension_root,
                                   const base::FilePath& profile_dir) = 0;
 
  protected:

chrome/browser/extensions/extension_assets_manager_chromeos.cc

@@ -175,16 +175,15 @@ void ExtensionAssetsManagerChromeOS::InstallExtension(
 void ExtensionAssetsManagerChromeOS::UninstallExtension(
     const std::string& id,
     const std::string& profile_user_name,
-    const base::FilePath& extensions_install_dir,
-    const base::FilePath& extension_dir_to_delete,
+    const base::FilePath& local_install_dir,
+    const base::FilePath& extension_root,
     const base::FilePath& profile_dir) {
-  if (extensions_install_dir.IsParent(extension_dir_to_delete)) {
-    file_util::UninstallExtension(profile_dir, extensions_install_dir,
-                                  extension_dir_to_delete);
+  if (local_install_dir.IsParent(extension_root)) {
+    file_util::UninstallExtension(profile_dir, local_install_dir, id);
     return;
   }
 
-  if (GetSharedInstallDir().IsParent(extension_dir_to_delete)) {
+  if (GetSharedInstallDir().IsParent(extension_root)) {
     // In some test extensions installed outside local_install_dir emulate
     // previous behavior that just do nothing in this case.
     content::GetUIThreadTaskRunner({})->PostTask(

chrome/browser/extensions/extension_assets_manager_chromeos.h

@@ -51,8 +51,8 @@ class ExtensionAssetsManagerChromeOS : public ExtensionAssetsManager {
       bool updates_from_webstore_or_empty_update_url) override;
   void UninstallExtension(const std::string& id,
                           const std::string& profile_user_name,
-                          const base::FilePath& extensions_install_dir,
-                          const base::FilePath& extension_dir_to_delete,
+                          const base::FilePath& local_install_dir,
+                          const base::FilePath& extension_root,
                           const base::FilePath& profile_dir) override;
 
   // Return shared install dir.

chrome/browser/extensions/extension_garbage_collector.cc

@@ -37,7 +37,6 @@
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extension_util.h"
 #include "extensions/common/extension.h"
-#include "extensions/common/extension_features.h"
 #include "extensions/common/file_util.h"
 
 namespace extensions {
@@ -209,17 +208,6 @@ void ExtensionGarbageCollector::GarbageCollectExtensions() {
                          service->install_directory(), extension_paths))) {
     NOTREACHED();
   }
-
-  if (!base::FeatureList::IsEnabled(
-          extensions_features::kExtensionsZipFileInstalledInProfileDir)) {
-    return;
-  }
-  if (!GetExtensionFileTaskRunner()->PostTask(
-          FROM_HERE, base::BindOnce(&GarbageCollectExtensionsOnFileThread,
-                                    service->unpacked_install_directory(),
-                                    extension_paths))) {
-    NOTREACHED();
-  }
 }
 
 void ExtensionGarbageCollector::GarbageCollectIsolatedStorageIfNeeded() {

chrome/browser/extensions/extension_garbage_collector_unittest.cc

@@ -20,7 +20,6 @@
 #include "content/public/test/test_utils.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/pref_names.h"
-#include "extensions/common/extension_features.h"
 #include "ppapi/buildflags/buildflags.h"
 
 #if BUILDFLAG(ENABLE_PLUGINS)
@@ -46,8 +45,6 @@ class ExtensionGarbageCollectorUnitTest : public ExtensionServiceTestBase {
     // Wait for GarbageCollectExtensions task to complete.
     content::RunAllTasksUntilIdle();
   }
-
-  base::test::ScopedFeatureList scoped_feature_list_;
 };
 
 // Test that partially deleted extensions are cleaned up during startup.
@@ -82,41 +79,6 @@ TEST_F(ExtensionGarbageCollectorUnitTest, CleanupOnStartup) {
   ASSERT_FALSE(base::PathExists(extension_dir));
 }
 
-// Test that partially deleted unpacked extensions are cleaned up during
-// startup. This test is only relevant when unpacked extensions are installed in
-// the profile directory.
-TEST_F(ExtensionGarbageCollectorUnitTest, CleanupUnpackedOnStartup) {
-  feature_list_.InitAndEnableFeature(
-      extensions_features::kExtensionsZipFileInstalledInProfileDir);
-  const std::string kExtensionId = "lckcjklfapeiadkadngidmocpbkemckm";
-
-  InitPluginService();
-  InitializeGoodInstalledExtensionService(/*unpacked=*/true);
-
-  // Simulate that the extensions was partially deleted by clearing its pref.
-  {
-    // TODO(crbug.com/1378775): The preferences don't seem to get loaded
-    // appropriately so this seems to be a no-op.
-    ScopedDictPrefUpdate update(profile_->GetPrefs(), pref_names::kExtensions);
-    update->Remove(kExtensionId);
-  }
-
-  service_->Init();
-  GarbageCollectExtensions();
-
-  base::FileEnumerator dirs(unpacked_install_dir(),
-                            false,  // not recursive
-                            base::FileEnumerator::DIRECTORIES);
-
-  // We should have have zero extensions now.
-  EXPECT_TRUE(dirs.Next().empty());
-
-  // And unpacked extension dir should now be toast.
-  base::FilePath zipped_extension_dir =
-      unpacked_install_dir().AppendASCII("good_juKvIh");
-  ASSERT_FALSE(base::PathExists(zipped_extension_dir));
-}
-
 // Test that garbage collection doesn't delete anything while a crx is being
 // installed.
 TEST_F(ExtensionGarbageCollectorUnitTest, NoCleanupDuringInstall) {

chrome/browser/extensions/extension_service.cc

@@ -156,21 +156,6 @@ const char* const kObsoleteComponentExtensionIds[] = {
     "jcgeabjmjgoblfofpppfkcoakmfobdko",  // Video Player
 };
 
-// When uninstalling an extension determine if the extension's directory
-// should be deleted when uninstalling. Returns `true` iff extension is
-// unpacked and installed outside the unpacked extensions installations dir.
-// Example: packed extensions are always deleted. But unpacked extensions are
-// in a folder outside the profile dir are not deleted.
-bool SkipDeleteExtensionDir(const Extension& extension,
-                            const base::FilePath& profile_path) {
-  bool is_unpacked_location =
-      Manifest::IsUnpackedLocation(extension.location());
-  bool extension_dir_not_direct_subdir_of_unpacked_extensions_install_dir =
-      extension.path().DirName() !=
-      profile_path.AppendASCII(extensions::kUnpackedInstallDirectoryName);
-  return is_unpacked_location &&
-         extension_dir_not_direct_subdir_of_unpacked_extensions_install_dir;
-}
 }  // namespace
 
 // ExtensionService.
@@ -835,25 +820,13 @@ bool ExtensionService::UninstallExtension(
         base::BarrierClosure(num_tasks, std::move(done_callback));
   }
 
-  // Delete extensions in profile directory (from webstore, or from .crx), but
-  // do not delete unpacked in a folder outside the profile directory.
-  if (!SkipDeleteExtensionDir(*extension, profile_->GetPath())) {
-    // Extensions installed from webstore or .crx are versioned in subdirs so we
-    // delete the parent dir. Unpacked (installed from .zip rather than folder)
-    // are not versioned so we just delete the single installation directory.
-    base::FilePath deletion_dir =
-        is_unpacked_location ? extension->path() : extension->path().DirName();
-
-    // Tell the backend to start deleting installed extension on the file
-    // thread.
+  // Tell the backend to start deleting installed extensions on the file thread.
+  if (!is_unpacked_location) {
     if (!GetExtensionFileTaskRunner()->PostTaskAndReply(
             FROM_HERE,
             base::BindOnce(&ExtensionService::UninstallExtensionOnFileThread,
                            extension->id(), profile_->GetProfileUserName(),
-                           /*extensions_install_dir=*/
-                           is_unpacked_location ? unpacked_install_directory_
-                                                : install_directory_,
-                           /*extension_dir_to_delete=*/std::move(deletion_dir),
+                           install_directory_, extension->path(),
                            profile_->GetPath()),
             subtask_done_callback)) {
       NOTREACHED();
@@ -879,14 +852,13 @@ bool ExtensionService::UninstallExtension(
 void ExtensionService::UninstallExtensionOnFileThread(
     const std::string& id,
     const std::string& profile_user_name,
-    const base::FilePath& extensions_install_dir,
-    const base::FilePath& extension_dir_to_delete,
+    const base::FilePath& install_dir,
+    const base::FilePath& extension_path,
     const base::FilePath& profile_dir) {
   ExtensionAssetsManager* assets_manager =
       ExtensionAssetsManager::GetInstance();
-  assets_manager->UninstallExtension(id, profile_user_name,
-                                     extensions_install_dir,
-                                     extension_dir_to_delete, profile_dir);
+  assets_manager->UninstallExtension(id, profile_user_name, install_dir,
+                                     extension_path, profile_dir);
 }
 
 bool ExtensionService::IsExtensionEnabled(