Eliminate the cooldown for direct calls to on-demand updates in CUS.

Also, restricted the access to ComponentUpdateService::GetComponentDetails
so only the friends of the ComponentUpdateService class can call it.


BUG=381199

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@275376 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/component_updater/component_updater_service.cc

@@ -201,6 +201,7 @@ class CrxUpdateService : public ComponentUpdateService, public OnDemandUpdater {
                         const CrxDownloader::Result& download_result);
 
   Status OnDemandUpdateInternal(CrxUpdateItem* item);
+  Status OnDemandUpdateWithCooldown(CrxUpdateItem* item);
 
   void ProcessPendingItems();
 
@@ -956,7 +957,7 @@ void CrxUpdateService::OnNewResourceThrottle(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // Check if we can on-demand update, else unblock the request anyway.
   CrxUpdateItem* item = FindUpdateItemById(crx_id);
-  Status status = OnDemandUpdateInternal(item);
+  Status status = OnDemandUpdateWithCooldown(item);
   if (status == kOk || status == kInProgress) {
     item->throttles.push_back(rt);
     return;
@@ -972,7 +973,7 @@ ComponentUpdateService::Status CrxUpdateService::OnDemandUpdate(
   return OnDemandUpdateInternal(FindUpdateItemById(component_id));
 }
 
-ComponentUpdateService::Status CrxUpdateService::OnDemandUpdateInternal(
+ComponentUpdateService::Status CrxUpdateService::OnDemandUpdateWithCooldown(
     CrxUpdateItem* uit) {
   if (!uit)
     return kError;
@@ -982,6 +983,14 @@ ComponentUpdateService::Status CrxUpdateService::OnDemandUpdateInternal(
   if (delta < base::TimeDelta::FromSeconds(config_->OnDemandDelay()))
     return kError;
 
+  return OnDemandUpdateInternal(uit);
+}
+
+ComponentUpdateService::Status CrxUpdateService::OnDemandUpdateInternal(
+    CrxUpdateItem* uit) {
+  if (!uit)
+    return kError;
+
   Status service_status = GetServiceStatus(uit->status);
   // If the item is already in the process of being updated, there is
   // no point in this call, so return kInProgress.

chrome/browser/component_updater/component_updater_service.h

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/gtest_prod_util.h"
 #include "base/version.h"
 #include "url/gurl.h"
 
@@ -199,12 +200,6 @@ class ComponentUpdateService {
   // Returns a list of registered components.
   virtual std::vector<std::string> GetComponentIDs() const = 0;
 
-  // Returns details about registered component.
-  // Note: Object returned here is owned by this class, in simple words
-  // don't try to free this object.
-  virtual CrxUpdateItem* GetComponentDetails(
-      const std::string& component_id) const = 0;
-
   // Returns an interface for on-demand updates. On-demand updates are
   // proactively triggered outside the normal component update service schedule.
   virtual OnDemandUpdater& GetOnDemandUpdater() = 0;
@@ -212,7 +207,13 @@ class ComponentUpdateService {
   virtual ~ComponentUpdateService() {}
 
  private:
+  // Returns details about registered component. The object returned is owned
+  // by this class. TODO(sorin): replace with a WeakPtr.
+  virtual CrxUpdateItem* GetComponentDetails(
+      const std::string& component_id) const = 0;
+
   friend class ::ComponentsUI;
+  FRIEND_TEST_ALL_PREFIXES(ComponentUpdaterTest, ResourceThrottleLiveNoUpdate);
 };
 
 typedef ComponentUpdateService::Observer ServiceObserver;
@@ -223,7 +224,11 @@ class OnDemandUpdater {
 
   // Returns a network resource throttle. It means that a component will be
   // downloaded and installed before the resource is unthrottled. This function
-  // can be called from the IO thread.
+  // can be called from the IO thread. The function implements a cooldown
+  // interval of 30 minutes. That means it will ineffective to call the
+  // function before the cooldown interval has passed. This behavior is intended
+  // to be defensive against programming bugs, usually triggered by web fetches,
+  // where the on-demand functionality is invoked too often.
   virtual content::ResourceThrottle* GetOnDemandResourceThrottle(
       net::URLRequest* request,
       const std::string& crx_id) = 0;
@@ -237,7 +242,7 @@ class OnDemandUpdater {
   // 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.
+  // on-demand update. The function does not implement any cooldown interval.
   virtual ComponentUpdateService::Status OnDemandUpdate(
       const std::string& component_id) = 0;
 };

chrome/browser/component_updater/test/component_updater_service_unittest.cc

@@ -687,10 +687,11 @@ TEST_F(ComponentUpdaterTest, OnDemandUpdate) {
           "<event eventtype=\"3\" eventresult=\"1\"/>"))
       << post_interceptor_->GetRequestsAsString();
 
-  // Also check what happens if previous check too soon.
+  // Also check what happens if previous check too soon. It works, since this
+  // direct OnDemand call does not implement a cooldown.
   test_configurator()->SetOnDemandTime(60 * 60);
   EXPECT_EQ(
-      ComponentUpdateService::kError,
+      ComponentUpdateService::kOk,
       OnDemandTester::OnDemand(component_updater(), GetCrxComponentID(com2)));
   // Okay, now reset to 0 for the other tests.
   test_configurator()->SetOnDemandTime(0);
@@ -1405,6 +1406,8 @@ class CancelResourceController : public TestResourceController {
   int resume_called_;
 };
 
+// Tests the on-demand update with resource throttle, including the
+// cooldown interval between calls.
 TEST_F(ComponentUpdaterTest, ResourceThrottleLiveNoUpdate) {
   MockServiceObserver observer;
   {
@@ -1419,6 +1422,19 @@ TEST_F(ComponentUpdaterTest, ResourceThrottleLiveNoUpdate) {
     EXPECT_CALL(observer,
                 OnEvent(ServiceObserver::COMPONENT_UPDATER_SLEEPING, ""))
         .Times(1);
+    EXPECT_CALL(observer,
+                OnEvent(ServiceObserver::COMPONENT_UPDATER_STARTED, ""))
+        .Times(1);
+    EXPECT_CALL(observer,
+                OnEvent(ServiceObserver::COMPONENT_NOT_UPDATED,
+                        "abagagagagagagagagagagagagagagag"))
+        .Times(1);
+    EXPECT_CALL(observer,
+                OnEvent(ServiceObserver::COMPONENT_UPDATER_SLEEPING, ""))
+        .Times(1);
+    EXPECT_CALL(observer,
+                OnEvent(ServiceObserver::COMPONENT_UPDATER_STARTED, ""))
+        .Times(1);
   }
 
   EXPECT_TRUE(post_interceptor_->ExpectRequest(
@@ -1441,23 +1457,69 @@ TEST_F(ComponentUpdaterTest, ResourceThrottleLiveNoUpdate) {
 
   EXPECT_EQ(0, post_interceptor_->GetHitCount());
 
-  CancelResourceController controller;
+  {
+    // First on-demand update check is expected to succeeded.
+    CancelResourceController controller;
 
-  BrowserThread::PostTask(
-      BrowserThread::IO,
-      FROM_HERE,
-      base::Bind(base::IgnoreResult(&RequestTestResourceThrottle),
-                 component_updater(),
-                 &controller,
-                 "abagagagagagagagagagagagagagagag"));
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(base::IgnoreResult(&RequestTestResourceThrottle),
+                   component_updater(),
+                   &controller,
+                   "abagagagagagagagagagagagagagagag"));
 
-  RunThreads();
+    RunThreads();
 
-  EXPECT_EQ(1, post_interceptor_->GetHitCount());
-  EXPECT_EQ(0, static_cast<TestInstaller*>(com.installer)->error());
-  EXPECT_EQ(0, static_cast<TestInstaller*>(com.installer)->install_count());
+    EXPECT_EQ(1, post_interceptor_->GetHitCount());
+    EXPECT_EQ(0, static_cast<TestInstaller*>(com.installer)->error());
+    EXPECT_EQ(0, static_cast<TestInstaller*>(com.installer)->install_count());
 
-  component_updater()->Stop();
+    component_updater()->Stop();
+  }
+
+  {
+    // Second on-demand update check is expected to succeed as well, since there
+    // is no cooldown interval between calls, due to calling SetOnDemandTime.
+    test_configurator()->SetOnDemandTime(0);
+    test_configurator()->SetLoopCount(1);
+    component_updater()->Start();
+
+    CancelResourceController controller;
+
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(base::IgnoreResult(&RequestTestResourceThrottle),
+                   component_updater(),
+                   &controller,
+                   "abagagagagagagagagagagagagagagag"));
+
+    RunThreads();
+
+    EXPECT_EQ(1, post_interceptor_->GetHitCount());
+    EXPECT_EQ(0, static_cast<TestInstaller*>(com.installer)->error());
+    EXPECT_EQ(0, static_cast<TestInstaller*>(com.installer)->install_count());
+
+    component_updater()->Stop();
+  }
+
+  {
+    // This on-demand call is expected not to trigger a component update check.
+    test_configurator()->SetOnDemandTime(1000000);
+    component_updater()->Start();
+
+    CancelResourceController controller;
+
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(base::IgnoreResult(&RequestTestResourceThrottle),
+                   component_updater(),
+                   &controller,
+                   "abagagagagagagagagagagagagagagag"));
+    RunThreadsUntilIdle();
+  }
 }
 
 // Tests adding and removing observers.