Make URLFetchers with SetAutomaticallyRetryOnNetworkChanges() retry i…

…mmediately regardless of network state.

If the network went offline then the retry attempt will fail immediately, and pass INTERNET_DISCONNECTED to the client.
This is less surprising than "hanging" fetchers.

BUG=164363,168390


Review URL: https://chromiumcodereview.appspot.com/11821055

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

net/url_request/url_fetcher.h

@@ -196,9 +196,6 @@ class NET_EXPORT URLFetcher {
   // Retries up to |max_retries| times when requests fail with
   // ERR_NETWORK_CHANGED. If ERR_NETWORK_CHANGED is received after having
   // retried |max_retries| times then it is propagated to the observer.
-  // Each retry can be delayed if the network if currently offline, and will be
-  // attempted once the network connection is back. The first fetch is also
-  // delayed if the network is offline when Start() is invoked.
   virtual void SetAutomaticallyRetryOnNetworkChanges(int max_retries) = 0;
 
   // By default, the response is saved in a string. Call this method to save the

net/url_request/url_fetcher_core.cc

@@ -307,17 +307,8 @@ void URLFetcherCore::Start() {
   }
   DCHECK(network_task_runner_.get()) << "We need an IO task runner";
 
-  if (num_retries_on_network_changes_ < max_retries_on_network_changes_ &&
-      NetworkChangeNotifier::IsOffline()) {
-    // We're currently offline and this request will immediately fail. Try to
-    // start later if |max_retries_on_network_changes_| is set, indicating that
-    // our owner wants the fetcher to automatically retry on network changes.
-    ++num_retries_on_network_changes_;
-    NetworkChangeNotifier::AddConnectionTypeObserver(this);
-  } else {
-    network_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&URLFetcherCore::StartOnIOThread, this));
-  }
+  network_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&URLFetcherCore::StartOnIOThread, this));
 }
 
 void URLFetcherCore::Stop() {
@@ -628,20 +619,6 @@ void URLFetcherCore::OnCertificateRequested(
   request->ContinueWithCertificate(NULL);
 }
 
-void URLFetcherCore::OnConnectionTypeChanged(
-    NetworkChangeNotifier::ConnectionType type) {
-  DCHECK_GT(num_retries_on_network_changes_, 0);
-  if (type == NetworkChangeNotifier::CONNECTION_NONE) {
-    // Keep waiting.
-    return;
-  }
-
-  // Stop observing and try again now.
-  NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
-  network_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&URLFetcherCore::StartOnIOThread, this));
-}
-
 void URLFetcherCore::CancelAll() {
   g_registry.Get().CancelAll();
 }
@@ -900,15 +877,10 @@ void URLFetcherCore::RetryOrCompleteUrlFetch() {
       num_retries_on_network_changes_ < max_retries_on_network_changes_) {
     ++num_retries_on_network_changes_;
 
-    if (NetworkChangeNotifier::IsOffline()) {
-      // Retry once we're back online.
-      NetworkChangeNotifier::AddConnectionTypeObserver(this);
-    } else {
-      // Retry soon, after flushing all the current tasks which may include
-      // further network change observers.
-      network_task_runner_->PostTask(
-          FROM_HERE, base::Bind(&URLFetcherCore::StartOnIOThread, this));
-    }
+    // Retry soon, after flushing all the current tasks which may include
+    // further network change observers.
+    network_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&URLFetcherCore::StartOnIOThread, this));
     return;
   }
 
