diff options
author | bauerb <bauerb@chromium.org> | 2015-02-04 17:09:10 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-02-05 01:09:55 +0000 |
commit | 810e60f487982e8edc208b78b0b4b2b2a12a77a0 (patch) | |
tree | f76a8cb2498a8e32804fd0a9eb04703ed7a84127 /components/update_client | |
parent | abfbde8049ea4726e3c750a15aad66c5ab9f5093 (diff) | |
download | chromium_src-810e60f487982e8edc208b78b0b4b2b2a12a77a0.zip chromium_src-810e60f487982e8edc208b78b0b4b2b2a12a77a0.tar.gz chromium_src-810e60f487982e8edc208b78b0b4b2b2a12a77a0.tar.bz2 |
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}
Diffstat (limited to 'components/update_client')
-rw-r--r-- | components/update_client/component_patcher.cc | 3 | ||||
-rw-r--r-- | components/update_client/component_patcher.h | 4 | ||||
-rw-r--r-- | components/update_client/component_patcher_operation.cc | 21 | ||||
-rw-r--r-- | components/update_client/component_patcher_operation.h | 27 | ||||
-rw-r--r-- | components/update_client/component_unpacker.cc | 6 | ||||
-rw-r--r-- | components/update_client/component_unpacker.h | 15 | ||||
-rw-r--r-- | components/update_client/test/component_patcher_unittest.cc | 2 | ||||
-rw-r--r-- | components/update_client/test/component_patcher_unittest.h | 2 | ||||
-rw-r--r-- | components/update_client/test/test_installer.cc | 7 | ||||
-rw-r--r-- | components/update_client/test/test_installer.h | 18 | ||||
-rw-r--r-- | components/update_client/update_client.h | 9 |
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; |