From 810e60f487982e8edc208b78b0b4b2b2a12a77a0 Mon Sep 17 00:00:00 2001 From: bauerb Date: Wed, 4 Feb 2015 17:09:10 -0800 Subject: 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} --- components/update_client/component_patcher.cc | 3 ++- components/update_client/component_patcher.h | 4 ++-- .../update_client/component_patcher_operation.cc | 21 +++++++++-------- .../update_client/component_patcher_operation.h | 27 +++++++++++----------- components/update_client/component_unpacker.cc | 6 ++--- components/update_client/component_unpacker.h | 15 ++++++------ .../test/component_patcher_unittest.cc | 2 +- .../test/component_patcher_unittest.h | 2 +- components/update_client/test/test_installer.cc | 7 +----- components/update_client/test/test_installer.h | 18 ++++++++++----- components/update_client/update_client.h | 9 ++++++-- 11 files changed, 62 insertions(+), 52 deletions(-) (limited to 'components/update_client') 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 installer, scoped_refptr out_of_process_patcher, scoped_refptr 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 { // out-of-process. ComponentPatcher(const base::FilePath& input_dir, const base::FilePath& unpack_dir, - ComponentInstaller* installer, + scoped_refptr installer, scoped_refptr out_of_process_patcher, scoped_refptr task_runner); @@ -86,7 +86,7 @@ class ComponentPatcher : public base::RefCountedThreadSafe { const base::FilePath input_dir_; const base::FilePath unpack_dir_; - ComponentInstaller* const installer_; + scoped_refptr installer_; scoped_refptr out_of_process_patcher_; ComponentUnpacker::Callback callback_; scoped_ptr 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 out_of_process_patcher) { + const scoped_refptr& 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 task_runner) { +void DeltaUpdateOp::Run( + const base::DictionaryValue* command_args, + const base::FilePath& input_dir, + const base::FilePath& unpack_dir, + const scoped_refptr& installer, + const ComponentUnpacker::Callback& callback, + const scoped_refptr& 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& 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& 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& 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 { void Run(const base::DictionaryValue* command_args, const base::FilePath& input_dir, const base::FilePath& unpack_dir, - ComponentInstaller* installer, + const scoped_refptr& installer, const ComponentUnpacker::Callback& callback, - scoped_refptr task_runner); + const scoped_refptr& task_runner); protected: virtual ~DeltaUpdateOp(); @@ -60,7 +60,7 @@ class DeltaUpdateOp : public base::RefCountedThreadSafe { virtual ComponentUnpacker::Error DoParseArguments( const base::DictionaryValue* command_args, const base::FilePath& input_dir, - ComponentInstaller* installer) = 0; + const scoped_refptr& 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& 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& installer) override; void DoRun(const ComponentUnpacker::Callback& callback) override; @@ -129,12 +129,13 @@ class DeltaUpdateOpCreate : public DeltaUpdateOp { class OutOfProcessPatcher : public base::RefCountedThreadSafe { public: - virtual void Patch(const std::string& operation, - scoped_refptr task_runner, - const base::FilePath& input_abs_path, - const base::FilePath& patch_abs_path, - const base::FilePath& output_abs_path, - base::Callback callback) = 0; + virtual void Patch( + const std::string& operation, + scoped_refptr task_runner, + const base::FilePath& input_abs_path, + const base::FilePath& patch_abs_path, + const base::FilePath& output_abs_path, + base::Callback callback) = 0; protected: friend class base::RefCountedThreadSafe; @@ -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& 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 out_of_process_patcher); + const scoped_refptr& 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& pk_hash, const base::FilePath& path, const std::string& fingerprint, - ComponentInstaller* installer, - scoped_refptr oop_patcher, - scoped_refptr task_runner) + const scoped_refptr& installer, + const scoped_refptr& oop_patcher, + const scoped_refptr& 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 { // 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& pk_hash, - const base::FilePath& path, - const std::string& fingerprint, - ComponentInstaller* installer, - scoped_refptr oop_patcher, - scoped_refptr task_runner); + ComponentUnpacker( + const std::vector& pk_hash, + const base::FilePath& path, + const std::string& fingerprint, + const scoped_refptr& installer, + const scoped_refptr& oop_patcher, + const scoped_refptr& 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 { bool is_delta_; std::string fingerprint_; scoped_refptr patcher_; - ComponentInstaller* installer_; + scoped_refptr installer_; Callback callback_; scoped_refptr 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 installer_; + scoped_refptr installer_; scoped_refptr 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 #include +#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 { 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; + virtual ~ComponentInstaller() {} }; @@ -57,7 +62,7 @@ class ComponentInstaller { // the issue 340448 is resolved. struct CrxComponent { std::vector pk_hash; - ComponentInstaller* installer; + scoped_refptr installer; Version version; std::string fingerprint; std::string name; -- cgit v1.1