Component updater eight piece

"in which we beef up the unit tests and find several bugs"

1- When parsing the xml response components not mentioned there were left in "updating" state so they got stranded there.
2- Wrong condition :
   if ((item->status != CrxUpdateItem::kNoUpdate) ||

       (item->status != CrxUpdateItem::kUpToDate))
   So again some components are stranded in the kNoUpdate state.

Also a new precondition ScheduleNextRun() cannot be called if a url_fetcher is in progress.

BUG=61602
TEST=included
Review URL: http://codereview.chromium.org/7601019

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

chrome/browser/component_updater/component_updater_interceptor.cc

@@ -9,7 +9,8 @@
 #include "net/url_request/url_request_test_job.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-ComponentUpdateInterceptor::ComponentUpdateInterceptor() {
+ComponentUpdateInterceptor::ComponentUpdateInterceptor()
+  : hit_count_(0) {
   net::URLRequest::Deprecated::RegisterRequestInterceptor(this);
 }
 
@@ -34,6 +35,7 @@ net::URLRequestJob* ComponentUpdateInterceptor::MaybeIntercept(
     return NULL;
   }
   const Response& response = it->second;
+  ++hit_count_;
 
   std::string contents;
   EXPECT_TRUE(file_util::ReadFileToString(response.data_path, &contents));

chrome/browser/component_updater/component_updater_interceptor.h

@@ -33,8 +33,11 @@ class ComponentUpdateInterceptor
                    const std::string& headers,
                    const FilePath& path);
 
+  // Returns how many requests have been issued that have a stored reply.
+  int hit_count() const { return hit_count_; }
+
  private:
-  // When computing matches, this ignores the query parameters of the url. 
+  // When computing matches, this ignores the query parameters of the url.
   virtual net::URLRequestJob* MaybeIntercept(net::URLRequest* request) OVERRIDE;
 
   friend class base::RefCountedThreadSafe<ComponentUpdateInterceptor>;
@@ -48,6 +51,7 @@ class ComponentUpdateInterceptor
 
   typedef std::map<GURL, Response> ResponseMap;
   ResponseMap responses_;
+  int hit_count_;
 
   DISALLOW_COPY_AND_ASSIGN(ComponentUpdateInterceptor);
 };

chrome/browser/component_updater/component_updater_service.cc

