Introduce ERR_NETWORK_CHANGED and allow URLFetcher to automatically retry on that error.

net/url_request/url_fetcher_core.cc

Index: net/url_request/url_fetcher_core.cc
diff --git a/net/url_request/url_fetcher_core.cc b/net/url_request/url_fetcher_core.cc
index 00aa56f5cdd5b4e8c6ffa7adaceaca765f261c8d..7b2fc37131165621a525e6329267d98e0a756667 100644
--- a/net/url_request/url_fetcher_core.cc
+++ b/net/url_request/url_fetcher_core.cc
@@ -287,6 +287,7 @@ URLFetcherCore::URLFetcherCore(URLFetcher* fetcher,
       stopped_on_redirect_(false),
       automatically_retry_on_5xx_(true),
       max_retries_(0),
+      max_retries_on_network_changes_(0),
       current_upload_bytes_(-1),
       current_response_bytes_(0),
       total_response_bytes_(-1) {
@@ -419,6 +420,10 @@ base::TimeDelta URLFetcherCore::GetBackoffDelay() const {
   return backoff_delay_;
 }
 
+void URLFetcherCore::SetAutomaticallyRetryOnNetworkChanges(int max_retries) {
+  max_retries_on_network_changes_ = max_retries;
+}
+
 void URLFetcherCore::SaveResponseToFileAtPath(
     const FilePath& file_path,
     scoped_refptr<base::TaskRunner> file_task_runner) {
@@ -542,6 +547,7 @@ void URLFetcherCore::OnReceivedRedirect(URLRequest* request,
 void URLFetcherCore::OnResponseStarted(URLRequest* request) {
   DCHECK_EQ(request, request_.get());
   DCHECK(network_task_runner_->BelongsToCurrentThread());
+
   if (request_->status().is_success()) {
     response_code_ = request_->GetResponseCode();
     response_headers_ = request_->response_headers();
@@ -606,6 +612,19 @@ void URLFetcherCore::OnReadCompleted(URLRequest* request,
   }
 }
 
+void URLFetcherCore::OnConnectionTypeChanged(
+    NetworkChangeNotifier::ConnectionType type) {
+  if (NetworkChangeNotifier::IsOffline()) {

[pauljensen, 2012/12/07 23:00:05]

I think it should be:
  if (type == NetworkChangeNotifier::CONNECTION_NONE)

[Joao da Silva, 2012/12/10 15:32:42]

Done.

+    // 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();
 }
@@ -857,6 +876,25 @@ void URLFetcherCore::RetryOrCompleteUrlFetch() {
   } else {
     backoff_delay = base::TimeDelta();
   }
+
+  // Retry if the request failed due to network changes.
+  if (status_.error() == ERR_NETWORK_CHANGED &&
+      max_retries_on_network_changes_ > 0) {
+    max_retries_on_network_changes_--;

[pauljensen, 2012/12/07 23:00:05]

I notice the code above uses a counter and a maximum value, rather than your
method of decreasing the maximum.  I don't know if one way is better than the
other.  If there were some way to restart a fetch, then having a separate
maximum might be nice because it could live across reuses.  Your call.

[Joao da Silva, 2012/12/10 15:32:42]

Done. Having a separate count also allows calling RemoveConnectionTypeObserver()
in ReleaseRequest() if there was a previous retry.

+
+    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));
+    }
+
+    return;
+  }
+
   request_context_getter_ = NULL;
   first_party_for_cookies_ = GURL();
   url_request_data_key_ = NULL;
@@ -871,6 +909,7 @@ void URLFetcherCore::RetryOrCompleteUrlFetch() {
 }
 
 void URLFetcherCore::ReleaseRequest() {
+  NetworkChangeNotifier::RemoveConnectionTypeObserver(this);

[pauljensen, 2012/12/07 23:00:05]

Would be nice if we didn't always have to do this (i.e. just when we did
register ourselves).  The ObserverList is rather slow (its thread safe so it
locks and it's really a list so it's linear time).

[Joao da Silva, 2012/12/10 15:32:42]

Checking that |num_retries_on_network_changes > 0| is not a perfect signal, but
removes the need to do this from the majority of fetchers. WDYT?

[pauljensen, 2012/12/10 16:09:14]

That seems fine.

   upload_progress_checker_timer_.reset();
   request_.reset();
   g_registry.Get().RemoveURLFetcherCore(this);