Handle the case when ChromeConfigurator::UpdateUrl returns no urls.

Since UpdateChecker gets its url configuration from the TestConfigurator,
some of its unit tests will fail is the TestConfigurator is misconfigured
to return an empty list of urls.

Returning no update urls is still an invalid configuration. Whether to allow this case or not is still an open question at this moment.

BUG=497332

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

Cr-Commit-Position: refs/heads/master@{#333408}

4 files changed, 90 insertions, 61 deletions

diff --git a/components/update_client/request_sender.cc b/components/update_client/request_sender.cc
index 6b89ca7..f91cbab 100644
--- a/components/update_client/request_sender.cc
+++ b/components/update_client/request_sender.cc
@@ -28,7 +28,8 @@ void RequestSender::Send(const std::string& request_string,
                          const RequestSenderCallback& request_sender_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (urls.empty()) {
-    request_sender_callback.Run(NULL);
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(request_sender_callback, nullptr));
     return;
   }
 
@@ -64,7 +65,8 @@ void RequestSender::OnURLFetchComplete(const net::URLFetcher* source) {
     return;
   }
 
-  request_sender_callback_.Run(source);
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(request_sender_callback_, source));
 }
 
 }  // namespace update_client
diff --git a/components/update_client/request_sender_unittest.cc b/components/update_client/request_sender_unittest.cc
index 459aea5..4f7a309 100644
--- a/components/update_client/request_sender_unittest.cc
+++ b/components/update_client/request_sender_unittest.cc
@@ -45,8 +45,8 @@ class RequestSenderTest : public testing::Test {
   scoped_ptr<RequestSender> request_sender_;
   scoped_ptr<InterceptorFactory> interceptor_factory_;
 
-  URLRequestPostInterceptor* post_interceptor_1;  // Owned by the factory.
-  URLRequestPostInterceptor* post_interceptor_2;  // Owned by the factory.
+  URLRequestPostInterceptor* post_interceptor_1_;  // Owned by the factory.
+  URLRequestPostInterceptor* post_interceptor_2_;  // Owned by the factory.
 
   const net::URLFetcher* url_fetcher_source_;
 
@@ -58,9 +58,9 @@ class RequestSenderTest : public testing::Test {
 };
 
 RequestSenderTest::RequestSenderTest()
-    : post_interceptor_1(NULL),
-      post_interceptor_2(NULL),
-      url_fetcher_source_(NULL) {
+    : post_interceptor_1_(nullptr),
+      post_interceptor_2_(nullptr),
+      url_fetcher_source_(nullptr) {
 }
 
 RequestSenderTest::~RequestSenderTest() {
@@ -71,12 +71,12 @@ void RequestSenderTest::SetUp() {
                                  base::ThreadTaskRunnerHandle::Get());
   interceptor_factory_.reset(
       new InterceptorFactory(base::ThreadTaskRunnerHandle::Get()));
-  post_interceptor_1 =
+  post_interceptor_1_ =
       interceptor_factory_->CreateInterceptorForPath(kUrlPath1);
-  post_interceptor_2 =
+  post_interceptor_2_ =
       interceptor_factory_->CreateInterceptorForPath(kUrlPath2);
-  EXPECT_TRUE(post_interceptor_1);
-  EXPECT_TRUE(post_interceptor_2);
+  EXPECT_TRUE(post_interceptor_1_);
+  EXPECT_TRUE(post_interceptor_2_);
 
   request_sender_.reset();
 }
@@ -84,8 +84,8 @@ void RequestSenderTest::SetUp() {
 void RequestSenderTest::TearDown() {
   request_sender_.reset();
 
-  post_interceptor_1 = NULL;
-  post_interceptor_2 = NULL;
+  post_interceptor_1_ = nullptr;
+  post_interceptor_2_ = nullptr;
 
   interceptor_factory_.reset();
 
@@ -123,7 +123,7 @@ void RequestSenderTest::RequestSenderComplete(const net::URLFetcher* source) {
 // Tests that when a request to the first url succeeds, the subsequent urls are
 // not tried.
 TEST_F(RequestSenderTest, RequestSendSuccess) {
-  EXPECT_TRUE(post_interceptor_1->ExpectRequest(new PartialMatch("test")));
+  EXPECT_TRUE(post_interceptor_1_->ExpectRequest(new PartialMatch("test")));
 
   std::vector<GURL> urls;
   urls.push_back(GURL(kUrl1));
@@ -134,12 +134,12 @@ TEST_F(RequestSenderTest, RequestSendSuccess) {
                                    base::Unretained(this)));
   RunThreads();
 
-  EXPECT_EQ(1, post_interceptor_1->GetHitCount())
-      << post_interceptor_1->GetRequestsAsString();
-  EXPECT_EQ(1, post_interceptor_1->GetCount())
-      << post_interceptor_1->GetRequestsAsString();
+  EXPECT_EQ(1, post_interceptor_1_->GetHitCount())
+      << post_interceptor_1_->GetRequestsAsString();
+  EXPECT_EQ(1, post_interceptor_1_->GetCount())
+      << post_interceptor_1_->GetRequestsAsString();
 
-  EXPECT_STREQ("test", post_interceptor_1->GetRequests()[0].c_str());
+  EXPECT_STREQ("test", post_interceptor_1_->GetRequests()[0].c_str());
   EXPECT_EQ(GURL(kUrl1), url_fetcher_source_->GetOriginalURL());
   EXPECT_EQ(200, url_fetcher_source_->GetResponseCode());
 }
@@ -147,8 +147,9 @@ TEST_F(RequestSenderTest, RequestSendSuccess) {
 // Tests that the request succeeds using the second url after the first url
 // has failed.
 TEST_F(RequestSenderTest, RequestSendSuccessWithFallback) {
-  EXPECT_TRUE(post_interceptor_1->ExpectRequest(new PartialMatch("test"), 403));
-  EXPECT_TRUE(post_interceptor_2->ExpectRequest(new PartialMatch("test")));
+  EXPECT_TRUE(
+      post_interceptor_1_->ExpectRequest(new PartialMatch("test"), 403));
+  EXPECT_TRUE(post_interceptor_2_->ExpectRequest(new PartialMatch("test")));
 
   std::vector<GURL> urls;
   urls.push_back(GURL(kUrl1));
@@ -159,25 +160,27 @@ TEST_F(RequestSenderTest, RequestSendSuccessWithFallback) {
                                    base::Unretained(this)));
   RunThreads();
 
-  EXPECT_EQ(1, post_interceptor_1->GetHitCount())
-      << post_interceptor_1->GetRequestsAsString();
-  EXPECT_EQ(1, post_interceptor_1->GetCount())
-      << post_interceptor_1->GetRequestsAsString();
-  EXPECT_EQ(1, post_interceptor_2->GetHitCount())
-      << post_interceptor_2->GetRequestsAsString();
-  EXPECT_EQ(1, post_interceptor_2->GetCount())
-      << post_interceptor_2->GetRequestsAsString();
-
-  EXPECT_STREQ("test", post_interceptor_1->GetRequests()[0].c_str());
-  EXPECT_STREQ("test", post_interceptor_2->GetRequests()[0].c_str());
+  EXPECT_EQ(1, post_interceptor_1_->GetHitCount())
+      << post_interceptor_1_->GetRequestsAsString();
+  EXPECT_EQ(1, post_interceptor_1_->GetCount())
+      << post_interceptor_1_->GetRequestsAsString();
+  EXPECT_EQ(1, post_interceptor_2_->GetHitCount())
+      << post_interceptor_2_->GetRequestsAsString();
+  EXPECT_EQ(1, post_interceptor_2_->GetCount())
+      << post_interceptor_2_->GetRequestsAsString();
+
+  EXPECT_STREQ("test", post_interceptor_1_->GetRequests()[0].c_str());
+  EXPECT_STREQ("test", post_interceptor_2_->GetRequests()[0].c_str());
   EXPECT_EQ(GURL(kUrl2), url_fetcher_source_->GetOriginalURL());
   EXPECT_EQ(200, url_fetcher_source_->GetResponseCode());
 }
 
 // Tests that the request fails when both urls have failed.
 TEST_F(RequestSenderTest, RequestSendFailed) {
-  EXPECT_TRUE(post_interceptor_1->ExpectRequest(new PartialMatch("test"), 403));
-  EXPECT_TRUE(post_interceptor_2->ExpectRequest(new PartialMatch("test"), 403));
+  EXPECT_TRUE(
+      post_interceptor_1_->ExpectRequest(new PartialMatch("test"), 403));
+  EXPECT_TRUE(
+      post_interceptor_2_->ExpectRequest(new PartialMatch("test"), 403));
 
   std::vector<GURL> urls;
   urls.push_back(GURL(kUrl1));
@@ -188,19 +191,31 @@ TEST_F(RequestSenderTest, RequestSendFailed) {
                                    base::Unretained(this)));
   RunThreads();
 
-  EXPECT_EQ(1, post_interceptor_1->GetHitCount())
-      << post_interceptor_1->GetRequestsAsString();
-  EXPECT_EQ(1, post_interceptor_1->GetCount())
-      << post_interceptor_1->GetRequestsAsString();
-  EXPECT_EQ(1, post_interceptor_2->GetHitCount())
-      << post_interceptor_2->GetRequestsAsString();
-  EXPECT_EQ(1, post_interceptor_2->GetCount())
-      << post_interceptor_2->GetRequestsAsString();
-
-  EXPECT_STREQ("test", post_interceptor_1->GetRequests()[0].c_str());
-  EXPECT_STREQ("test", post_interceptor_2->GetRequests()[0].c_str());
+  EXPECT_EQ(1, post_interceptor_1_->GetHitCount())
+      << post_interceptor_1_->GetRequestsAsString();
+  EXPECT_EQ(1, post_interceptor_1_->GetCount())
+      << post_interceptor_1_->GetRequestsAsString();
+  EXPECT_EQ(1, post_interceptor_2_->GetHitCount())
+      << post_interceptor_2_->GetRequestsAsString();
+  EXPECT_EQ(1, post_interceptor_2_->GetCount())
+      << post_interceptor_2_->GetRequestsAsString();
+
+  EXPECT_STREQ("test", post_interceptor_1_->GetRequests()[0].c_str());
+  EXPECT_STREQ("test", post_interceptor_2_->GetRequests()[0].c_str());
   EXPECT_EQ(GURL(kUrl2), url_fetcher_source_->GetOriginalURL());
   EXPECT_EQ(403, url_fetcher_source_->GetResponseCode());
 }
 
+// Tests that the request fails when no urls are provided.
+TEST_F(RequestSenderTest, RequestSendFailedNoUrls) {
+  std::vector<GURL> urls;
+  request_sender_.reset(new RequestSender(*config_));
+  request_sender_->Send("test", urls,
+                        base::Bind(&RequestSenderTest::RequestSenderComplete,
+                                   base::Unretained(this)));
+  RunThreads();
+
+  EXPECT_EQ(nullptr, url_fetcher_source_);
+}
+
 }  // namespace update_client
diff --git a/components/update_client/update_checker.cc b/components/update_client/update_checker.cc
index 9a38a5a..be686ef 100644
--- a/components/update_client/update_checker.cc
+++ b/components/update_client/update_checker.cc
@@ -15,6 +15,7 @@
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/stringprintf.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/threading/thread_checker.h"
 #include "components/update_client/configurator.h"
 #include "components/update_client/crx_update_item.h"
@@ -128,30 +129,39 @@ bool UpdateCheckerImpl::CheckForUpdates(
 void UpdateCheckerImpl::OnRequestSenderComplete(const net::URLFetcher* source) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  const GURL original_url(source->GetOriginalURL());
-  VLOG(1) << "Update check request went to: " << original_url.spec();
-
+  GURL original_url;
   int error = 0;
   std::string error_message;
   UpdateResponse update_response;
 
-  if (FetchSuccess(*source)) {
-    std::string xml;
-    source->GetResponseAsString(&xml);
-    if (!update_response.Parse(xml)) {
-      error = -1;
-      error_message = update_response.errors();
-      VLOG(1) << "Update request failed: " << error_message;
+  if (source) {
+    original_url = source->GetOriginalURL();
+    VLOG(1) << "Update check request went to: " << original_url.spec();
+    if (FetchSuccess(*source)) {
+      std::string xml;
+      source->GetResponseAsString(&xml);
+      if (!update_response.Parse(xml)) {
+        error = -1;
+        error_message = update_response.errors();
+      }
+    } else {
+      error = GetFetchError(*source);
+      error_message.assign("network error");
     }
   } else {
-    error = GetFetchError(*source);
-    error_message.assign("network error");
-    VLOG(1) << "Update request failed: network error";
+    error = -1;
+    error_message = "no fetcher";
+  }
+
+  if (error) {
+    VLOG(1) << "Update request failed: " << error_message;
   }
 
   request_sender_.reset();
-  update_check_callback_.Run(original_url, error, error_message,
-                             update_response.results());
+
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(update_check_callback_, original_url, error,
+                            error_message, update_response.results()));
 }
 
 }  // namespace
diff --git a/components/update_client/update_checker_unittest.cc b/components/update_client/update_checker_unittest.cc
index 117aca29..0bea798 100644
--- a/components/update_client/update_checker_unittest.cc
+++ b/components/update_client/update_checker_unittest.cc
@@ -181,7 +181,7 @@ TEST_F(UpdateCheckerTest, UpdateCheckSuccess) {
 
   EXPECT_EQ(1, post_interceptor_->GetHitCount())
       << post_interceptor_->GetRequestsAsString();
-  EXPECT_EQ(1, post_interceptor_->GetCount())
+  ASSERT_EQ(1, post_interceptor_->GetCount())
       << post_interceptor_->GetRequestsAsString();
 
   // Sanity check the request.
@@ -197,6 +197,7 @@ TEST_F(UpdateCheckerTest, UpdateCheckSuccess) {
             post_interceptor_->GetRequests()[0].find("<hw physmemory="));
 
   // Sanity check the arguments of the callback after parsing.
+  ASSERT_FALSE(config_->UpdateUrl().empty());
   EXPECT_EQ(config_->UpdateUrl().front(), original_url_);
   EXPECT_EQ(0, error_);
   EXPECT_TRUE(error_message_.empty());
@@ -228,6 +229,7 @@ TEST_F(UpdateCheckerTest, UpdateCheckError) {
   EXPECT_EQ(1, post_interceptor_->GetCount())
       << post_interceptor_->GetRequestsAsString();
 
+  ASSERT_FALSE(config_->UpdateUrl().empty());
   EXPECT_EQ(config_->UpdateUrl().front(), original_url_);
   EXPECT_EQ(403, error_);
   EXPECT_STREQ("network error", error_message_.c_str());