Provide a callback for Component Updater OnDemand calls.

The idea here is that there is no good way for a client to know when a subsequent OnDemand call can be issued without getting an Error::ERROR_UPDATE_IN_PROGRESS. This issue showed up when writing a browser test for an unrelated component updater feature. This change is mostly mechanical, especially for the existing users of the OnDemand interface. All existing call sites pass a null callback as an argument, therefore no new code paths are introduced with this change. BUG=639180 Review-Url: https://codereview.chromium.org/2261533003 Cr-Commit-Position: refs/heads/master@{#413178}
Denger-Network · Aug 19, 2016 · 4c52018 · 4c52018
1 parent 197e56b
commit 4c52018
Show file tree

Hide file tree

Showing 8 changed files with 88 additions and 43 deletions.
diff --git a/chrome/browser/component_updater/supervised_user_whitelist_installer.cc b/chrome/browser/component_updater/supervised_user_whitelist_installer.cc
@@ -642,8 +642,8 @@ std::vector<uint8_t> SupervisedUserWhitelistInstaller::GetHashFromCrxId(
 void SupervisedUserWhitelistInstaller::TriggerComponentUpdate(
     OnDemandUpdater* updater,
     const std::string& crx_id) {
-  const bool result = updater->OnDemandUpdate(crx_id);
-  DCHECK(result);
+  // TODO(sorin): use a callback to check the result (crbug.com/639189).
+  updater->OnDemandUpdate(crx_id, ComponentUpdateService::CompletionCallback());
 }
 
 }  // namespace component_updater
diff --git a/chrome/browser/component_updater/supervised_user_whitelist_installer_unittest.cc b/chrome/browser/component_updater/supervised_user_whitelist_installer_unittest.cc
@@ -142,16 +142,17 @@ class MockComponentUpdateService : public ComponentUpdateService,
   }
 
   // OnDemandUpdater implementation:
-  bool OnDemandUpdate(const std::string& crx_id) override {
+  void OnDemandUpdate(
+      const std::string& crx_id,
+      ComponentUpdateService::CompletionCallback callback) override {
     on_demand_update_called_ = true;
 
     if (!component_) {
       ADD_FAILURE() << "Trying to update unregistered component " << crx_id;
-      return false;
+      return;
     }
 
     EXPECT_EQ(GetCrxComponentID(*component_), crx_id);
-    return true;
   }
 
  private:

diff --git a/chrome/browser/plugins/plugin_observer.cc b/chrome/browser/plugins/plugin_observer.cc
@@ -417,7 +417,8 @@ void PluginObserver::OnBlockedComponentUpdatedPlugin(
   component_observers_[placeholder_id] =
       base::MakeUnique<ComponentObserver>(this, placeholder_id, identifier);
   g_browser_process->component_updater()->GetOnDemandUpdater().OnDemandUpdate(
-      identifier);
+      identifier,
+      component_updater::ComponentUpdateService::CompletionCallback());
 }
 
 void PluginObserver::RemoveComponentObserver(int placeholder_id) {

diff --git a/chrome/browser/ui/webui/components_ui.cc b/chrome/browser/ui/webui/components_ui.cc
@@ -159,7 +159,9 @@ ComponentsUI::~ComponentsUI() {
 void ComponentsUI::OnDemandUpdate(const std::string& component_id) {
   component_updater::ComponentUpdateService* cus =
       g_browser_process->component_updater();
-  cus->GetOnDemandUpdater().OnDemandUpdate(component_id);
+  cus->GetOnDemandUpdater().OnDemandUpdate(
+      component_id,
+      component_updater::ComponentUpdateService::CompletionCallback());
 }
 
 // static

diff --git a/components/component_updater/component_updater_service.cc b/components/component_updater/component_updater_service.cc
@@ -239,13 +239,19 @@ void CrxUpdateService::MaybeThrottle(const std::string& id,
   callback.Run();  // Unblock the request if the request can't be throttled.
 }
 
-bool CrxUpdateService::OnDemandUpdate(const std::string& id) {
+void CrxUpdateService::OnDemandUpdate(const std::string& id,
+                                      CompletionCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (!GetComponent(id))
-    return false;
+  if (!GetComponent(id)) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(callback,
+                   update_client::Error::ERROR_UPDATE_INVALID_ARGUMENT));
+    return;
+  }
 
-  return OnDemandUpdateInternal(id);
+  OnDemandUpdateInternal(id, callback);
 }
 
 bool CrxUpdateService::OnDemandUpdateWithCooldown(const std::string& id) {
@@ -261,21 +267,20 @@ bool CrxUpdateService::OnDemandUpdateWithCooldown(const std::string& id) {
       return false;
   }
 
-  return OnDemandUpdateInternal(id);
+  OnDemandUpdateInternal(id, CompletionCallback());
+  return true;
 }
 
-bool CrxUpdateService::OnDemandUpdateInternal(const std::string& id) {
+void CrxUpdateService::OnDemandUpdateInternal(const std::string& id,
+                                              CompletionCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   UMA_HISTOGRAM_ENUMERATION("ComponentUpdater.Calls", UPDATE_TYPE_MANUAL,
                             UPDATE_TYPE_COUNT);
-
   update_client_->Install(
       id, base::Bind(&CrxUpdateService::OnUpdate, base::Unretained(this)),
       base::Bind(&CrxUpdateService::OnUpdateComplete, base::Unretained(this),
-                 base::TimeTicks::Now()));
-
-  return true;
+                 callback, base::TimeTicks::Now()));
 }
 
 bool CrxUpdateService::CheckForUpdates() {
@@ -301,15 +306,15 @@ bool CrxUpdateService::CheckForUpdates() {
         unsecure_ids,
         base::Bind(&CrxUpdateService::OnUpdate, base::Unretained(this)),
         base::Bind(&CrxUpdateService::OnUpdateComplete, base::Unretained(this),
-                   base::TimeTicks::Now()));
+                   CompletionCallback(), base::TimeTicks::Now()));
   }
 
   if (!secure_ids.empty()) {
     update_client_->Update(
         secure_ids,
         base::Bind(&CrxUpdateService::OnUpdate, base::Unretained(this)),
         base::Bind(&CrxUpdateService::OnUpdateComplete, base::Unretained(this),
-                   base::TimeTicks::Now()));
+                   CompletionCallback(), base::TimeTicks::Now()));
   }
 
   return true;
