Make ComponentInstaller refcounted.

Before this CL, component installers were leaked in almost all cases. If we allow uninstalling components (see https://codereview.chromium.org/879993005/), we need to fix those leaks.

BUG=436459

Review URL: https://codereview.chromium.org/897873002

Cr-Commit-Position: refs/heads/master@{#314701}

11 files changed, 62 insertions, 52 deletions

diff --git a/components/update_client/component_patcher.cc b/components/update_client/component_patcher.cc
index 651756e..3077bc4 100644
--- a/components/update_client/component_patcher.cc
+++ b/components/update_client/component_patcher.cc
@@ -16,6 +16,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/values.h"
 #include "components/update_client/component_patcher_operation.h"
+#include "components/update_client/update_client.h"
 
 namespace update_client {
 
@@ -42,7 +43,7 @@ base::ListValue* ReadCommands(const base::FilePath& unpack_path) {
 ComponentPatcher::ComponentPatcher(
     const base::FilePath& input_dir,
     const base::FilePath& unpack_dir,
-    ComponentInstaller* installer,
+    scoped_refptr<ComponentInstaller> installer,
     scoped_refptr<OutOfProcessPatcher> out_of_process_patcher,
     scoped_refptr<base::SequencedTaskRunner> task_runner)
     : input_dir_(input_dir),
diff --git a/components/update_client/component_patcher.h b/components/update_client/component_patcher.h
index b7dd216..394edfe 100644
--- a/components/update_client/component_patcher.h
+++ b/components/update_client/component_patcher.h
@@ -61,7 +61,7 @@ class ComponentPatcher : public base::RefCountedThreadSafe<ComponentPatcher> {
   // out-of-process.
   ComponentPatcher(const base::FilePath& input_dir,
                    const base::FilePath& unpack_dir,
-                   ComponentInstaller* installer,
+                   scoped_refptr<ComponentInstaller> installer,
                    scoped_refptr<OutOfProcessPatcher> out_of_process_patcher,
                    scoped_refptr<base::SequencedTaskRunner> task_runner);
 
@@ -86,7 +86,7 @@ class ComponentPatcher : public base::RefCountedThreadSafe<ComponentPatcher> {
 
   const base::FilePath input_dir_;
   const base::FilePath unpack_dir_;
-  ComponentInstaller* const installer_;
+  scoped_refptr<ComponentInstaller> installer_;
   scoped_refptr<OutOfProcessPatcher> out_of_process_patcher_;
   ComponentUnpacker::Callback callback_;
   scoped_ptr<base::ListValue> commands_;
diff --git a/components/update_client/component_patcher_operation.cc b/components/update_client/component_patcher_operation.cc
index adb5b8f..c5e7437 100644
--- a/components/update_client/component_patcher_operation.cc
+++ b/components/update_client/component_patcher_operation.cc
@@ -43,7 +43,7 @@ const char kPatch[] = "patch";
 
 DeltaUpdateOp* CreateDeltaUpdateOp(
     const std::string& operation,
-    scoped_refptr<OutOfProcessPatcher> out_of_process_patcher) {
+    const scoped_refptr<OutOfProcessPatcher>& out_of_process_patcher) {
   if (operation == "copy") {
     return new DeltaUpdateOpCopy();
   } else if (operation == "create") {
@@ -60,12 +60,13 @@ DeltaUpdateOp::DeltaUpdateOp() {
 DeltaUpdateOp::~DeltaUpdateOp() {
 }
 
-void DeltaUpdateOp::Run(const base::DictionaryValue* command_args,
-                        const base::FilePath& input_dir,
-                        const base::FilePath& unpack_dir,
-                        ComponentInstaller* installer,
-                        const ComponentUnpacker::Callback& callback,
-                        scoped_refptr<base::SequencedTaskRunner> task_runner) {
+void DeltaUpdateOp::Run(
+    const base::DictionaryValue* command_args,
+    const base::FilePath& input_dir,
+    const base::FilePath& unpack_dir,
+    const scoped_refptr<ComponentInstaller>& installer,
+    const ComponentUnpacker::Callback& callback,
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner) {
   callback_ = callback;
   task_runner_ = task_runner;
   std::string output_rel_path;
@@ -140,7 +141,7 @@ DeltaUpdateOpCopy::~DeltaUpdateOpCopy() {
 ComponentUnpacker::Error DeltaUpdateOpCopy::DoParseArguments(
     const base::DictionaryValue* command_args,
     const base::FilePath& input_dir,
-    ComponentInstaller* installer) {
+    const scoped_refptr<ComponentInstaller>& installer) {
   std::string input_rel_path;
   if (!command_args->GetString(kInput, &input_rel_path))
     return ComponentUnpacker::kDeltaBadCommands;
@@ -167,7 +168,7 @@ DeltaUpdateOpCreate::~DeltaUpdateOpCreate() {
 ComponentUnpacker::Error DeltaUpdateOpCreate::DoParseArguments(
     const base::DictionaryValue* command_args,
     const base::FilePath& input_dir,
-    ComponentInstaller* installer) {
+    const scoped_refptr<ComponentInstaller>& installer) {
   std::string patch_rel_path;
   if (!command_args->GetString(kPatch, &patch_rel_path))
     return ComponentUnpacker::kDeltaBadCommands;
@@ -198,7 +199,7 @@ DeltaUpdateOpPatch::~DeltaUpdateOpPatch() {
 ComponentUnpacker::Error DeltaUpdateOpPatch::DoParseArguments(
     const base::DictionaryValue* command_args,
     const base::FilePath& input_dir,
-    ComponentInstaller* installer) {
+    const scoped_refptr<ComponentInstaller>& installer) {
   std::string patch_rel_path;
   std::string input_rel_path;
   if (!command_args->GetString(kPatch, &patch_rel_path) ||
diff --git a/components/update_client/component_patcher_operation.h b/components/update_client/component_patcher_operation.h
index 8f31c04..0771757 100644
--- a/components/update_client/component_patcher_operation.h
+++ b/components/update_client/component_patcher_operation.h
@@ -37,9 +37,9 @@ class DeltaUpdateOp : public base::RefCountedThreadSafe<DeltaUpdateOp> {
   void Run(const base::DictionaryValue* command_args,
            const base::FilePath& input_dir,
            const base::FilePath& unpack_dir,
-           ComponentInstaller* installer,
+           const scoped_refptr<ComponentInstaller>& installer,
            const ComponentUnpacker::Callback& callback,
-           scoped_refptr<base::SequencedTaskRunner> task_runner);
+           const scoped_refptr<base::SequencedTaskRunner>& task_runner);
 
  protected:
   virtual ~DeltaUpdateOp();
@@ -60,7 +60,7 @@ class DeltaUpdateOp : public base::RefCountedThreadSafe<DeltaUpdateOp> {
   virtual ComponentUnpacker::Error DoParseArguments(
       const base::DictionaryValue* command_args,
       const base::FilePath& input_dir,
-      ComponentInstaller* installer) = 0;
+      const scoped_refptr<ComponentInstaller>& installer) = 0;
 
   // Subclasses must override DoRun to actually perform the patching operation.
   // They must call the provided callback when they have completed their
@@ -92,7 +92,7 @@ class DeltaUpdateOpCopy : public DeltaUpdateOp {
   ComponentUnpacker::Error DoParseArguments(
       const base::DictionaryValue* command_args,
       const base::FilePath& input_dir,
-      ComponentInstaller* installer) override;
+      const scoped_refptr<ComponentInstaller>& installer) override;
 
   void DoRun(const ComponentUnpacker::Callback& callback) override;
 
@@ -116,7 +116,7 @@ class DeltaUpdateOpCreate : public DeltaUpdateOp {
   ComponentUnpacker::Error DoParseArguments(
       const base::DictionaryValue* command_args,
       const base::FilePath& input_dir,
-      ComponentInstaller* installer) override;
+      const scoped_refptr<ComponentInstaller>& installer) override;
 
   void DoRun(const ComponentUnpacker::Callback& callback) override;
 
@@ -129,12 +129,13 @@ class DeltaUpdateOpCreate : public DeltaUpdateOp {
 class OutOfProcessPatcher
     : public base::RefCountedThreadSafe<OutOfProcessPatcher> {
  public:
-  virtual void Patch(const std::string& operation,
-                     scoped_refptr<base::SequencedTaskRunner> task_runner,
-                     const base::FilePath& input_abs_path,
-                     const base::FilePath& patch_abs_path,
-                     const base::FilePath& output_abs_path,
-                     base::Callback<void(int result)> callback) = 0;
+  virtual void Patch(
+      const std::string& operation,
+      scoped_refptr<base::SequencedTaskRunner> task_runner,
+      const base::FilePath& input_abs_path,
+      const base::FilePath& patch_abs_path,
+      const base::FilePath& output_abs_path,
+      base::Callback<void(int result)> callback) = 0;
 
  protected:
   friend class base::RefCountedThreadSafe<OutOfProcessPatcher>;
@@ -159,7 +160,7 @@ class DeltaUpdateOpPatch : public DeltaUpdateOp {
   ComponentUnpacker::Error DoParseArguments(
       const base::DictionaryValue* command_args,
       const base::FilePath& input_dir,
-      ComponentInstaller* installer) override;
+      const scoped_refptr<ComponentInstaller>& installer) override;
 
   void DoRun(const ComponentUnpacker::Callback& callback) override;
 
@@ -177,7 +178,7 @@ class DeltaUpdateOpPatch : public DeltaUpdateOp {
 
 DeltaUpdateOp* CreateDeltaUpdateOp(
     const std::string& operation,
-    scoped_refptr<OutOfProcessPatcher> out_of_process_patcher);
+    const scoped_refptr<OutOfProcessPatcher>& out_of_process_patcher);
 
 }  // namespace update_client
 
diff --git a/components/update_client/component_unpacker.cc b/components/update_client/component_unpacker.cc
index 27d75b9..eb1b2d8 100644
--- a/components/update_client/component_unpacker.cc
+++ b/components/update_client/component_unpacker.cc
@@ -102,9 +102,9 @@ ComponentUnpacker::ComponentUnpacker(
     const std::vector<uint8_t>& pk_hash,
     const base::FilePath& path,
     const std::string& fingerprint,
-    ComponentInstaller* installer,
-    scoped_refptr<OutOfProcessPatcher> oop_patcher,
-    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    const scoped_refptr<ComponentInstaller>& installer,
+    const scoped_refptr<OutOfProcessPatcher>& oop_patcher,
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner)
     : pk_hash_(pk_hash),
       path_(path),
       is_delta_(false),
diff --git a/components/update_client/component_unpacker.h b/components/update_client/component_unpacker.h
index 7c2b6ef..033fd33 100644
--- a/components/update_client/component_unpacker.h
+++ b/components/update_client/component_unpacker.h
@@ -95,12 +95,13 @@ class ComponentUnpacker : public base::RefCountedThreadSafe<ComponentUnpacker> {
   // Constructs an unpacker for a specific component unpacking operation.
   // |pk_hash| is the expected/ public key SHA256 hash. |path| is the current
   // location of the CRX.
-  ComponentUnpacker(const std::vector<uint8_t>& pk_hash,
-                    const base::FilePath& path,
-                    const std::string& fingerprint,
-                    ComponentInstaller* installer,
-                    scoped_refptr<OutOfProcessPatcher> oop_patcher,
-                    scoped_refptr<base::SequencedTaskRunner> task_runner);
+  ComponentUnpacker(
+      const std::vector<uint8_t>& pk_hash,
+      const base::FilePath& path,
+      const std::string& fingerprint,
+      const scoped_refptr<ComponentInstaller>& installer,
+      const scoped_refptr<OutOfProcessPatcher>& oop_patcher,
+      const scoped_refptr<base::SequencedTaskRunner>& task_runner);
 
   // Begins the actual unpacking of the files. May invoke a patcher if the
   // package is a differential update. Calls |callback| with the result.
@@ -147,7 +148,7 @@ class ComponentUnpacker : public base::RefCountedThreadSafe<ComponentUnpacker> {
   bool is_delta_;
   std::string fingerprint_;
   scoped_refptr<ComponentPatcher> patcher_;
-  ComponentInstaller* installer_;
+  scoped_refptr<ComponentInstaller> installer_;
   Callback callback_;
   scoped_refptr<OutOfProcessPatcher> oop_patcher_;
   Error error_;
diff --git a/components/update_client/test/component_patcher_unittest.cc b/components/update_client/test/component_patcher_unittest.cc
index ce6e7a1..4f5e143 100644
--- a/components/update_client/test/component_patcher_unittest.cc
+++ b/components/update_client/test/component_patcher_unittest.cc
@@ -69,7 +69,7 @@ ComponentPatcherOperationTest::ComponentPatcherOperationTest() {
   EXPECT_TRUE(unpack_dir_.CreateUniqueTempDir());
   EXPECT_TRUE(input_dir_.CreateUniqueTempDir());
   EXPECT_TRUE(installed_dir_.CreateUniqueTempDir());
-  installer_.reset(new ReadOnlyTestInstaller(installed_dir_.path()));
+  installer_ = new ReadOnlyTestInstaller(installed_dir_.path());
   task_runner_ = base::MessageLoop::current()->task_runner();
 }
 
diff --git a/components/update_client/test/component_patcher_unittest.h b/components/update_client/test/component_patcher_unittest.h
index f7a4aea..2fcd5f5 100644
--- a/components/update_client/test/component_patcher_unittest.h
+++ b/components/update_client/test/component_patcher_unittest.h
@@ -30,7 +30,7 @@ class ComponentPatcherOperationTest : public testing::Test {
   base::ScopedTempDir input_dir_;
   base::ScopedTempDir installed_dir_;
   base::ScopedTempDir unpack_dir_;
-  scoped_ptr<ReadOnlyTestInstaller> installer_;
+  scoped_refptr<ReadOnlyTestInstaller> installer_;
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
  private:
diff --git a/components/update_client/test/test_installer.cc b/components/update_client/test/test_installer.cc
index b6a3f84..de32b1f 100644
--- a/components/update_client/test/test_installer.cc
+++ b/components/update_client/test/test_installer.cc
@@ -30,12 +30,7 @@ bool TestInstaller::GetInstalledFile(const std::string& file,
   return false;
 }
 
-int TestInstaller::error() const {
-  return error_;
-}
-
-int TestInstaller::install_count() const {
-  return install_count_;
+TestInstaller::~TestInstaller() {
 }
 
 ReadOnlyTestInstaller::ReadOnlyTestInstaller(const base::FilePath& install_dir)
diff --git a/components/update_client/test/test_installer.h b/components/update_client/test/test_installer.h
index 933fe21..2d9ae9f 100644
--- a/components/update_client/test/test_installer.h
+++ b/components/update_client/test/test_installer.h
@@ -31,11 +31,17 @@ class TestInstaller : public ComponentInstaller {
   bool GetInstalledFile(const std::string& file,
                         base::FilePath* installed_file) override;
 
-  int error() const;
+  int error() const {
+    return error_;
+  }
 
-  int install_count() const;
+  int install_count() const {
+    return install_count_;
+  }
 
  protected:
+  ~TestInstaller() override;
+
   int error_;
   int install_count_;
 };
@@ -46,12 +52,12 @@ class ReadOnlyTestInstaller : public TestInstaller {
  public:
   explicit ReadOnlyTestInstaller(const base::FilePath& installed_path);
 
-  ~ReadOnlyTestInstaller() override;
-
   bool GetInstalledFile(const std::string& file,
                         base::FilePath* installed_file) override;
 
  private:
+  ~ReadOnlyTestInstaller() override;
+
   base::FilePath install_directory_;
 };
 
@@ -61,8 +67,6 @@ class VersionedTestInstaller : public TestInstaller {
  public:
   VersionedTestInstaller();
 
-  ~VersionedTestInstaller() override;
-
   bool Install(const base::DictionaryValue& manifest,
                const base::FilePath& unpack_path) override;
 
@@ -70,6 +74,8 @@ class VersionedTestInstaller : public TestInstaller {
                         base::FilePath* installed_file) override;
 
  private:
+  ~VersionedTestInstaller() override;
+
   base::FilePath install_directory_;
   Version current_version_;
 };
diff --git a/components/update_client/update_client.h b/components/update_client/update_client.h
index 07b5617..28b69c0 100644
--- a/components/update_client/update_client.h
+++ b/components/update_client/update_client.h
@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "base/memory/ref_counted.h"
 #include "base/version.h"
 
 namespace base {
@@ -21,7 +22,8 @@ namespace update_client {
 // Component specific installers must derive from this class and implement
 // OnUpdateError() and Install(). A valid instance of this class must be
 // given to ComponentUpdateService::RegisterComponent().
-class ComponentInstaller {
+class ComponentInstaller
+    : public base::RefCountedThreadSafe<ComponentInstaller> {
  public:
   // Called by the component updater on the main thread when there was a
   // problem unpacking or verifying the component. |error| is a non-zero
@@ -43,6 +45,9 @@ class ComponentInstaller {
   virtual bool GetInstalledFile(const std::string& file,
                                 base::FilePath* installed_file) = 0;
 
+ protected:
+  friend class base::RefCountedThreadSafe<ComponentInstaller>;
+
   virtual ~ComponentInstaller() {}
 };
 
@@ -57,7 +62,7 @@ class ComponentInstaller {
 // the issue 340448 is resolved.
 struct CrxComponent {
   std::vector<uint8_t> pk_hash;
-  ComponentInstaller* installer;
+  scoped_refptr<ComponentInstaller> installer;
   Version version;
   std::string fingerprint;
   std::string name;