@@ -344,6 +344,7 @@ ComponentUpdateService::Status CrxUpdateService::Stop() {
 // long one.
 void CrxUpdateService::ScheduleNextRun(bool step_delay) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(url_fetcher_.get() == NULL);
   CHECK(!timer_.IsRunning());
   // It could be the case that Stop() had been called while a url request
   // or unpacking was in flight, if so we arrive here but |running_| is
@@ -351,17 +352,18 @@ void CrxUpdateService::ScheduleNextRun(bool step_delay) {
   if (!running_)
     return;
 
+  int64 delay = step_delay ? config_->StepDelay() : config_->NextCheckDelay();
+
   if (!step_delay) {
     NotificationService::current()->Notify(
         chrome::NOTIFICATION_COMPONENT_UPDATER_SLEEPING,
         Source<ComponentUpdateService>(this),
         NotificationService::NoDetails());
     // Zero is only used for unit tests.
-    if (0 == config_->NextCheckDelay())
+    if (0 == delay)
       return;
   }
 
-  int64 delay = step_delay ? config_->StepDelay() : config_->NextCheckDelay();
   timer_.Start(base::TimeDelta::FromSeconds(delay),
                this, &CrxUpdateService::ProcessPendingItems);
 }
@@ -489,7 +491,7 @@ void CrxUpdateService::ProcessPendingItems() {
   for (UpdateItems::const_iterator it = work_items_.begin();
        it != work_items_.end(); ++it) {
     CrxUpdateItem* item = *it;
-    if ((item->status != CrxUpdateItem::kNoUpdate) ||
+    if ((item->status != CrxUpdateItem::kNoUpdate) &&
         (item->status != CrxUpdateItem::kUpToDate))
       continue;
     base::TimeDelta delta = base::Time::Now() - item->last_check;
@@ -532,11 +534,12 @@ void CrxUpdateService::OnURLFetchComplete(const URLFetcher* source,
   if (FetchSuccess(*source)) {
     std::string xml;
     source->GetResponseAsString(&xml);
+    url_fetcher_.reset();
     ParseManifest(xml);
   } else {
+    url_fetcher_.reset();
     CrxUpdateService::OnParseUpdateManifestFailed("network error");
   }
-  url_fetcher_.reset();
 }
 
 // Parsing the manifest is either done right now for tests or in a sandboxed
@@ -577,16 +580,21 @@ void CrxUpdateService::OnParseUpdateManifestSucceeded(
       continue;  // Not updating this component now.
 
     if (it->version.empty()) {
+      // No version means no update available.
       crx->status = CrxUpdateItem::kNoUpdate;
-      continue;  // No version means no update available.
+      continue;
     }
     if (!IsVersionNewer(crx->component.version, it->version)) {
+      // Our component is up to date.
       crx->status = CrxUpdateItem::kUpToDate;
-      continue;  // Our component is up to date.
+      continue;
     }
     if (!it->browser_min_version.empty()) {
-      if (IsVersionNewer(chrome_version_, it->browser_min_version))
-        continue;  // Does not apply for this version.
+      if (IsVersionNewer(chrome_version_, it->browser_min_version)) {
+        // Does not apply for this chrome version.
+        crx->status = CrxUpdateItem::kNoUpdate;
+        continue;
+      }
     }
     // All test passed. Queue an upgrade for this component and fire the
     // notifications.
@@ -599,6 +607,11 @@ void CrxUpdateService::OnParseUpdateManifestSucceeded(
         Source<std::string>(&crx->id),
         NotificationService::NoDetails());
   }
+
+  // All the components that are not mentioned in the manifest we
+  // consider them up to date.
+  ChangeItemStatus(CrxUpdateItem::kChecking, CrxUpdateItem::kUpToDate);
+
   // If there are updates pending we do a short wait.
   ScheduleNextRun(update_pending ? true : false);
 }
@@ -624,6 +637,7 @@ void CrxUpdateService::OnURLFetchComplete(const URLFetcher* source,
     size_t count = ChangeItemStatus(CrxUpdateItem::kDownloading,
                                     CrxUpdateItem::kNoUpdate);
     DCHECK_EQ(count, 1ul);
+    url_fetcher_.reset();
     ScheduleNextRun(false);
   } else {
     FilePath temp_crx_path;
@@ -631,6 +645,8 @@ void CrxUpdateService::OnURLFetchComplete(const URLFetcher* source,
     size_t count = ChangeItemStatus(CrxUpdateItem::kDownloading,
                                     CrxUpdateItem::kUpdating);
     DCHECK_EQ(count, 1ul);
+    url_fetcher_.reset();
+
     NotificationService::current()->Notify(
         chrome::NOTIFICATION_COMPONENT_UPDATE_READY,
         Source<std::string>(&context->id),
@@ -642,8 +658,6 @@ void CrxUpdateService::OnURLFetchComplete(const URLFetcher* source,
                           temp_crx_path),
         config_->StepDelay());
   }
-
-  url_fetcher_.reset();
 }
 
 // Install consists of digital signature verification, unpacking and then

chrome/browser/component_updater/component_updater_service_unittest.cc

@@ -30,6 +30,9 @@ namespace {
 // and loops faster. In actual usage it takes hours do to a full cycle.
 class TestConfigurator : public ComponentUpdateService::Configurator {
  public:
+  TestConfigurator() : times_(1) {
+  }
+
   virtual int InitialDelay() OVERRIDE { return 0; }
 
   virtual int NextCheckDelay() OVERRIDE {
@@ -37,6 +40,9 @@ class TestConfigurator : public ComponentUpdateService::Configurator {
     // to happen. In test we normally only test one cycle so it is a good
     // time to break from the test messageloop Run() method so the test can
     // finish.
+    if (--times_ > 0)
+      return 1;
+
     MessageLoop::current()->Quit();
     return 0;
   }
@@ -59,6 +65,12 @@ class TestConfigurator : public ComponentUpdateService::Configurator {
 
   // Don't use the utility process to decode files.
   virtual bool InProcess() OVERRIDE { return true; }
+
+  // Set how many update checks are called, the default value is just once.
+  void SetLoopCount(int times) { times_ = times; }
+
+ private:
+  int times_;
 };
 
 class TestInstaller : public ComponentInstaller {
@@ -110,10 +122,10 @@ const char header_ok_reply[] =
 // Common fixture for all the component updater tests.
 class ComponentUpdaterTest : public TestingBrowserProcessTest {
  public:
-  ComponentUpdaterTest() : component_updater_(NULL) {
+  ComponentUpdaterTest() : component_updater_(NULL), test_config_(NULL) {
     // The component updater instance under test.
-    component_updater_.reset(
-        ComponentUpdateServiceFactory(new TestConfigurator));
+    test_config_ = new TestConfigurator;
+    component_updater_.reset(ComponentUpdateServiceFactory(test_config_));
     // The test directory is chrome/test/data/components.
     PathService::Get(chrome::DIR_TEST_DATA, &test_data_dir_);
     test_data_dir_ = test_data_dir_.AppendASCII("components");
@@ -150,10 +162,15 @@ class ComponentUpdaterTest : public TestingBrowserProcessTest {
     return notification_tracker_;
   }
 
+  TestConfigurator* test_configurator() {
+    return test_config_;
+  }
+
  private:
   scoped_ptr<ComponentUpdateService> component_updater_;
   FilePath test_data_dir_;
   TestNotificationTracker notification_tracker_;
+  TestConfigurator* test_config_;
 };
 
 // Verify that our test fixture work and the component updater can
@@ -208,6 +225,8 @@ TEST_F(ComponentUpdaterTest, CheckCrxSleep) {
                            header_ok_reply,
                            test_file("updatecheck_reply_1.xml"));
 
+  // We loop twice, but there are no updates so we expect two sleep messages.
+  test_configurator()->SetLoopCount(2);
   component_updater()->Start();
 
   ASSERT_EQ(1ul, notification_tracker().size());
@@ -216,9 +235,39 @@ TEST_F(ComponentUpdaterTest, CheckCrxSleep) {
 
   message_loop.Run();
 
-  ASSERT_EQ(2ul, notification_tracker().size());
+  ASSERT_EQ(3ul, notification_tracker().size());
   TestNotificationTracker::Event ev2 = notification_tracker().at(1);
   EXPECT_EQ(chrome::NOTIFICATION_COMPONENT_UPDATER_SLEEPING, ev2.type);
+  TestNotificationTracker::Event ev3 = notification_tracker().at(2);
+  EXPECT_EQ(chrome::NOTIFICATION_COMPONENT_UPDATER_SLEEPING, ev2.type);
+  EXPECT_EQ(2, interceptor->hit_count());
+
+  EXPECT_EQ(0, static_cast<TestInstaller*>(com.installer)->error());
+  EXPECT_EQ(0, static_cast<TestInstaller*>(com.installer)->install_count());
+
+  component_updater()->Stop();
+
+  // Loop twice again but this case we simulate a server error by returning
+  // an empty file.
+
+  interceptor->SetResponse(expected_update_url,
+                           header_ok_reply,
+                           test_file("updatecheck_reply_empty"));
+
+  notification_tracker().Reset();
+  test_configurator()->SetLoopCount(2);
+  component_updater()->Start();
+
+  message_loop.Run();
+
+  ASSERT_EQ(3ul, notification_tracker().size());
+  ev1 = notification_tracker().at(0);
+  EXPECT_EQ(chrome::NOTIFICATION_COMPONENT_UPDATER_STARTED, ev1.type);
+  ev2 = notification_tracker().at(1);
+  EXPECT_EQ(chrome::NOTIFICATION_COMPONENT_UPDATER_SLEEPING, ev2.type);
+  ev3 = notification_tracker().at(2);
+  EXPECT_EQ(chrome::NOTIFICATION_COMPONENT_UPDATER_SLEEPING, ev2.type);
+  EXPECT_EQ(4, interceptor->hit_count());
 
   EXPECT_EQ(0, static_cast<TestInstaller*>(com.installer)->error());
   EXPECT_EQ(0, static_cast<TestInstaller*>(com.installer)->install_count());