Retry to download the AppPack when the network changes.

This fixes an issue where the download starts too early and always fails.

BUG=130602
TEST=Bug doesn't trigger anymore


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

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

2 files changed, 70 insertions, 13 deletions

diff --git a/chrome/browser/policy/app_pack_updater.cc b/chrome/browser/policy/app_pack_updater.cc
index ac66728..6d32cb1 100644
--- a/chrome/browser/policy/app_pack_updater.cc
+++ b/chrome/browser/policy/app_pack_updater.cc
@@ -100,6 +100,7 @@ AppPackUpdater::AppPackUpdater(net::URLRequestContextGetter* request_context,
 AppPackUpdater::~AppPackUpdater() {
   chromeos::CrosSettings::Get()->RemoveSettingsObserver(
       chromeos::kAppPack, this);
+  net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
 }
 
 ExternalExtensionLoader* AppPackUpdater::CreateExternalExtensionLoader() {
@@ -135,6 +136,7 @@ void AppPackUpdater::Init() {
       this,
       chrome::NOTIFICATION_EXTENSION_INSTALL_ERROR,
       content::NotificationService::AllBrowserContextsAndSources());
+  net::NetworkChangeNotifier::AddIPAddressObserver(this);
   LoadPolicy();
 }
 
@@ -166,6 +168,28 @@ 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(
@@ -189,7 +213,8 @@ void AppPackUpdater::LoadPolicy() {
       std::string update_url;
       if (dict->GetString(kExtensionId, &id) &&
           dict->GetString(kUpdateUrl, &update_url)) {
-        app_pack_extensions_[id] = update_url;
+        app_pack_extensions_[id].update_url = update_url;
+        app_pack_extensions_[id].update_checked = false;
       } else {
         LOG(WARNING) << "Failed to read required fields for an AppPack entry, "
                      << "ignoring.";
@@ -233,6 +258,7 @@ void AppPackUpdater::BlockingCheckCache(
                                      base::Owned(entries)));
 }
 
+// static
 void AppPackUpdater::BlockingCheckCacheInternal(
     const std::set<std::string>* valid_ids,
     CacheEntryMap* entries) {
@@ -334,12 +360,10 @@ void AppPackUpdater::OnCacheUpdated(CacheEntryMap* cache_entries) {
   cached_extensions_.swap(*cache_entries);
 
   CacheEntryMap::iterator it = cached_extensions_.find(screen_saver_id_);
-  if (it != cached_extensions_.end()) {
+  if (it != cached_extensions_.end())
     SetScreenSaverPath(FilePath(it->second.path));
-    cached_extensions_.erase(it);
-  } else {
+  else
     SetScreenSaverPath(FilePath());
-  }
 
   VLOG(1) << "Updated AppPack cache, there are " << cached_extensions_.size()
           << " extensions cached and "
@@ -361,6 +385,10 @@ void AppPackUpdater::UpdateExtensionLoader() {
   scoped_ptr<base::DictionaryValue> prefs(new base::DictionaryValue());
   for (CacheEntryMap::iterator it = cached_extensions_.begin();
        it != cached_extensions_.end(); ++it) {
+    // The screensaver isn't installed into the Profile.
+    if (it->first == screen_saver_id_)
+      continue;
+
     base::DictionaryValue* dict = new base::DictionaryValue();
     dict->SetString(ExternalExtensionProviderImpl::kExternalCrx,
                     it->second.path);
@@ -384,8 +412,9 @@ void AppPackUpdater::DownloadMissingExtensions() {
   }
   for (PolicyEntryMap::iterator it = app_pack_extensions_.begin();
        it != app_pack_extensions_.end(); ++it) {
-    downloader_->AddPendingExtension(it->first, GURL(it->second));
+    downloader_->AddPendingExtension(it->first, GURL(it->second.update_url));
   }
+  VLOG(1) << "Downloading AppPack update manifest now";
   downloader_->StartAllPending();
 }
 
@@ -393,7 +422,11 @@ void AppPackUpdater::OnExtensionDownloadFailed(
     const std::string& id,
     extensions::ExtensionDownloaderDelegate::Error error,
     const extensions::ExtensionDownloaderDelegate::PingResult& ping_result) {
-  if (error != NO_UPDATE_AVAILABLE) {
+  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;
   }
@@ -405,6 +438,10 @@ void AppPackUpdater::OnExtensionDownloadFinished(
     const GURL& download_url,
     const std::string& version,
     const extensions::ExtensionDownloaderDelegate::PingResult& ping_result) {
+  // 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,
@@ -493,15 +530,15 @@ void AppPackUpdater::OnCacheEntryInstalled(const std::string& id,
                                            const std::string& path,
                                            const std::string& version) {
   VLOG(1) << "AppPack installed a new extension in the cache: " << path;
+  // Add to the list of cached extensions.
+  CacheEntry& entry = cached_extensions_[id];
+  entry.path = path;
+  entry.cached_version = version;
 
   if (id == screen_saver_id_) {
     VLOG(1) << "AppPack got the screen saver extension at " << path;
     SetScreenSaverPath(FilePath(path));
   } else {
-    // Add to the list of cached extensions.
-    CacheEntry& entry = cached_extensions_[id];
-    entry.path = path;
-    entry.cached_version = version;
     UpdateExtensionLoader();
   }
 }
@@ -546,4 +583,10 @@ 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 b34a2f0..c8f5733 100644
--- a/chrome/browser/policy/app_pack_updater.h
+++ b/chrome/browser/policy/app_pack_updater.h
@@ -20,6 +20,7 @@
 #include "chrome/browser/policy/cloud_policy_subsystem.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "net/base/network_change_notifier.h"
 
 class CrxInstaller;
 class ExternalExtensionLoader;
@@ -48,6 +49,7 @@ class BrowserPolicyConnector;
 // at login time.
 class AppPackUpdater : public CloudPolicySubsystem::Observer,
                        public content::NotificationObserver,
+                       public net::NetworkChangeNotifier::IPAddressObserver,
                        public extensions::ExtensionDownloaderDelegate {
  public:
   // Callback to listen for updates to the screensaver extension's path.
@@ -74,13 +76,18 @@ class AppPackUpdater : public CloudPolicySubsystem::Observer,
   void SetScreenSaverUpdateCallback(const ScreenSaverUpdateCallback& callback);
 
  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.
-  typedef std::map<std::string, std::string> PolicyEntryMap;
+  // Maps an extension ID to its update URL and update information.
+  typedef std::map<std::string, AppPackEntry> PolicyEntryMap;
 
   // Maps an extension ID to a CacheEntry.
   typedef std::map<std::string, CacheEntry> CacheEntryMap;
@@ -97,6 +104,9 @@ class AppPackUpdater : public CloudPolicySubsystem::Observer,
                        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();
 
@@ -177,6 +187,10 @@ class AppPackUpdater : public CloudPolicySubsystem::Observer,
   // 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 updates to the |device_cloud_policy_subsystem_|, to detect