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

net/url_request/url_fetcher_core.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/url_request/url_fetcher_core.h"
 
 #include "base/bind.h"
 #include "base/file_util_proxy.h"
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 269 matching lines @@
       url_request_data_key_(NULL),
       was_fetched_via_proxy_(false),
       is_chunked_upload_(false),
       num_retries_(0),
       was_cancelled_(false),
       response_destination_(STRING),
       stop_on_redirect_(false),
       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) {
   CHECK(original_url_.is_valid());
 }
 
 void URLFetcherCore::Start() {
   DCHECK(delegate_task_runner_);
   DCHECK(request_context_getter_) << "We need an URLRequestContext!";
   if (network_task_runner_) {
@@ skipping 112 matching lines @@
 }
 
 int URLFetcherCore::GetMaxRetries() const {
   return max_retries_;
 }
 
 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) {
   DCHECK(delegate_task_runner_->BelongsToCurrentThread());
   file_task_runner_ = file_task_runner;
   response_destination_ = URLFetcherCore::PERMANENT_FILE;
   response_destination_file_path_ = file_path;
 }
 
 void URLFetcherCore::SaveResponseToTemporaryFile(
@@ skipping 103 matching lines @@
     response_code_ = request_->GetResponseCode();
     was_fetched_via_proxy_ = request_->was_fetched_via_proxy();
     request->Cancel();
     OnReadCompleted(request, 0);
   }
 }
 
 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();
     socket_address_ = request_->GetSocketAddress();
     was_fetched_via_proxy_ = request_->was_fetched_via_proxy();
     total_response_bytes_ = request_->GetExpectedContentSize();
   }
 
   ReadResponse();
 }
@@ skipping 44 matching lines @@
       // If the file is open, close it.  After closing the file,
       // RetryOrCompleteUrlFetch() will be called.
       file_writer_->CloseFileAndCompleteRequest();
     } else {
       // Otherwise, complete or retry the URL request directly.
       RetryOrCompleteUrlFetch();
     }
   }
 }
 
+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();
 }
 
 int URLFetcherCore::GetNumFetcherCores() {
   return g_registry.Get().size();
 }
 
 void URLFetcherCore::SetEnableInterceptionForTests(bool enabled) {
   g_interception_enabled = enabled;
@@ skipping 231 matching lines @@
     if (backoff_delay < base::TimeDelta())
       backoff_delay = base::TimeDelta();
 
     if (automatically_retry_on_5xx_ && num_retries_ <= max_retries_) {
       StartOnIOThread();
       return;
     }
   } 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;
   url_request_create_data_callback_.Reset();
   bool posted = delegate_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&URLFetcherCore::OnCompletedURLRequest, this, backoff_delay));
 
   // If the delegate message loop does not exist any more, then the delegate
   // should be gone too.
   DCHECK(posted || !delegate_);
 }
 
 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);
 }
 
 base::TimeTicks URLFetcherCore::GetBackoffReleaseTime() {
   DCHECK(network_task_runner_->BelongsToCurrentThread());
 
   if (original_url_throttler_entry_) {
     base::TimeTicks original_url_backoff =
@@ skipping 123 matching lines @@
 }
 
 void URLFetcherCore::InformDelegateDownloadDataInDelegateThread(
     scoped_ptr<std::string> download_data) {
   DCHECK(delegate_task_runner_->BelongsToCurrentThread());
   if (delegate_)
     delegate_->OnURLFetchDownloadData(fetcher_, download_data.Pass());
 }
 
 }  // namespace net