[Extension] Implement ExtensionSystem::InstallUpdate.

Note that the signature of InstallUpdate is changed to match with
CrxInstaller::InstallUnpackedCrx.

Bug: 722942
Change-Id: I789c6277458447976d24aa7ba5dcced670c8cc11
Reviewed-on: https://chromium-review.googlesource.com/765095
Commit-Queue: Minh Nguyen <mxnguyen@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521481}

chrome/browser/extensions/crx_installer.cc

@@ -256,6 +256,45 @@ void CrxInstaller::InstallWebApp(const WebApplicationInfo& web_app) {
     NOTREACHED();
 }
 
+void CrxInstaller::UpdateExtensionFromUnpackedCrx(
+    const std::string& extension_id,
+    const std::string& public_key,
+    const base::FilePath& unpacked_dir) {
+  ExtensionService* service = service_weak_.get();
+  if (!service || service->browser_terminating())
+    return;
+
+  const Extension* extension = service->GetInstalledExtension(extension_id);
+  if (!extension) {
+    LOG(WARNING) << "Will not update extension " << extension_id
+                 << " because it is not installed";
+    if (delete_source_)
+      temp_dir_ = unpacked_dir;
+    if (installer_callback_.is_null()) {
+      installer_task_runner_->PostTask(
+          FROM_HERE, base::BindOnce(&CrxInstaller::CleanupTempFiles, this));
+    } else {
+      installer_task_runner_->PostTaskAndReply(
+          FROM_HERE, base::BindOnce(&CrxInstaller::CleanupTempFiles, this),
+          base::BindOnce(
+              base::BindOnce(std::move(installer_callback_), false)));
+    }
+    return;
+  }
+
+  expected_id_ = extension_id;
+  install_source_ = extension->location();
+  install_cause_ = extension_misc::INSTALL_CAUSE_UPDATE;
+  InitializeCreationFlagsForUpdate(extension, Extension::NO_FLAGS);
+
+  const ExtensionPrefs* extension_prefs =
+      ExtensionPrefs::Get(service->GetBrowserContext());
+  DCHECK(extension_prefs);
+  set_do_not_sync(extension_prefs->DoNotSync(extension_id));
+
+  InstallUnpackedCrx(extension_id, public_key, unpacked_dir);
+}
+
 void CrxInstaller::ConvertWebAppOnFileThread(
     const WebApplicationInfo& web_app) {
   scoped_refptr<Extension> extension(ConvertWebAppToExtension(
@@ -703,6 +742,34 @@ void CrxInstaller::OnInstallPromptDone(ExtensionInstallPrompt::Result result) {
   Release();  // balanced in ConfirmInstall() or ConfirmReEnable().
 }
 
+void CrxInstaller::InitializeCreationFlagsForUpdate(const Extension* extension,
+                                                    const int initial_flags) {
+  DCHECK(extension);
+
+  creation_flags_ = initial_flags;
+
+  // If the extension was installed from or has migrated to the webstore, or
+  // its auto-update URL is from the webstore, treat it as a webstore install.
+  // Note that we ignore some older extensions with blank auto-update URLs
+  // because we are mostly concerned with restrictions on NaCl extensions,
+  // which are newer.
+  if (extension->from_webstore() || ManifestURL::UpdatesFromGallery(extension))
+    creation_flags_ |= Extension::FROM_WEBSTORE;
+
+  // Bookmark apps being updated is kind of a contradiction, but that's because
+  // we mark the default apps as bookmark apps, and they're hosted in the web
+  // store, thus they can get updated. See http://crbug.com/101605 for more
+  // details.
+  if (extension->from_bookmark())
+    creation_flags_ |= Extension::FROM_BOOKMARK;
+
+  if (extension->was_installed_by_default())
+    creation_flags_ |= Extension::WAS_INSTALLED_BY_DEFAULT;
+
+  if (extension->was_installed_by_oem())
+    creation_flags_ |= Extension::WAS_INSTALLED_BY_OEM;
+}
+
 void CrxInstaller::UpdateCreationFlagsAndCompleteInstall() {
   creation_flags_ = extension()->creation_flags() | Extension::REQUIRE_KEY;
   // If the extension was already installed and had file access, also grant file

chrome/browser/extensions/crx_installer.h

@@ -119,8 +119,19 @@ class CrxInstaller : public SandboxedUnpackerClient {
   // Convert the specified web app into an extension and install it.
   void InstallWebApp(const WebApplicationInfo& web_app);
 
+  // Update the extension |extension_id| with the unpacked crx in
+  // |unpacked_dir|.
+  // If |delete_source_| is true, |unpacked_dir| will be removed at the end of
+  // the update.
+  void UpdateExtensionFromUnpackedCrx(const std::string& extension_id,
+                                      const std::string& public_key,
+                                      const base::FilePath& unpacked_dir);
+
   void OnInstallPromptDone(ExtensionInstallPrompt::Result result);
 
+  void InitializeCreationFlagsForUpdate(const Extension* extension,
+                                        const int initial_flags);
+
   int creation_flags() const { return creation_flags_; }
   void set_creation_flags(int val) { creation_flags_ = val; }
 

chrome/browser/extensions/extension_service.cc

@@ -586,7 +586,7 @@ bool ExtensionService::UpdateExtension(const extensions::CRXFileInfo& file,
 
     return false;
   }
-
+  // Either |pending_extension_info| or |extension| or both must not be null.
   scoped_refptr<CrxInstaller> installer(CrxInstaller::CreateSilent(this));
   installer->set_expected_id(id);
   installer->set_expected_hash(file.expected_hash);
@@ -611,39 +611,25 @@ bool ExtensionService::UpdateExtension(const extensions::CRXFileInfo& file,
     creation_flags = pending_extension_info->creation_flags();
     if (pending_extension_info->mark_acknowledged())
       external_install_manager_->AcknowledgeExternalExtension(id);
-  } else if (extension) {
+    // If the extension was installed from or has migrated to the webstore, or
+    // its auto-update URL is from the webstore, treat it as a webstore install.
+    // Note that we ignore some older extensions with blank auto-update URLs
+    // because we are mostly concerned with restrictions on NaCl extensions,
+    // which are newer.
+    if (!extension && extension_urls::IsWebstoreUpdateUrl(
+                          pending_extension_info->update_url()))
+      creation_flags |= Extension::FROM_WEBSTORE;
+  } else {
+    // |extension| must not be null.
     installer->set_install_source(extension->location());
   }
-  // If the extension was installed from or has migrated to the webstore, or
-  // its auto-update URL is from the webstore, treat it as a webstore install.
-  // Note that we ignore some older extensions with blank auto-update URLs
-  // because we are mostly concerned with restrictions on NaCl extensions,
-  // which are newer.
-  if ((extension && extension->from_webstore()) ||
-      (extension && extensions::ManifestURL::UpdatesFromGallery(extension)) ||
-      (!extension && extension_urls::IsWebstoreUpdateUrl(
-           pending_extension_info->update_url()))) {
-    creation_flags |= Extension::FROM_WEBSTORE;
-  }
-
-  // Bookmark apps being updated is kind of a contradiction, but that's because
-  // we mark the default apps as bookmark apps, and they're hosted in the web
-  // store, thus they can get updated. See http://crbug.com/101605 for more
-  // details.
-  if (extension && extension->from_bookmark())
-    creation_flags |= Extension::FROM_BOOKMARK;
-
-  if (extension && extension->was_installed_by_default())
-    creation_flags |= Extension::WAS_INSTALLED_BY_DEFAULT;
-
-  if (extension && extension->was_installed_by_oem())
-    creation_flags |= Extension::WAS_INSTALLED_BY_OEM;
 
-  if (extension)
+  if (extension) {
+    installer->InitializeCreationFlagsForUpdate(extension, creation_flags);
     installer->set_do_not_sync(extension_prefs_->DoNotSync(id));
-
-  installer->set_creation_flags(creation_flags);
-
+  } else {
+    installer->set_creation_flags(creation_flags);
+  }
   installer->set_delete_source(file_ownership_passed);
   installer->set_install_cause(extension_misc::INSTALL_CAUSE_UPDATE);
   installer->InstallCrxFile(file);

chrome/browser/extensions/extension_system_impl.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/extensions/chrome_app_sorting.h"
 #include "chrome/browser/extensions/chrome_content_verifier_delegate.h"
 #include "chrome/browser/extensions/component_loader.h"
+#include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/extension_error_reporter.h"
 #include "chrome/browser/extensions/extension_garbage_collector.h"
 #include "chrome/browser/extensions/extension_management.h"
@@ -432,10 +433,21 @@ std::unique_ptr<ExtensionSet> ExtensionSystemImpl::GetDependentExtensions(
       extension);
 }
 
-void ExtensionSystemImpl::InstallUpdate(const std::string& extension_id,
-                                        const base::FilePath& temp_dir) {
-  NOTREACHED() << "Not yet implemented";
-  base::DeleteFile(temp_dir, true /* recursive */);
+void ExtensionSystemImpl::InstallUpdate(
+    const std::string& extension_id,
+    const std::string& public_key,
+    const base::FilePath& unpacked_dir,
+    InstallUpdateCallback install_update_callback) {
+  DCHECK(!install_update_callback.is_null());
+
+  ExtensionService* service = extension_service();
+  DCHECK(service);
+
+  scoped_refptr<CrxInstaller> installer = CrxInstaller::CreateSilent(service);
+  installer->set_delete_source(true);
+  installer->set_installer_callback(std::move(install_update_callback));
+  installer->UpdateExtensionFromUnpackedCrx(extension_id, public_key,
+                                            unpacked_dir);
 }
 
 void ExtensionSystemImpl::RegisterExtensionWithRequestContexts(

chrome/browser/extensions/extension_system_impl.h

@@ -40,6 +40,8 @@ class ValueStoreFactoryImpl;
 // but with a shared instance for incognito) keeps the common services.
 class ExtensionSystemImpl : public ExtensionSystem {
  public:
+  using InstallUpdateCallback = ExtensionSystem::InstallUpdateCallback;
+
   explicit ExtensionSystemImpl(Profile* profile);
   ~ExtensionSystemImpl() override;
 
@@ -73,7 +75,9 @@ class ExtensionSystemImpl : public ExtensionSystem {
   std::unique_ptr<ExtensionSet> GetDependentExtensions(
       const Extension* extension) override;
   void InstallUpdate(const std::string& extension_id,
-                     const base::FilePath& temp_dir) override;
+                     const std::string& public_key,
+                     const base::FilePath& unpacked_dir,
+                     InstallUpdateCallback install_update_callback) override;
 
  private:
   friend class ExtensionSystemSharedFactory;

chrome/browser/extensions/test_extension_system.cc

@@ -136,8 +136,11 @@ std::unique_ptr<ExtensionSet> TestExtensionSystem::GetDependentExtensions(
       extension);
 }
 
-void TestExtensionSystem::InstallUpdate(const std::string& extension_id,
-                                        const base::FilePath& temp_dir) {
+void TestExtensionSystem::InstallUpdate(
+    const std::string& extension_id,
+    const std::string& public_key,
+    const base::FilePath& temp_dir,
+    InstallUpdateCallback install_update_callback) {
   NOTREACHED();
 }
 

chrome/browser/extensions/test_extension_system.h

@@ -33,6 +33,7 @@ class TestValueStoreFactory;
 // Test ExtensionSystem, for use with TestingProfile.
 class TestExtensionSystem : public ExtensionSystem {
  public:
+  using InstallUpdateCallback = ExtensionSystem::InstallUpdateCallback;
   explicit TestExtensionSystem(Profile* profile);
   ~TestExtensionSystem() override;
 
@@ -68,7 +69,9 @@ class TestExtensionSystem : public ExtensionSystem {
   std::unique_ptr<ExtensionSet> GetDependentExtensions(
       const Extension* extension) override;
   void InstallUpdate(const std::string& extension_id,
-                     const base::FilePath& temp_dir) override;
+                     const std::string& public_key,
+                     const base::FilePath& temp_dir,
+                     InstallUpdateCallback install_update_callback) override;
 
   // Note that you probably want to use base::RunLoop().RunUntilIdle() right
   // after this to run all the accumulated tasks.

extensions/browser/BUILD.gn

@@ -557,7 +557,9 @@ source_set("unit_tests") {
     "requirements_checker_unittest.cc",
     "runtime_data_unittest.cc",
     "sandboxed_unpacker_unittest.cc",
+    "updater/extension_installer_unittest.cc",
     "updater/safe_manifest_parser_unittest.cc",
+    "updater/update_data_provider_unittest.cc",
     "updater/update_service_unittest.cc",
     "value_store/leveldb_scoped_database_unittest.cc",
     "value_store/leveldb_value_store_unittest.cc",

extensions/browser/extension_system.h

@@ -132,7 +132,9 @@ class ExtensionSystem : public KeyedService {
   // transferred and implementors of this function are responsible for cleaning
   // it up on errors, etc.
   virtual void InstallUpdate(const std::string& extension_id,
-                             const base::FilePath& unpacked_dir) = 0;
+                             const std::string& public_key,
+                             const base::FilePath& unpacked_dir,
+                             InstallUpdateCallback install_update_callback) = 0;
 };
 
 }  // namespace extensions

extensions/browser/mock_extension_system.cc

@@ -76,8 +76,11 @@ std::unique_ptr<ExtensionSet> MockExtensionSystem::GetDependentExtensions(
   return std::unique_ptr<ExtensionSet>();
 }
 
-void MockExtensionSystem::InstallUpdate(const std::string& extension_id,
-                                        const base::FilePath& temp_dir) {
+void MockExtensionSystem::InstallUpdate(
+    const std::string& extension_id,
+    const std::string& public_key,
+    const base::FilePath& temp_dir,
+    InstallUpdateCallback install_update_callback) {
   NOTREACHED();
 }
 

extensions/browser/mock_extension_system.h

@@ -21,6 +21,8 @@ namespace extensions {
 // with the MockExtensionSystemFactory below.
 class MockExtensionSystem : public ExtensionSystem {
  public:
+  using InstallUpdateCallback = ExtensionSystem::InstallUpdateCallback;
+
   explicit MockExtensionSystem(content::BrowserContext* context);
   ~MockExtensionSystem() override;
 
@@ -44,7 +46,9 @@ class MockExtensionSystem : public ExtensionSystem {
   std::unique_ptr<ExtensionSet> GetDependentExtensions(
       const Extension* extension) override;
   void InstallUpdate(const std::string& extension_id,
-                     const base::FilePath& temp_dir) override;
+                     const std::string& public_key,
+                     const base::FilePath& temp_dir,
+                     InstallUpdateCallback install_update_callback) override;
 
  private:
   content::BrowserContext* browser_context_;

extensions/browser/updater/BUILD.gn

@@ -15,6 +15,8 @@ source_set("updater") {
     "extension_downloader_delegate.cc",
     "extension_downloader_delegate.h",
     "extension_downloader_test_delegate.h",
+    "extension_installer.cc",
+    "extension_installer.h",
     "manifest_fetch_data.cc",
     "manifest_fetch_data.h",
     "null_extension_cache.cc",
@@ -25,8 +27,6 @@ source_set("updater") {
     "safe_manifest_parser.h",
     "update_data_provider.cc",
     "update_data_provider.h",
-    "update_install_shim.cc",
-    "update_install_shim.h",
     "update_service.cc",
     "update_service.h",
     "update_service_factory.cc",

extensions/browser/updater/update_install_shim.cc → extensions/browser/updater/extension_installer.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "extensions/browser/updater/update_install_shim.h"
+#include "extensions/browser/updater/extension_installer.h"
 
 #include <utility>
 
@@ -13,6 +13,7 @@
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task_scheduler/post_task.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "components/update_client/update_client_errors.h"
 #include "content/public/browser/browser_thread.h"
 
@@ -23,54 +24,39 @@ using InstallError = update_client::InstallError;
 using Result = update_client::CrxInstaller::Result;
 }  // namespace
 
-UpdateInstallShim::UpdateInstallShim(std::string extension_id,
-                                     const base::FilePath& extension_root,
-                                     UpdateInstallShimCallback callback)
+ExtensionInstaller::ExtensionInstaller(
+    std::string extension_id,
+    const base::FilePath& extension_root,
+    ExtensionInstallerCallback extension_installer_callback)
     : extension_id_(extension_id),
       extension_root_(extension_root),
-      callback_(std::move(callback)) {}
+      extension_installer_callback_(std::move(extension_installer_callback)) {}
 
-void UpdateInstallShim::OnUpdateError(int error) {
+void ExtensionInstaller::OnUpdateError(int error) {
   VLOG(1) << "OnUpdateError (" << extension_id_ << ") " << error;
 }
 
-void UpdateInstallShim::Install(const base::FilePath& unpack_path,
-                                const std::string& /*public_key*/,
-                                Callback callback) {
-  base::ScopedTempDir temp_dir;
-  if (!temp_dir.CreateUniqueTempDir()) {
-    content::BrowserThread::PostTask(
-        content::BrowserThread::UI, FROM_HERE,
-        base::BindOnce(std::move(callback),
-                       Result(InstallError::GENERIC_ERROR)));
+void ExtensionInstaller::Install(const base::FilePath& unpack_path,
+                                 const std::string& public_key,
+                                 UpdateClientCallback update_client_callback) {
+  auto ui_thread = content::BrowserThread::GetTaskRunnerForThread(
+      content::BrowserThread::UI);
+  DCHECK(ui_thread);
+  DCHECK(!extension_installer_callback_.is_null());
+  if (base::PathExists(unpack_path)) {
+    ui_thread->PostTask(FROM_HERE,
+                        base::BindOnce(std::move(extension_installer_callback_),
+                                       extension_id_, public_key, unpack_path,
+                                       std::move(update_client_callback)));
     return;
   }
-
-  // The UpdateClient code will delete unpack_path if it still exists after
-  // this method is done, so we rename it on top of our temp dir.
-  if (!base::DeleteFile(temp_dir.GetPath(), true) ||
-      !base::Move(unpack_path, temp_dir.GetPath())) {
-    LOG(ERROR) << "Trying to install update for " << extension_id_
-               << "and failed to move " << unpack_path.value() << " to  "
-               << temp_dir.GetPath().value();
-    content::BrowserThread::PostTask(
-        content::BrowserThread::UI, FROM_HERE,
-        base::BindOnce(std::move(callback),
-                       Result(InstallError::GENERIC_ERROR)));
-    return;
-  }
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::BindOnce(&UpdateInstallShim::RunCallbackOnUIThread, this,
-                     temp_dir.Take()));
-
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::BindOnce(std::move(callback), Result(InstallError::NONE)));
+  ui_thread->PostTask(FROM_HERE,
+                      base::BindOnce(std::move(update_client_callback),
+                                     Result(InstallError::GENERIC_ERROR)));
 }
 
-bool UpdateInstallShim::GetInstalledFile(const std::string& file,
-                                         base::FilePath* installed_file) {
+bool ExtensionInstaller::GetInstalledFile(const std::string& file,
+                                          base::FilePath* installed_file) {
   base::FilePath relative_path = base::FilePath::FromUTF8Unsafe(file);
   if (relative_path.IsAbsolute() || relative_path.ReferencesParent())
     return false;
@@ -84,22 +70,11 @@ bool UpdateInstallShim::GetInstalledFile(const std::string& file,
   return true;
 }
 
-bool UpdateInstallShim::Uninstall() {
+bool ExtensionInstaller::Uninstall() {
   NOTREACHED();
   return false;
 }
 
-UpdateInstallShim::~UpdateInstallShim() {}
-
-void UpdateInstallShim::RunCallbackOnUIThread(const base::FilePath& temp_dir) {
-  if (callback_.is_null()) {
-    base::PostTaskWithTraits(FROM_HERE,
-                             {base::MayBlock(), base::TaskPriority::BACKGROUND},
-                             base::Bind(base::IgnoreResult(&base::DeleteFile),
-                                        temp_dir, true /*recursive */));
-    return;
-  }
-  std::move(callback_).Run(extension_id_, temp_dir);
-}
+ExtensionInstaller::~ExtensionInstaller() {}
 
 }  // namespace extensions

extensions/browser/updater/update_install_shim.h → extensions/browser/updater/extension_installer.h

-// Copyright 2015 The Chromium Authors. All rights reserved.
+// Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef EXTENSIONS_BROWSER_UPDATER_UPDATE_INSTALL_SHIM_H_
-#define EXTENSIONS_BROWSER_UPDATER_UPDATE_INSTALL_SHIM_H_
+#ifndef EXTENSIONS_BROWSER_UPDATER_EXTENSIONS_INSTALLER_H_
+#define EXTENSIONS_BROWSER_UPDATER_EXTENSIONS_INSTALLER_H_
 
 #include <memory>
 #include <string>
@@ -15,62 +15,54 @@
 
 namespace extensions {
 
-// A callback to implement the install of a new version of the extension.
-// Takes ownership of the directory at |temp_dir|.
-using UpdateInstallShimCallback =
-    base::OnceCallback<void(const std::string& extension_id,
-                            const base::FilePath& temp_dir)>;
-
 // This class is used as a shim between the components::update_client and
 // extensions code, to help the generic update_client code prepare and then
 // install an updated version of an extension. Because the update_client code
 // doesn't have the notion of extension ids, we use instances of this class to
 // map an install request back to the original update check for a given
 // extension.
-class UpdateInstallShim : public update_client::CrxInstaller {
+class ExtensionInstaller : public update_client::CrxInstaller {
  public:
+  using UpdateClientCallback = update_client::CrxInstaller::Callback;
+  // A callback to implement the install of a new version of the extension.
+  // Takes ownership of the directory at |unpacked_dir|.
+  using ExtensionInstallerCallback =
+      base::OnceCallback<void(const std::string& extension_id,
+                              const std::string& public_key,
+                              const base::FilePath& unpacked_dir,
+                              UpdateClientCallback update_client_callback)>;
+
   // This method takes the id and root directory for an extension we're doing
   // an update check for, as well as a callback to call if we get a new version
   // of it to install.
-  UpdateInstallShim(std::string extension_id,
-                    const base::FilePath& extension_root,
-                    UpdateInstallShimCallback callback);
+  ExtensionInstaller(std::string extension_id,
+                     const base::FilePath& extension_root,
+                     ExtensionInstallerCallback extension_installer_callback);
 
-  // Called when an update attempt failed.
+  // update_client::CrxInstaller::
   void OnUpdateError(int error) override;
 
-  // This is called when a new version of an extension is unpacked at
-  // |unpack_path| and is ready for install. |public_key| contains the
-  // CRX public_key in PEM format, without the header and the footer.
+  // This function is executed by the component update client, which runs on a
+  // blocking thread with background priority.
+  // |update_client_callback| is expected to be called on a UI thread.
   void Install(const base::FilePath& unpack_path,
                const std::string& public_key,
-               Callback callback) override;
-
-  // This is called by the generic differential update code in the
-  // update_client to provide the path to an existing file in the current
-  // version of the extension, so that it can be copied (or serve as the input
-  // to diff-patching) with output going to the directory with the new version
-  // being staged on disk for install.
+               UpdateClientCallback update_client_callback) override;
   bool GetInstalledFile(const std::string& file,
                         base::FilePath* installed_file) override;
-
-  // This method is not relevant to extension updating.
   bool Uninstall() override;
 
  private:
-  friend class base::RefCountedThreadSafe<UpdateInstallShim>;
-  ~UpdateInstallShim() override;
-
-  // Takes ownership of the directory at path |temp_dir|.
-  void RunCallbackOnUIThread(const base::FilePath& temp_dir);
+  friend class base::RefCountedThreadSafe<ExtensionInstaller>;
+  ~ExtensionInstaller() override;
 
   std::string extension_id_;
   base::FilePath extension_root_;
-  UpdateInstallShimCallback callback_;
+  ExtensionInstallerCallback extension_installer_callback_;
 
-  DISALLOW_COPY_AND_ASSIGN(UpdateInstallShim);
+  DISALLOW_COPY_AND_ASSIGN(ExtensionInstaller);
 };
 
 }  // namespace extensions
 
-#endif  // EXTENSIONS_BROWSER_UPDATER_UPDATE_INSTALL_SHIM_H_
+#endif  // EXTENSIONS_BROWSER_UPDATER_EXTENSIONS_INSTALLER_H_

extensions/browser/updater/extension_installer_unittest.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/browser/updater/extension_installer.h"
+
+#include <memory>
+#include <string>
+
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/run_loop.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "components/update_client/update_client.h"
+#include "components/update_client/update_client_errors.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "content/public/test/test_utils.h"
+#include "extensions/browser/extensions_test.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace extensions {
+
+namespace {
+
+class ExtensionInstallerTest : public ExtensionsTest {
+ public:
+  using UpdateClientCallback =
+      extensions::ExtensionInstaller::UpdateClientCallback;
+  using ExtensionInstallerCallback =
+      ExtensionInstaller::ExtensionInstallerCallback;
+  using Result = update_client::CrxInstaller::Result;
+  using InstallError = update_client::InstallError;
+
+  ExtensionInstallerTest();
+  ~ExtensionInstallerTest() override;
+
+  void InstallCompleteCallback(const Result& result);
+
+ protected:
+  void RunThreads();
+
+ protected:
+  const std::string kExtensionId = "ldnnhddmnhbkjipkidpdiheffobcpfmf";
+  const std::string kPublicKey =
+      "MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQC8c4fBSPZ6utYoZ8NiWF/"
+      "DSaimBhihjwgOsskyleFGaurhi3TDClTVSGPxNkgCzrz0wACML7M4aNjpd05qupdbR2d294j"
+      "kDuI7caxEGUucpP7GJRRHnm8Sx+"
+      "y0ury28n8jbN0PnInKKWcxpIXXmNQyC19HBuO3QIeUq9Dqc+7YFQIDAQAB";
+
+  base::RunLoop run_loop_;
+  Result result_;
+  bool executed_;
+
+  DISALLOW_COPY_AND_ASSIGN(ExtensionInstallerTest);
+};
+
+ExtensionInstallerTest::ExtensionInstallerTest()
+    : ExtensionsTest(std::make_unique<content::TestBrowserThreadBundle>()),
+      result_(-1),
+      executed_(false) {}
+
+ExtensionInstallerTest::~ExtensionInstallerTest() {}
+
+void ExtensionInstallerTest::InstallCompleteCallback(const Result& result) {
+  result_ = result;
+  executed_ = true;
+  run_loop_.Quit();
+}
+
+void ExtensionInstallerTest::RunThreads() {
+  run_loop_.Run();
+}
+
+TEST_F(ExtensionInstallerTest, GetInstalledFile) {
+  base::ScopedTempDir root_dir;
+  ASSERT_TRUE(root_dir.CreateUniqueTempDir());
+  ASSERT_TRUE(base::PathExists(root_dir.GetPath()));
+  scoped_refptr<ExtensionInstaller> installer =
+      base::MakeRefCounted<ExtensionInstaller>(kExtensionId, root_dir.GetPath(),
+                                               ExtensionInstallerCallback());
+
+  base::FilePath installed_file;
+
+#ifdef FILE_PATH_USES_DRIVE_LETTERS
+  const std::string absolute_path = "C:\\abc\\def";
+  const std::string relative_path = "abc\\..\\def\\ghi";
+#else
+  const std::string absolute_path = "/abc/def";
+  const std::string relative_path = "/abc/../def/ghi";
+#endif
+
+  installed_file.clear();
+  EXPECT_FALSE(installer->GetInstalledFile(absolute_path, &installed_file));
+  installed_file.clear();
+  EXPECT_FALSE(installer->GetInstalledFile(relative_path, &installed_file));
+  installed_file.clear();
+  EXPECT_FALSE(installer->GetInstalledFile("extension", &installed_file));
+
+  installed_file.clear();
+  base::FilePath temp_file;
+  ASSERT_TRUE(base::CreateTemporaryFileInDir(root_dir.GetPath(), &temp_file));
+  base::FilePath base_temp_file = temp_file.BaseName();
+  EXPECT_TRUE(installer->GetInstalledFile(
+      std::string(base_temp_file.value().begin(), base_temp_file.value().end()),
+      &installed_file));
+#ifndef FILE_PATH_USES_DRIVE_LETTERS
+  // On some Win*, this test is flaky because of the way Win* constructs path.
+  // For example,
+  // "C:\Users\chrome-bot\AppData" is the same as "C:\Users\CHROME~1\AppData"
+  EXPECT_EQ(temp_file, installed_file);
+#endif
+}
+
+TEST_F(ExtensionInstallerTest, Install_InvalidUnpackedDir) {
+  // The unpacked folder is not valid, the installer will return an error.
+  base::ScopedTempDir root_dir;
+  ASSERT_TRUE(root_dir.CreateUniqueTempDir());
+  ASSERT_TRUE(base::PathExists(root_dir.GetPath()));
+  scoped_refptr<ExtensionInstaller> installer =
+      base::MakeRefCounted<ExtensionInstaller>(
+          kExtensionId, root_dir.GetPath(),
+          base::BindOnce([](const std::string& extension_id,
+                            const std::string& public_key,
+                            const base::FilePath& unpacked_dir,
+                            UpdateClientCallback update_client_callback) {
+            // This function should never be executed.
+            EXPECT_TRUE(false);
+          }));
+
+  // Non-existing unpacked dir
+  base::ScopedTempDir unpacked_dir;
+  ASSERT_TRUE(unpacked_dir.CreateUniqueTempDir());
+  ASSERT_TRUE(base::PathExists(unpacked_dir.GetPath()));
+  ASSERT_TRUE(base::DeleteFile(unpacked_dir.GetPath(), true));
+  ASSERT_FALSE(base::PathExists(unpacked_dir.GetPath()));
+  installer->Install(
+      unpacked_dir.GetPath(), kPublicKey,
+      base::BindOnce(&ExtensionInstallerTest::InstallCompleteCallback,
+                     base::Unretained(this)));
+
+  RunThreads();
+
+  EXPECT_TRUE(executed_);
+  EXPECT_EQ(static_cast<int>(InstallError::GENERIC_ERROR), result_.error);
+}
+
+TEST_F(ExtensionInstallerTest, Install_BasicInstallOperation_Error) {
+  base::ScopedTempDir root_dir;
+  ASSERT_TRUE(root_dir.CreateUniqueTempDir());
+  ASSERT_TRUE(base::PathExists(root_dir.GetPath()));
+  scoped_refptr<ExtensionInstaller> installer =
+      base::MakeRefCounted<ExtensionInstaller>(
+          kExtensionId, root_dir.GetPath(),
+          base::BindOnce([](const std::string& extension_id,
+                            const std::string& public_key,
+                            const base::FilePath& unpacked_dir,
+                            UpdateClientCallback update_client_callback) {
+            std::move(update_client_callback)
+                .Run(Result(InstallError::GENERIC_ERROR));
+          }));
+
+  base::ScopedTempDir unpacked_dir;
+  ASSERT_TRUE(unpacked_dir.CreateUniqueTempDir());
+  ASSERT_TRUE(base::PathExists(unpacked_dir.GetPath()));
+
+  installer->Install(
+      unpacked_dir.GetPath(), kPublicKey,
+      base::BindOnce(&ExtensionInstallerTest::InstallCompleteCallback,
+                     base::Unretained(this)));
+
+  RunThreads();
+
+  EXPECT_TRUE(executed_);
+  EXPECT_EQ(static_cast<int>(InstallError::GENERIC_ERROR), result_.error);
+}
+
+TEST_F(ExtensionInstallerTest, Install_BasicInstallOperation_Success) {
+  base::ScopedTempDir root_dir;
+  ASSERT_TRUE(root_dir.CreateUniqueTempDir());
+  ASSERT_TRUE(base::PathExists(root_dir.GetPath()));
+  scoped_refptr<ExtensionInstaller> installer =
+      base::MakeRefCounted<ExtensionInstaller>(
+          kExtensionId, root_dir.GetPath(),
+          base::BindOnce([](const std::string& extension_id,
+                            const std::string& public_key,
+                            const base::FilePath& unpacked_dir,
+                            UpdateClientCallback update_client_callback) {
+            std::move(update_client_callback).Run(Result(InstallError::NONE));
+          }));
+
+  base::ScopedTempDir unpacked_dir;
+  ASSERT_TRUE(unpacked_dir.CreateUniqueTempDir());
+  ASSERT_TRUE(base::PathExists(unpacked_dir.GetPath()));
+
+  installer->Install(
+      unpacked_dir.GetPath(), kPublicKey,
+      base::BindOnce(&ExtensionInstallerTest::InstallCompleteCallback,
+                     base::Unretained(this)));
+
+  RunThreads();
+
+  EXPECT_TRUE(executed_);
+  EXPECT_EQ(static_cast<int>(InstallError::NONE), result_.error);
+}
+
+}  // namespace
+
+}  // namespace extensions

extensions/browser/updater/update_data_provider.cc

@@ -11,18 +11,20 @@
 #include "base/strings/string_util.h"
 #include "base/task_scheduler/post_task.h"
 #include "components/update_client/update_client.h"
+#include "content/public/browser/browser_thread.h"
 #include "crypto/sha2.h"
 #include "extensions/browser/content_verifier.h"
+#include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
-#include "extensions/browser/updater/update_install_shim.h"
+#include "extensions/browser/extensions_browser_client.h"
 #include "extensions/common/extension.h"
 
 namespace extensions {
 
 UpdateDataProvider::UpdateDataProvider(content::BrowserContext* context,
-                                       const InstallCallback& callback)
-    : context_(context), callback_(callback) {}
+                                       InstallCallback install_callback)
+    : context_(context), install_callback_(std::move(install_callback)) {}
 
 UpdateDataProvider::~UpdateDataProvider() {}
 
@@ -36,6 +38,7 @@ void UpdateDataProvider::GetData(
   if (!context_)
     return;
   const ExtensionRegistry* registry = ExtensionRegistry::Get(context_);
+  const ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(context_);
   for (const auto& id : ids) {
     const Extension* extension = registry->GetInstalledExtension(id);
     if (!extension)
@@ -50,22 +53,44 @@ void UpdateDataProvider::GetData(
     info->version = *extension->version();
     info->allows_background_download = false;
     info->requires_network_encryption = true;
-    info->installer = new UpdateInstallShim(
+    info->installer = base::MakeRefCounted<ExtensionInstaller>(
         id, extension->path(),
-        base::Bind(&UpdateDataProvider::RunInstallCallback, this));
+        base::BindOnce(&UpdateDataProvider::RunInstallCallback, this));
+    if (!ExtensionsBrowserClient::Get()->IsExtensionEnabled(id, context_)) {
+      int disabled_reasons = extension_prefs->GetDisableReasons(id);
+      if (disabled_reasons == extensions::disable_reason::DISABLE_NONE ||
+          disabled_reasons >= extensions::disable_reason::DISABLE_REASON_LAST) {
+        info->disabled_reasons.push_back(0);
+      }
+      for (int enum_value = 1;
+           enum_value < extensions::disable_reason::DISABLE_REASON_LAST;
+           enum_value <<= 1) {
+        if (disabled_reasons & enum_value)
+          info->disabled_reasons.push_back(enum_value);
+      }
+    }
   }
 }
 
-void UpdateDataProvider::RunInstallCallback(const std::string& extension_id,
-                                            const base::FilePath& temp_dir) {
+void UpdateDataProvider::RunInstallCallback(
+    const std::string& extension_id,
+    const std::string& public_key,
+    const base::FilePath& unpacked_dir,
+    UpdateClientCallback update_client_callback) {
   if (!context_) {
     base::PostTaskWithTraits(
-        FROM_HERE, {base::MayBlock(), base::TaskPriority::BACKGROUND},
-        base::Bind(base::IgnoreResult(&base::DeleteFile), temp_dir, false));
+        FROM_HERE, {base::TaskPriority::BACKGROUND, base::MayBlock()},
+        base::BindOnce(base::IgnoreResult(&base::DeleteFile), unpacked_dir,
+                       true));
     return;
-  } else {
-    callback_.Run(context_, extension_id, temp_dir);
   }
+
+  DCHECK(!install_callback_.is_null());
+  content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::UI)
+      ->PostTask(FROM_HERE,
+                 base::BindOnce(std::move(install_callback_), context_,
+                                extension_id, public_key, unpacked_dir,
+                                std::move(update_client_callback)));
 }
 
 }  // namespace extensions

extensions/browser/updater/update_data_provider.h

@@ -11,6 +11,7 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "extensions/browser/updater/extension_installer.h"
 
 namespace base {
 class FilePath;
@@ -30,17 +31,20 @@ namespace extensions {
 // extensions it is doing an update check for.
 class UpdateDataProvider : public base::RefCounted<UpdateDataProvider> {
  public:
-  typedef base::Callback<void(content::BrowserContext* context,
-                              const std::string& /* extension_id */,
-                              const base::FilePath& /* temp_dir */)>
-      InstallCallback;
+  using UpdateClientCallback = ExtensionInstaller::UpdateClientCallback;
+  using InstallCallback = base::OnceCallback<void(
+      content::BrowserContext* context,
+      const std::string& /* extension_id */,
+      const std::string& /* public_key */,
+      const base::FilePath& /* unpacked_dir */,
+      UpdateClientCallback /* update_client_callback */)>;
 
   // We need a browser context to use when retrieving data for a set of
-  // extension ids, as well as a callback for proceeding with installation
-  // steps once the UpdateClient has downloaded and unpacked an update for an
-  // extension.
+  // extension ids, as well as an install callback for proceeding with
+  // installation steps once the UpdateClient has downloaded and unpacked
+  // an update for an extension.
   UpdateDataProvider(content::BrowserContext* context,
-                     const InstallCallback& callback);
+                     InstallCallback install_callback);
 
   // Notify this object that the associated browser context is being shut down
   // the pointer to the context should be dropped and no more work should be
@@ -55,11 +59,14 @@ class UpdateDataProvider : public base::RefCounted<UpdateDataProvider> {
   friend class base::RefCounted<UpdateDataProvider>;
   ~UpdateDataProvider();
 
+  // This function should be called on the browser UI thread.
   void RunInstallCallback(const std::string& extension_id,
-                          const base::FilePath& temp_dir);
+                          const std::string& public_key,
+                          const base::FilePath& unpacked_dir,
+                          UpdateClientCallback update_client_callback);
 
   content::BrowserContext* context_;
-  InstallCallback callback_;
+  InstallCallback install_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(UpdateDataProvider);
 };

extensions/browser/updater/update_service.cc

@@ -9,6 +9,7 @@
 #include "components/update_client/update_client.h"
 #include "components/update_client/update_client_errors.h"
 #include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/browser/updater/update_data_provider.h"
@@ -16,15 +17,30 @@
 
 namespace {
 
+using UpdateClientCallback =
+    extensions::UpdateDataProvider::UpdateClientCallback;
+
 void UpdateCheckCompleteCallback(update_client::Error error) {}
 
 void SendUninstallPingCompleteCallback(update_client::Error error) {}
 
 void InstallUpdateCallback(content::BrowserContext* context,
                            const std::string& extension_id,
-                           const base::FilePath& temp_dir) {
-  extensions::ExtensionSystem::Get(context)
-      ->InstallUpdate(extension_id, temp_dir);
+                           const std::string& public_key,
+                           const base::FilePath& unpacked_dir,
+                           UpdateClientCallback update_client_callback) {
+  using InstallError = update_client::InstallError;
+  using Result = update_client::CrxInstaller::Result;
+  extensions::ExtensionSystem::Get(context)->InstallUpdate(
+      extension_id, public_key, unpacked_dir,
+      base::BindOnce(
+          [](UpdateClientCallback update_client_callback, bool success) {
+            DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+            std::move(update_client_callback)
+                .Run(Result(success ? InstallError::NONE
+                                    : InstallError::GENERIC_ERROR));
+          },
+          std::move(update_client_callback)));
 }
 
 }  // namespace
@@ -49,16 +65,17 @@ void UpdateService::SendUninstallPing(const std::string& id,
                                       const base::Version& version,
                                       int reason) {
   update_client_->SendUninstallPing(
-      id, version, reason, base::Bind(&SendUninstallPingCompleteCallback));
+      id, version, reason, base::BindOnce(&SendUninstallPingCompleteCallback));
 }
 
 void UpdateService::StartUpdateCheck(
     const std::vector<std::string>& extension_ids) {
   if (!update_client_)
     return;
-  update_client_->Update(extension_ids, base::Bind(&UpdateDataProvider::GetData,
-                                                   update_data_provider_),
-                         base::Bind(&UpdateCheckCompleteCallback));
+  update_client_->Update(
+      extension_ids,
+      base::BindOnce(&UpdateDataProvider::GetData, update_data_provider_),
+      base::BindOnce(&UpdateCheckCompleteCallback));
 }
 
 UpdateService::UpdateService(
@@ -66,8 +83,8 @@ UpdateService::UpdateService(
     scoped_refptr<update_client::UpdateClient> update_client)
     : context_(context), update_client_(update_client) {
   CHECK(update_client_);
-  update_data_provider_ =
-      new UpdateDataProvider(context_, base::Bind(&InstallUpdateCallback));
+  update_data_provider_ = base::MakeRefCounted<UpdateDataProvider>(
+      context_, base::BindOnce(&InstallUpdateCallback));
 }
 
 UpdateService::~UpdateService() {}

extensions/browser/updater/update_service_unittest.cc

@@ -112,6 +112,7 @@ UninstallPingSender::FilterResult ShouldPing(const Extension* extension,
 // versions of an extension.
 class FakeExtensionSystem : public MockExtensionSystem {
  public:
+  using InstallUpdateCallback = MockExtensionSystem::InstallUpdateCallback;
   explicit FakeExtensionSystem(content::BrowserContext* context)
       : MockExtensionSystem(context) {}
   ~FakeExtensionSystem() override {}
@@ -131,14 +132,18 @@ class FakeExtensionSystem : public MockExtensionSystem {
 
   // ExtensionSystem override
   void InstallUpdate(const std::string& extension_id,
-                     const base::FilePath& temp_dir) override {
+                     const std::string& public_key,
+                     const base::FilePath& temp_dir,
+                     InstallUpdateCallback install_update_callback) override {
     base::DeleteFile(temp_dir, true /*recursive*/);
     InstallUpdateRequest request;
     request.extension_id = extension_id;
     request.temp_dir = temp_dir;
     install_requests_.push_back(request);
-    if (!next_install_callback_.is_null())
+    if (!next_install_callback_.is_null()) {
       std::move(next_install_callback_).Run();
+    }
+    std::move(install_update_callback).Run(true);
   }
 
  private:
@@ -261,12 +266,16 @@ TEST_F(UpdateServiceTest, BasicUpdateOperations) {
   base::ScopedTempDir new_version_dir;
   ASSERT_TRUE(new_version_dir.CreateUniqueTempDir());
 
+  bool done = false;
   installer->Install(
       new_version_dir.GetPath(), std::string(),
-      base::Bind([](const update_client::CrxInstaller::Result& result) {
-        EXPECT_EQ(0, result.error);
-        EXPECT_EQ(0, result.extended_error);
-      }));
+      base::BindOnce(
+          [](bool* done, const update_client::CrxInstaller::Result& result) {
+            *done = true;
+            EXPECT_EQ(0, result.error);
+            EXPECT_EQ(0, result.extended_error);
+          },
+          &done));
 
   scoped_refptr<content::MessageLoopRunner> loop_runner =
       base::MakeRefCounted<content::MessageLoopRunner>();
@@ -277,8 +286,9 @@ TEST_F(UpdateServiceTest, BasicUpdateOperations) {
       extension_system()->install_requests();
   ASSERT_EQ(1u, requests->size());
   EXPECT_EQ(requests->at(0).extension_id, extension1->id());
-  EXPECT_NE(requests->at(0).temp_dir.value(),
+  EXPECT_EQ(requests->at(0).temp_dir.value(),
             new_version_dir.GetPath().value());
+  EXPECT_TRUE(done);
 }
 
 TEST_F(UpdateServiceTest, UninstallPings) {

extensions/shell/browser/shell_extension_system.cc

@@ -198,8 +198,11 @@ std::unique_ptr<ExtensionSet> ShellExtensionSystem::GetDependentExtensions(
   return std::make_unique<ExtensionSet>();
 }
 
-void ShellExtensionSystem::InstallUpdate(const std::string& extension_id,
-                                         const base::FilePath& temp_dir) {
+void ShellExtensionSystem::InstallUpdate(
+    const std::string& extension_id,
+    const std::string& public_key,
+    const base::FilePath& temp_dir,
+    InstallUpdateCallback install_update_callback) {
   NOTREACHED();
   base::DeleteFile(temp_dir, true /* recursive */);
 }

extensions/shell/browser/shell_extension_system.h

@@ -29,6 +29,7 @@ class ValueStoreFactory;
 // app_shell to skip initialization of services it doesn't need.
 class ShellExtensionSystem : public ExtensionSystem {
  public:
+  using InstallUpdateCallback = ExtensionSystem::InstallUpdateCallback;
   explicit ShellExtensionSystem(content::BrowserContext* browser_context);
   ~ShellExtensionSystem() override;
 
@@ -75,7 +76,9 @@ class ShellExtensionSystem : public ExtensionSystem {
   std::unique_ptr<ExtensionSet> GetDependentExtensions(
       const Extension* extension) override;
   void InstallUpdate(const std::string& extension_id,
-                     const base::FilePath& temp_dir) override;
+                     const std::string& public_key,
+                     const base::FilePath& temp_dir,
+                     InstallUpdateCallback install_update_callback) override;
 
  private:
   void OnExtensionRegisteredWithRequestContexts(