Make UpdateEngine and UpdateContext refcounted types.

Bug: 819706
Change-Id: I5d9a244871ec06b33babb56eb8bd2453133c3c8b
Reviewed-on: https://chromium-review.googlesource.com/953243
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541660}

components/update_client/ping_manager_unittest.cc

@@ -34,7 +34,7 @@ class PingManagerTest : public testing::Test {
   ~PingManagerTest() override {}
 
   PingManager::Callback MakePingCallback();
-  std::unique_ptr<UpdateContext> MakeFakeUpdateContext() const;
+  scoped_refptr<UpdateContext> MakeFakeUpdateContext() const;
 
   // Overrides from testing::Test.
   void SetUp() override;
@@ -96,8 +96,8 @@ void PingManagerTest::PingSentCallback(int error, const std::string& response) {
   Quit();
 }
 
-std::unique_ptr<UpdateContext> PingManagerTest::MakeFakeUpdateContext() const {
-  return std::make_unique<UpdateContext>(
+scoped_refptr<UpdateContext> PingManagerTest::MakeFakeUpdateContext() const {
+  return base::MakeRefCounted<UpdateContext>(
       config_, false, std::vector<std::string>(),
       UpdateClient::CrxDataCallback(), UpdateEngine::NotifyObserversCallback(),
       UpdateEngine::Callback(), nullptr);

components/update_client/task.h

@@ -13,8 +13,6 @@
 
 namespace update_client {
 
-class Task;
-
 // Defines an abstraction for a unit of work done by the update client.
 // Each invocation of the update client API results in a task being created and
 // run. In most cases, a task corresponds to a set of CRXs, which are updated

components/update_client/task_send_uninstall_ping.cc

@@ -15,11 +15,12 @@
 
 namespace update_client {
 
-TaskSendUninstallPing::TaskSendUninstallPing(UpdateEngine* update_engine,
-                                             const std::string& id,
-                                             const base::Version& version,
-                                             int reason,
-                                             Callback callback)
+TaskSendUninstallPing::TaskSendUninstallPing(
+    scoped_refptr<UpdateEngine> update_engine,
+    const std::string& id,
+    const base::Version& version,
+    int reason,
+    Callback callback)
     : update_engine_(update_engine),
       id_(id),
       version_(version),

components/update_client/task_send_uninstall_ping.h

@@ -34,7 +34,7 @@ class TaskSendUninstallPing : public Task {
   // |id| represents the CRX to send the ping for.
   // |callback| is called to return the execution flow back to creator of
   //    this task when the task is done.
-  TaskSendUninstallPing(UpdateEngine* update_engine,
+  TaskSendUninstallPing(scoped_refptr<UpdateEngine> update_engine,
                         const std::string& id,
                         const base::Version& version,
                         int reason,
@@ -54,8 +54,7 @@ class TaskSendUninstallPing : public Task {
   void TaskComplete(Error error);
 
   base::ThreadChecker thread_checker_;
-
-  UpdateEngine* update_engine_;  // Not owned by this class.
+  scoped_refptr<UpdateEngine> update_engine_;
   const std::string id_;
   const base::Version version_;
   int reason_;

components/update_client/task_update.cc

@@ -14,7 +14,7 @@
 
 namespace update_client {
 
-TaskUpdate::TaskUpdate(UpdateEngine* update_engine,
+TaskUpdate::TaskUpdate(scoped_refptr<UpdateEngine> update_engine,
                        bool is_foreground,
                        const std::vector<std::string>& ids,
                        UpdateClient::CrxDataCallback crx_data_callback,

components/update_client/task_update.h

@@ -32,7 +32,7 @@ class TaskUpdate : public Task {
   // |crx_data_callback| is called to get update data for the these CRXs.
   // |callback| is called to return the execution flow back to creator of
   //    this task when the task is done.
-  TaskUpdate(UpdateEngine* update_engine,
+  TaskUpdate(scoped_refptr<UpdateEngine> update_engine,
              bool is_foreground,
              const std::vector<std::string>& ids,
              UpdateClient::CrxDataCallback crx_data_callback,
@@ -52,9 +52,7 @@ class TaskUpdate : public Task {
   void TaskComplete(Error error);
 
   base::ThreadChecker thread_checker_;
-
-  UpdateEngine* update_engine_;  // Not owned by this class.
-
+  scoped_refptr<UpdateEngine> update_engine_;
   const bool is_foreground_;
   const std::vector<std::string> ids_;
   UpdateClient::CrxDataCallback crx_data_callback_;

components/update_client/update_checker_unittest.cc

@@ -136,10 +136,10 @@ class UpdateCheckerTest : public testing::Test,
   int error_ = 0;
   int retry_after_sec_ = 0;
 
-  std::unique_ptr<UpdateContext> update_context_;
+  scoped_refptr<UpdateContext> update_context_;
 
  private:
-  std::unique_ptr<UpdateContext> MakeFakeUpdateContext() const;
+  scoped_refptr<UpdateContext> MakeFakeUpdateContext() const;
 
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   base::OnceClosure quit_closure_;
@@ -205,9 +205,8 @@ void UpdateCheckerTest::UpdateCheckComplete(int error, int retry_after_sec) {
   Quit();
 }
 
-std::unique_ptr<UpdateContext> UpdateCheckerTest::MakeFakeUpdateContext()
-    const {
-  return std::make_unique<UpdateContext>(
+scoped_refptr<UpdateContext> UpdateCheckerTest::MakeFakeUpdateContext() const {
+  return base::MakeRefCounted<UpdateContext>(
       config_, false, std::vector<std::string>(),
       UpdateClient::CrxDataCallback(), UpdateEngine::NotifyObserversCallback(),
       UpdateEngine::Callback(), nullptr);

components/update_client/update_client.cc

@@ -67,7 +67,7 @@ UpdateClientImpl::UpdateClientImpl(
     : is_stopped_(false),
       config_(config),
       ping_manager_(ping_manager),
-      update_engine_(std::make_unique<UpdateEngine>(
+      update_engine_(base::MakeRefCounted<UpdateEngine>(
           config,
           update_checker_factory,
           crx_downloader_factory,

components/update_client/update_client_internal.h

@@ -80,11 +80,8 @@ class UpdateClientImpl : public UpdateClient {
   // tasks are running. In addition, concurrent install tasks for the same id
   // are not allowed.
   std::set<scoped_refptr<Task>> tasks_;
-
-  // TODO(sorin): try to make the ping manager an observer of the service.
   scoped_refptr<PingManager> ping_manager_;
-  std::unique_ptr<UpdateEngine> update_engine_;
-
+  scoped_refptr<UpdateEngine> update_engine_;
   base::ObserverList<Observer> observer_list_;
 
   DISALLOW_COPY_AND_ASSIGN(UpdateClientImpl);

components/update_client/update_engine.cc

@@ -87,10 +87,11 @@ void UpdateEngine::Update(bool is_foreground,
     return;
   }
 
-  const auto result = update_contexts_.insert(std::make_unique<UpdateContext>(
-      config_, is_foreground, ids, std::move(crx_data_callback),
-      notify_observers_callback_, std::move(callback),
-      crx_downloader_factory_));
+  const auto result =
+      update_contexts_.insert(base::MakeRefCounted<UpdateContext>(
+          config_, is_foreground, ids, std::move(crx_data_callback),
+          notify_observers_callback_, std::move(callback),
+          crx_downloader_factory_));
 
   DCHECK(result.second);
 
@@ -354,10 +355,12 @@ void UpdateEngine::SendUninstallPing(const std::string& id,
                                      Callback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  const auto result = update_contexts_.insert(std::make_unique<UpdateContext>(
-      config_, false, std::vector<std::string>{id},
-      UpdateClient::CrxDataCallback(), UpdateEngine::NotifyObserversCallback(),
-      std::move(callback), nullptr));
+  const auto result =
+      update_contexts_.insert(base::MakeRefCounted<UpdateContext>(
+          config_, false, std::vector<std::string>{id},
+          UpdateClient::CrxDataCallback(),
+          UpdateEngine::NotifyObserversCallback(), std::move(callback),
+          nullptr));
 
   DCHECK(result.second);
 

components/update_client/update_engine.h

@@ -38,7 +38,7 @@ struct UpdateContext;
 // Handles updates for a group of components. Updates for different groups
 // are run concurrently but within the same group of components, updates are
 // applied one at a time.
-class UpdateEngine {
+class UpdateEngine : public base::RefCounted<UpdateEngine> {
  public:
   using Callback = base::OnceCallback<void(Error error)>;
   using NotifyObserversCallback =
@@ -51,7 +51,6 @@ class UpdateEngine {
                CrxDownloader::Factory crx_downloader_factory,
                scoped_refptr<PingManager> ping_manager,
                const NotifyObserversCallback& notify_observers_callback);
-  ~UpdateEngine();
 
   bool GetUpdateState(const std::string& id, CrxUpdateItem* update_state);
 
@@ -66,7 +65,10 @@ class UpdateEngine {
                          Callback update_callback);
 
  private:
-  using UpdateContexts = std::set<std::unique_ptr<UpdateContext>>;
+  friend class base::RefCounted<UpdateEngine>;
+  ~UpdateEngine();
+
+  using UpdateContexts = std::set<scoped_refptr<UpdateContext>>;
   using UpdateContextIterator = UpdateContexts::iterator;
 
   void UpdateComplete(UpdateContextIterator it, Error error);
@@ -110,9 +112,8 @@ class UpdateEngine {
   DISALLOW_COPY_AND_ASSIGN(UpdateEngine);
 };
 
-// TODO(sorin): consider making this a ref counted type.
 // Describes a group of components which are installed or updated together.
-struct UpdateContext {
+struct UpdateContext : public base::RefCounted<UpdateContext> {
   UpdateContext(
       scoped_refptr<Configurator> config,
       bool is_foreground,
@@ -122,8 +123,6 @@ struct UpdateContext {
       UpdateEngine::Callback callback,
       CrxDownloader::Factory crx_downloader_factory);
 
-  ~UpdateContext();
-
   scoped_refptr<Configurator> config;
 
   // True if the component is updated as a result of user interaction.
@@ -173,6 +172,9 @@ struct UpdateContext {
   const std::string session_id;
 
  private:
+  friend class base::RefCounted<UpdateContext>;
+  ~UpdateContext();
+
   DISALLOW_COPY_AND_ASSIGN(UpdateContext);
 };