Automatically retry the following URLFetchers when the network changes:

extensions_downloader.cc (extensions, retail mode AppPack)
gaia_oauth_fetcher.cc
gaia_oauth_client.cc
device_management_service.cc (user/device policy fetches, auto-enrollment)

All of these have been noticed to fail when network change notifications are sent, in particular on ChromeOS.

TBR=jochen
BUG=145021,163710,130602,chromium-os:16114

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

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

6 files changed, 29 insertions, 55 deletions

diff --git a/chrome/browser/extensions/updater/extension_downloader.cc b/chrome/browser/extensions/updater/extension_downloader.cc
index f134e87..95f6270 100644
--- a/chrome/browser/extensions/updater/extension_downloader.cc
+++ b/chrome/browser/extensions/updater/extension_downloader.cc
@@ -429,6 +429,11 @@ void ExtensionDownloader::CreateManifestFetcher() {
   manifest_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                                   net::LOAD_DO_NOT_SAVE_COOKIES |
                                   net::LOAD_DISABLE_CACHE);
+  // Update checks can be interrupted if a network change is detected; this is
+  // common for the retail mode AppPack on ChromeOS. Retrying once should be
+  // enough to recover in those cases; let the fetcher retry up to 3 times
+  // just in case. http://crosbug.com/130602
+  manifest_fetcher_->SetAutomaticallyRetryOnNetworkChanges(3);
   manifest_fetcher_->Start();
 }
 
@@ -674,6 +679,7 @@ void ExtensionDownloader::CreateExtensionFetcher() {
   extension_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
                                    net::LOAD_DO_NOT_SAVE_COOKIES |
                                    net::LOAD_DISABLE_CACHE);