@@ -926,8 +898,6 @@ void URLFetcherCore::RetryOrCompleteUrlFetch() {
 }
 
 void URLFetcherCore::ReleaseRequest() {
-  if (num_retries_on_network_changes_ > 0)
-    NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
   upload_progress_checker_timer_.reset();
   request_.reset();
   g_registry.Get().RemoveURLFetcherCore(this);

net/url_request/url_fetcher_core.h

@@ -20,7 +20,6 @@
 #include "base/timer.h"
 #include "googleurl/src/gurl.h"
 #include "net/base/host_port_pair.h"
-#include "net/base/network_change_notifier.h"
 #include "net/http/http_request_headers.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request.h"
@@ -39,8 +38,7 @@ class URLRequestThrottlerEntryInterface;
 
 class URLFetcherCore
     : public base::RefCountedThreadSafe<URLFetcherCore>,
-      public URLRequest::Delegate,
-      public NetworkChangeNotifier::ConnectionTypeObserver {
+      public URLRequest::Delegate {
  public:
   URLFetcherCore(URLFetcher* fetcher,
                  const GURL& original_url,
@@ -128,10 +126,6 @@ class URLFetcherCore
       URLRequest* request,
       SSLCertRequestInfo* cert_request_info) OVERRIDE;
 
-  // Overridden from NetworkChangeNotifier::ConnectionTypeObserver:
-  virtual void OnConnectionTypeChanged(
-      NetworkChangeNotifier::ConnectionType type) OVERRIDE;
-
   URLFetcherDelegate* delegate() const { return delegate_; }
   static void CancelAll();
   static int GetNumFetcherCores();

net/url_request/url_fetcher_impl_unittest.cc

@@ -75,24 +75,6 @@ class ThrottlingTestURLRequestContextGetter
   TestURLRequestContext* const context_;
 };
 
-class TestNetworkChangeNotifier : public NetworkChangeNotifier {
- public:
-  TestNetworkChangeNotifier() : connection_type_(CONNECTION_UNKNOWN) {}
-
-  // Implementation of NetworkChangeNotifier:
-  virtual ConnectionType GetCurrentConnectionType() const OVERRIDE {
-    return connection_type_;
-  }
-
-  void SetCurrentConnectionType(ConnectionType type) {
-    connection_type_ = type;
-    NotifyObserversOfConnectionTypeChange();
-  }
-
- private:
-  ConnectionType connection_type_;
-};
-
 }  // namespace
 
 class URLFetcherTest : public testing::Test,
@@ -175,8 +157,6 @@ class URLFetcherMockDnsTest : public URLFetcherTest {
   scoped_ptr<TestServer> test_server_;
   MockHostResolver resolver_;
   scoped_ptr<URLFetcher> completed_fetcher_;
-  NetworkChangeNotifier::DisableForTest disable_default_notifier_;
-  TestNetworkChangeNotifier network_change_notifier_;
 };
 
 void URLFetcherTest::CreateFetcher(const GURL& url) {
@@ -1012,97 +992,6 @@ TEST_F(URLFetcherMockDnsTest, RetryOnNetworkChangedAndSucceed) {
   EXPECT_EQ(200, completed_fetcher_->GetResponseCode());
 }
 
-TEST_F(URLFetcherMockDnsTest, RetryAfterComingBackOnline) {
-  EXPECT_EQ(0, GetNumFetcherCores());
-  EXPECT_FALSE(resolver_.has_pending_requests());
-
-  // This posts a task to start the fetcher.
-  CreateFetcher(test_url_);
-  fetcher_->SetAutomaticallyRetryOnNetworkChanges(1);
-  fetcher_->Start();
-  EXPECT_EQ(0, GetNumFetcherCores());
-  MessageLoop::current()->RunUntilIdle();
-
-  // The fetcher is now running, but is pending the host resolve.
-  EXPECT_EQ(1, GetNumFetcherCores());
-  EXPECT_TRUE(resolver_.has_pending_requests());
-  ASSERT_FALSE(completed_fetcher_);
-
-  // Make it fail by changing the connection type to offline.
-  EXPECT_EQ(NetworkChangeNotifier::CONNECTION_UNKNOWN,
-            NetworkChangeNotifier::GetConnectionType());
-  EXPECT_FALSE(NetworkChangeNotifier::IsOffline());
-  network_change_notifier_.SetCurrentConnectionType(
-      NetworkChangeNotifier::CONNECTION_NONE);
-  // This makes the connect job fail:
-  NetworkChangeNotifier::NotifyObserversOfIPAddressChangeForTests();
-  resolver_.ResolveAllPending();
-  MessageLoop::current()->RunUntilIdle();
-
-  // The fetcher is now waiting for the connection to become online again.
-  EXPECT_EQ(NetworkChangeNotifier::CONNECTION_NONE,
-            NetworkChangeNotifier::GetConnectionType());
-  EXPECT_TRUE(NetworkChangeNotifier::IsOffline());
-  EXPECT_FALSE(resolver_.has_pending_requests());
-  ASSERT_FALSE(completed_fetcher_);
-  // The core is still alive, but it dropped its request.
-  EXPECT_EQ(0, GetNumFetcherCores());
-
-  // It should retry once the connection is back.
-  network_change_notifier_.SetCurrentConnectionType(
-      NetworkChangeNotifier::CONNECTION_WIFI);
-  MessageLoop::current()->RunUntilIdle();
-  EXPECT_EQ(1, GetNumFetcherCores());
-  ASSERT_FALSE(completed_fetcher_);
-  EXPECT_TRUE(resolver_.has_pending_requests());
-
-  // Resolve the pending request; the fetcher should complete now.
-  resolver_.ResolveAllPending();
-  MessageLoop::current()->Run();
-
-  EXPECT_EQ(0, GetNumFetcherCores());
-  EXPECT_FALSE(resolver_.has_pending_requests());
-  ASSERT_TRUE(completed_fetcher_);
-  EXPECT_EQ(OK, completed_fetcher_->GetStatus().error());
-  EXPECT_EQ(200, completed_fetcher_->GetResponseCode());
-}
-
-TEST_F(URLFetcherMockDnsTest, StartOnlyWhenOnline) {
-  // Start offline.
-  network_change_notifier_.SetCurrentConnectionType(
-      NetworkChangeNotifier::CONNECTION_NONE);
-  EXPECT_TRUE(NetworkChangeNotifier::IsOffline());
-  EXPECT_EQ(0, GetNumFetcherCores());
-  EXPECT_FALSE(resolver_.has_pending_requests());
-
-  // Create a fetcher that retries on network changes. It will try to connect
-  // only once the network is back online.
-  CreateFetcher(test_url_);
-  fetcher_->SetAutomaticallyRetryOnNetworkChanges(1);
-  fetcher_->Start();
-  MessageLoop::current()->RunUntilIdle();
-  EXPECT_EQ(0, GetNumFetcherCores());
-  EXPECT_FALSE(resolver_.has_pending_requests());
-
-  // It should retry once the connection is back.
-  network_change_notifier_.SetCurrentConnectionType(
-      NetworkChangeNotifier::CONNECTION_WIFI);
-  MessageLoop::current()->RunUntilIdle();
-  EXPECT_EQ(1, GetNumFetcherCores());
-  ASSERT_FALSE(completed_fetcher_);
-  EXPECT_TRUE(resolver_.has_pending_requests());
-
-  // Resolve the pending request; the fetcher should complete now.
-  resolver_.ResolveAllPending();
-  MessageLoop::current()->Run();
-
-  EXPECT_EQ(0, GetNumFetcherCores());
-  EXPECT_FALSE(resolver_.has_pending_requests());
-  ASSERT_TRUE(completed_fetcher_);
-  EXPECT_EQ(OK, completed_fetcher_->GetStatus().error());
-  EXPECT_EQ(200, completed_fetcher_->GetResponseCode());
-}
-
 TEST_F(URLFetcherPostTest, Basic) {
   TestServer test_server(TestServer::TYPE_HTTP,
                          TestServer::kLocalhost,