@@ -354,7 +359,8 @@ void CrxUpdateService::OnUpdate(const std::vector<std::string>& ids,
   }
 }
 
-void CrxUpdateService::OnUpdateComplete(const base::TimeTicks& start_time,
+void CrxUpdateService::OnUpdateComplete(CompletionCallback callback,
+                                        const base::TimeTicks& start_time,
                                         int error) {
   DCHECK(thread_checker_.CalledOnValidThread());
   VLOG(1) << "Update completed with error " << error;
@@ -370,6 +376,11 @@ void CrxUpdateService::OnUpdateComplete(const base::TimeTicks& start_time,
         DoUnregisterComponent(*component);
     }
   }
+
+  if (!callback.is_null()) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  base::Bind(callback, error));
+  }
 }
 
 void CrxUpdateService::OnEvent(Events event, const std::string& id) {

diff --git a/components/component_updater/component_updater_service.h b/components/component_updater/component_updater_service.h
@@ -77,6 +77,7 @@ struct ComponentInfo {
 // All methods are safe to call ONLY from the browser's main thread.
 class ComponentUpdateService {
  public:
+  using CompletionCallback = update_client::UpdateClient::CompletionCallback;
   using Observer = update_client::UpdateClient::Observer;
 
   // Adds an observer for this class. An observer should not be added more
@@ -162,11 +163,11 @@ class OnDemandUpdater {
   // returned by GetCrxComponentID(). If an update for this component is already
   // in progress, the function returns |kInProgress|. If an update is available,
   // the update will be applied. The caller can subscribe to component update
-  // service notifications to get an indication about the outcome of the
-  // on-demand update. The function does not implement any cooldown interval.
-  // TODO(sorin): improve this API so that the result of this non-blocking
-  // call is provided by a callback.
-  virtual bool OnDemandUpdate(const std::string& id) = 0;
+  // service notifications and provide an optional callback to get the result
+  // of the call. The function does not implement any cooldown interval.
+  virtual void OnDemandUpdate(
+      const std::string& id,
+      ComponentUpdateService::CompletionCallback callback) = 0;
 };
 
 // Creates the component updater.

diff --git a/components/component_updater/component_updater_service_internal.h b/components/component_updater/component_updater_service_internal.h
@@ -56,15 +56,17 @@ class CrxUpdateService : public ComponentUpdateService,
   void OnEvent(Events event, const std::string& id) override;
 
   // Overrides for OnDemandUpdater.
-  bool OnDemandUpdate(const std::string& id) override;
+  void OnDemandUpdate(const std::string& id,
+                      CompletionCallback callback) override;
 
  private:
   void Start();
   void Stop();
 
   bool CheckForUpdates();
 
-  bool OnDemandUpdateInternal(const std::string& id);
+  void OnDemandUpdateInternal(const std::string& id,
+                              CompletionCallback callback);
   bool OnDemandUpdateWithCooldown(const std::string& id);
 
   bool DoUnregisterComponent(const CrxComponent& component);
@@ -75,7 +77,9 @@ class CrxUpdateService : public ComponentUpdateService,
 
   void OnUpdate(const std::vector<std::string>& ids,
                 std::vector<CrxComponent>* components);
-  void OnUpdateComplete(const base::TimeTicks& start_time, int error);
+  void OnUpdateComplete(CompletionCallback callback,
+                        const base::TimeTicks& start_time,
+                        int error);
 
   base::ThreadChecker thread_checker_;
 

diff --git a/components/component_updater/component_updater_service_unittest.cc b/components/component_updater/component_updater_service_unittest.cc
@@ -127,7 +127,13 @@ class ComponentUpdaterTest : public testing::Test {
 
 class OnDemandTester {
  public:
-  static bool OnDemand(ComponentUpdateService* cus, const std::string& id);
+  void OnDemand(ComponentUpdateService* cus, const std::string& id);
+  int error() const { return error_; }
+
+ private:
+  void OnDemandComplete(int error);
+
+  int error_ = 0;
 };
 
 MockInstaller::MockInstaller() {
@@ -148,9 +154,15 @@ MockServiceObserver::MockServiceObserver() {
 MockServiceObserver::~MockServiceObserver() {
 }
 
-bool OnDemandTester::OnDemand(ComponentUpdateService* cus,
+void OnDemandTester::OnDemand(ComponentUpdateService* cus,
                               const std::string& id) {
-  return cus->GetOnDemandUpdater().OnDemandUpdate(id);
+  cus->GetOnDemandUpdater().OnDemandUpdate(
+      id,
+      base::Bind(&OnDemandTester::OnDemandComplete, base::Unretained(this)));
+}
+
+void OnDemandTester::OnDemandComplete(int error) {
+  error_ = error;
 }
 
 std::unique_ptr<ComponentUpdateService> TestComponentUpdateServiceFactory(
@@ -274,8 +286,8 @@ TEST_F(ComponentUpdaterTest, RegisterComponent) {
 TEST_F(ComponentUpdaterTest, OnDemandUpdate) {
   class LoopHandler {
    public:
-    LoopHandler(int max_cnt, const base::Closure& quit_closure)
-        : max_cnt_(max_cnt), quit_closure_(quit_closure) {}
+    LoopHandler(int max_cnt)
+        : max_cnt_(max_cnt) {}
 
     void OnInstall(
         const std::string& ids,
@@ -284,13 +296,16 @@ TEST_F(ComponentUpdaterTest, OnDemandUpdate) {
       completion_callback.Run(0);
       static int cnt = 0;
       ++cnt;
-      if (cnt >= max_cnt_)
-        quit_closure_.Run();
+      if (cnt >= max_cnt_) {
+        base::ThreadTaskRunnerHandle::Get()->PostTask(
+            FROM_HERE, base::Bind(&LoopHandler::Quit, base::Unretained(this)));
+      }
     }
 
    private:
+    void Quit() { base::MessageLoop::current()->QuitWhenIdle(); }
+
     const int max_cnt_;
-    base::Closure quit_closure_;
   };
 
   base::HistogramTester ht;
@@ -300,27 +315,37 @@ TEST_F(ComponentUpdaterTest, OnDemandUpdate) {
 
   auto& cus = component_updater();
 
-  const std::string id = "jebgalgnebhfojomionfpkfelancnnkf";
-  EXPECT_FALSE(OnDemandTester::OnDemand(&cus, id));
+  // Tests calling OnDemand for an unregistered component. This call results in
+  // an error, which is recorded by the OnDemandTester instance. Since the
+  // component was not registered, the call is ignored for UMA metrics.
+  OnDemandTester ondemand_tester_component_not_registered;
+  ondemand_tester_component_not_registered.OnDemand(
+      &cus, "ihfokbkgjpifnbbojhneepfflplebdkc");
 
-  scoped_refptr<MockInstaller> installer(new MockInstaller());
+  const std::string id = "jebgalgnebhfojomionfpkfelancnnkf";
 
   using update_client::jebg_hash;
   CrxComponent crx_component;
   crx_component.pk_hash.assign(jebg_hash, jebg_hash + arraysize(jebg_hash));
   crx_component.version = Version("0.9");
-  crx_component.installer = installer;
+  crx_component.installer = new MockInstaller();
 
-  LoopHandler loop_handler(1, quit_closure());
+  LoopHandler loop_handler(1);
   EXPECT_CALL(update_client(),
               Install("jebgalgnebhfojomionfpkfelancnnkf", _, _))
       .WillOnce(Invoke(&loop_handler, &LoopHandler::OnInstall));
   EXPECT_CALL(update_client(), Stop()).Times(1);
 
   EXPECT_TRUE(cus.RegisterComponent(crx_component));
-  EXPECT_TRUE(OnDemandTester::OnDemand(&cus, id));
+  OnDemandTester ondemand_tester;
+  ondemand_tester.OnDemand(&cus, id);
 
-  RunThreads();
+  base::RunLoop().Run();
+
+  EXPECT_EQ(
+      static_cast<int>(update_client::Error::ERROR_UPDATE_INVALID_ARGUMENT),
+      ondemand_tester_component_not_registered.error());
+  EXPECT_EQ(0, ondemand_tester.error());
 
   ht.ExpectUniqueSample("ComponentUpdater.Calls", 0, 1);
   ht.ExpectUniqueSample("ComponentUpdater.UpdateCompleteResult", 0, 1);