+  extension_fetcher_->SetAutomaticallyRetryOnNetworkChanges(3);
   // Download CRX files to a temp file. The blacklist is small and will be
   // processed in memory, so it is fetched into a string.
   if (extensions_queue_.active_request()->id != kBlacklistAppID) {
diff --git a/chrome/browser/net/gaia/gaia_oauth_fetcher.cc b/chrome/browser/net/gaia/gaia_oauth_fetcher.cc
index a42aa89..521e0d8 100644
--- a/chrome/browser/net/gaia/gaia_oauth_fetcher.cc
+++ b/chrome/browser/net/gaia/gaia_oauth_fetcher.cc
@@ -63,6 +63,11 @@ net::URLFetcher* GaiaOAuthFetcher::CreateGaiaFetcher(
       empty_body ? net::URLFetcher::GET : net::URLFetcher::POST,
       delegate);
   result->SetRequestContext(getter);
+  // Fetchers are sometimes cancelled because a network change was detected,
+  // especially at startup and after sign-in on ChromeOS. Retrying once should
+  // be enough in those cases; let the fetcher retry up to 3 times just in case.
+  // http://crbug.com/163710
+  result->SetAutomaticallyRetryOnNetworkChanges(3);
 
   // The Gaia/OAuth token exchange requests do not require any cookie-based
   // identification as part of requests.  We suppress sending any cookies to
diff --git a/chrome/browser/policy/app_pack_updater.cc b/chrome/browser/policy/app_pack_updater.cc
index 7355e28..a6e16bc 100644
--- a/chrome/browser/policy/app_pack_updater.cc
+++ b/chrome/browser/policy/app_pack_updater.cc
@@ -102,7 +102,6 @@ AppPackUpdater::AppPackUpdater(net::URLRequestContextGetter* request_context,
 AppPackUpdater::~AppPackUpdater() {
   chromeos::CrosSettings::Get()->RemoveSettingsObserver(
       chromeos::kAppPack, this);
-  net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
 }
 
 extensions::ExternalLoader* AppPackUpdater::CreateExternalLoader() {
@@ -141,7 +140,6 @@ void AppPackUpdater::Init() {
       this,
       chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR,
       content::NotificationService::AllBrowserContextsAndSources());
-  net::NetworkChangeNotifier::AddIPAddressObserver(this);
   LoadPolicy();
 }
 
@@ -172,28 +170,6 @@ void AppPackUpdater::Observe(int type,
   }
 }
 
-void AppPackUpdater::OnIPAddressChanged() {
-  // Check if the AppPack has been fully downloaded whenever the network
-  // changes. This allows the AppPack to recover in case the network wasn't
-  // ready early during startup.
-  // To avoid performing too many update checks in case the network conditions
-  // change too often, an update is only triggered now if there are extensions
-  // configured via policy that haven't been checked for updates yet.
-  for (PolicyEntryMap::iterator it = app_pack_extensions_.begin();
-       it != app_pack_extensions_.end(); ++it) {
-    if (!it->second.update_checked) {
-      // |id| is configured via policy, but hasn't been updated before.
-      // Drop any pending requests and start a full check now.
-      VLOG(1) << "Extension " << it->first << " hasn't been checked yet, "
-              << "new update triggered now by network change notification.";
-      downloader_.reset();
-      weak_ptr_factory_.InvalidateWeakPtrs();
-      LoadPolicy();
-      break;
-    }
-  }
-}
-
 void AppPackUpdater::LoadPolicy() {
   chromeos::CrosSettings* settings = chromeos::CrosSettings::Get();
   if (chromeos::CrosSettingsProvider::TRUSTED != settings->PrepareTrustedValues(
@@ -217,8 +193,7 @@ void AppPackUpdater::LoadPolicy() {
       std::string update_url;
       if (dict->GetString(kExtensionId, &id) &&
           dict->GetString(kUpdateUrl, &update_url)) {
-        app_pack_extensions_[id].update_url = update_url;
-        app_pack_extensions_[id].update_checked = false;
+        app_pack_extensions_[id] = update_url;
       } else {
         LOG(WARNING) << "Failed to read required fields for an AppPack entry, "
                      << "ignoring.";
@@ -405,7 +380,7 @@ void AppPackUpdater::UpdateExtensionLoader() {
     PolicyEntryMap::iterator policy_entry = app_pack_extensions_.find(id);
     if (policy_entry != app_pack_extensions_.end() &&
         extension_urls::IsWebstoreUpdateUrl(
-            GURL(policy_entry->second.update_url))) {
+            GURL(policy_entry->second))) {
       dict->SetBoolean(extensions::ExternalProviderImpl::kIsFromWebstore, true);
     }
 
@@ -427,7 +402,7 @@ void AppPackUpdater::DownloadMissingExtensions() {
   }
   for (PolicyEntryMap::iterator it = app_pack_extensions_.begin();
        it != app_pack_extensions_.end(); ++it) {
-    downloader_->AddPendingExtension(it->first, GURL(it->second.update_url), 0);
+    downloader_->AddPendingExtension(it->first, GURL(it->second), 0);
   }
   VLOG(1) << "Downloading AppPack update manifest now";
   downloader_->StartAllPending();
@@ -441,7 +416,6 @@ void AppPackUpdater::OnExtensionDownloadFailed(
   if (error == NO_UPDATE_AVAILABLE) {
     if (!ContainsKey(cached_extensions_, id))
       LOG(ERROR) << "AppPack extension " << id << " not found on update server";
-    SetUpdateChecked(id);
   } else {
     LOG(ERROR) << "AppPack failed to download extension " << id
                << ", error " << error;
@@ -455,10 +429,6 @@ void AppPackUpdater::OnExtensionDownloadFinished(
     const std::string& version,
     const extensions::ExtensionDownloaderDelegate::PingResult& ping_result,
     const std::set<int>& request_ids) {
-  // Just downloaded the latest version, no need to do further update checks
-  // for this extension.
-  SetUpdateChecked(id);
-
   // The explicit copy ctors are to make sure that Bind() binds a copy and not
   // a reference to the arguments.
   PostBlockingTask(FROM_HERE,
@@ -599,10 +569,4 @@ void AppPackUpdater::SetScreenSaverPath(const FilePath& path) {
   }
 }
 
-void AppPackUpdater::SetUpdateChecked(const std::string& id) {
-  PolicyEntryMap::iterator entry = app_pack_extensions_.find(id);
-  if (entry != app_pack_extensions_.end())
-    entry->second.update_checked = true;
-}
-
 }  // namespace policy
diff --git a/chrome/browser/policy/app_pack_updater.h b/chrome/browser/policy/app_pack_updater.h
index 057b481..142ec9a 100644
--- a/chrome/browser/policy/app_pack_updater.h
+++ b/chrome/browser/policy/app_pack_updater.h
@@ -18,7 +18,6 @@
 #include "chrome/browser/extensions/updater/extension_downloader_delegate.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
-#include "net/base/network_change_notifier.h"
 
 class GURL;
 
@@ -45,7 +44,6 @@ class EnterpriseInstallAttributes;
 // device policy to be locally cached and installed into the Demo user account
 // at login time.
 class AppPackUpdater : public content::NotificationObserver,
-                       public net::NetworkChangeNotifier::IPAddressObserver,
                        public extensions::ExtensionDownloaderDelegate {
  public:
   // Callback to listen for updates to the screensaver extension's path.
@@ -77,18 +75,13 @@ class AppPackUpdater : public content::NotificationObserver,
   void OnDamagedFileDetected(const FilePath& path);
 
  private:
-  struct AppPackEntry {
-    std::string update_url;
-    bool update_checked;
-  };
-
   struct CacheEntry {
     std::string path;
     std::string cached_version;
   };
 
-  // Maps an extension ID to its update URL and update information.
-  typedef std::map<std::string, AppPackEntry> PolicyEntryMap;
+  // Maps an extension ID to its update URL.
+  typedef std::map<std::string, std::string> PolicyEntryMap;
 
   // Maps an extension ID to a CacheEntry.
   typedef std::map<std::string, CacheEntry> CacheEntryMap;
@@ -100,9 +93,6 @@ class AppPackUpdater : public content::NotificationObserver,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
-  // net::NetworkChangeNotifier::IPAddressObserver:
-  virtual void OnIPAddressChanged() OVERRIDE;
-
   // Loads the current policy and schedules a cache update.
   void LoadPolicy();
 
@@ -186,10 +176,6 @@ class AppPackUpdater : public content::NotificationObserver,
   // appropriate.
   void SetScreenSaverPath(const FilePath& path);
 
-  // Marks extension |id| in |app_pack_extensions_| as having already been
-  // checked for updates, if it exists.
-  void SetUpdateChecked(const std::string& id);
-
   base::WeakPtrFactory<AppPackUpdater> weak_ptr_factory_;
 
   // Observes failures to install CRX files.
diff --git a/chrome/browser/policy/device_management_service.cc b/chrome/browser/policy/device_management_service.cc
index 18245b3..b218e31 100644
--- a/chrome/browser/policy/device_management_service.cc
+++ b/chrome/browser/policy/device_management_service.cc
@@ -512,6 +512,12 @@ void DeviceManagementService::StartJob(DeviceManagementRequestJobImpl* job,
                         net::LOAD_DISABLE_CACHE |
                         (bypass_proxy ? net::LOAD_BYPASS_PROXY : 0));
   fetcher->SetRequestContext(request_context_getter_.get());
+  // Early device policy fetches on ChromeOS and Auto-Enrollment checks are
+  // often interrupted during ChromeOS startup when network change notifications
+  // are sent. Allowing the fetcher to retry once after that is enough to
+  // recover; allow it to retry up to 3 times just in case.
+  // http://crosbug.com/16114
+  fetcher->SetAutomaticallyRetryOnNetworkChanges(3);
   job->ConfigureRequest(fetcher);
   pending_jobs_[fetcher] = job;
   fetcher->Start();
diff --git a/google_apis/gaia/gaia_oauth_client.cc b/google_apis/gaia/gaia_oauth_client.cc
index 1adefb8..a9bae4f 100644
--- a/google_apis/gaia/gaia_oauth_client.cc
+++ b/google_apis/gaia/gaia_oauth_client.cc
@@ -129,6 +129,11 @@ void GaiaOAuthClient::Core::GetUserInfo(const std::string& oauth_access_token,
   request_->AddExtraRequestHeader(
       "Authorization: OAuth " + oauth_access_token);
   request_->SetMaxRetriesOn5xx(max_retries);
+  // Fetchers are sometimes cancelled because a network change was detected,
+  // especially at startup and after sign-in on ChromeOS. Retrying once should
+  // be enough in those cases; let the fetcher retry up to 3 times just in case.
+  // http://crbug.com/163710
+  request_->SetAutomaticallyRetryOnNetworkChanges(3);
   request_->Start();
 }
 
@@ -144,6 +149,8 @@ void GaiaOAuthClient::Core::MakeGaiaRequest(
   request_->SetRequestContext(request_context_getter_);
   request_->SetUploadData("application/x-www-form-urlencoded", post_body);
   request_->SetMaxRetriesOn5xx(max_retries);
+  // See comment on SetAutomaticallyRetryOnNetworkChanges() above.
+  request_->SetAutomaticallyRetryOnNetworkChanges(3);
   request_->Start();
 }