contacts: Rate-limit GData photo requests and handle 404s.

The API returns 503 errors if we try to download photos too
quickly.  This change adds cheesy rate-limiting so we don't
request more than 10 photos per second (which appears to be
the undocumented limit) and also makes us retry when we get
a 503.

I'm also making GDataContactsService report success even
when we got 404 errors for some photos, as I see this happen
with my personal account. :-/

BUG=128805
TEST=none


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

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

chrome/browser/chromeos/gdata/gdata_contacts_service.cc

@@ -13,6 +13,8 @@
 #include "base/logging.h"
 #include "base/memory/weak_ptr.h"
 #include "base/stl_util.h"
+#include "base/time.h"
+#include "base/timer.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/contacts/contact.pb.h"
 #include "chrome/browser/chromeos/gdata/gdata_operation_registry.h"
@@ -28,8 +30,9 @@ namespace gdata {
 
 namespace {
 
-// Maximum number of profile photos that we'll download at once.
-const int kMaxSimultaneousPhotoDownloads = 10;
+// Maximum number of profile photos that we'll download per second.
+// At values above 10, Google starts returning 503 errors.
+const int kMaxPhotoDownloadsPerSecond = 10;
 
 // Field in the top-level object containing the contacts feed.
 const char kFeedField[] = "feed";
@@ -319,15 +322,13 @@ class GDataContactsService::DownloadContactsRequest
                           GDataOperationRunner* runner,
                           SuccessCallback success_callback,
                           FailureCallback failure_callback,
-                          const base::Time& min_update_time,
-                          int max_simultaneous_photo_downloads)
+                          const base::Time& min_update_time)
       : service_(service),
         runner_(runner),
         success_callback_(success_callback),
         failure_callback_(failure_callback),
         min_update_time_(min_update_time),
         contacts_(new ScopedVector<contacts::Contact>),
-        max_simultaneous_photo_downloads_(max_simultaneous_photo_downloads),
         num_in_progress_photo_downloads_(0),
         photo_download_failed_(false) {
     DCHECK(service_);
@@ -374,6 +375,10 @@ class GDataContactsService::DownloadContactsRequest
       return;
     }
 
+    StartPhotoDownloads();
+    photo_download_timer_.Start(
+        FROM_HERE, service_->photo_download_timer_interval_,
+        this, &DownloadContactsRequest::StartPhotoDownloads);
     CheckCompletion();
   }
 
@@ -467,6 +472,7 @@ class GDataContactsService::DownloadContactsRequest
     if (contacts_needing_photo_downloads_.empty() &&
         num_in_progress_photo_downloads_ == 0) {
       VLOG(1) << "Done downloading photos; invoking callback";
+      photo_download_timer_.Stop();
       if (photo_download_failed_)
         failure_callback_.Run();
       else
@@ -474,10 +480,14 @@ class GDataContactsService::DownloadContactsRequest
       service_->OnRequestComplete(this);
       return;
     }
+  }
 
+  // Starts photo downloads for contacts in |contacts_needing_photo_downloads_|.
+  // Should be invoked only once per second.
+  void StartPhotoDownloads() {
     while (!contacts_needing_photo_downloads_.empty() &&
            (num_in_progress_photo_downloads_ <
-            max_simultaneous_photo_downloads_)) {
+            service_->max_photo_downloads_per_second_)) {
       contacts::Contact* contact = contacts_needing_photo_downloads_.back();
       contacts_needing_photo_downloads_.pop_back();
       DCHECK(contact_photo_urls_.count(contact));
@@ -505,10 +515,24 @@ class GDataContactsService::DownloadContactsRequest
             << " (error=" << error << " size=" << download_data->size() << ")";
     num_in_progress_photo_downloads_--;
 
+    if (error == HTTP_INTERNAL_SERVER_ERROR ||
+        error == HTTP_SERVICE_UNAVAILABLE) {
+      LOG(WARNING) << "Got error " << error << " while downloading photo "
+                   << "for " << contact->provider_id() << "; retrying";
+      contacts_needing_photo_downloads_.push_back(contact);
+      return;
+    }
+
+    if (error == HTTP_NOT_FOUND) {
+      LOG(WARNING) << "Got error " << error << " while downloading photo "
+                   << "for " << contact->provider_id() << "; skipping";
+      CheckCompletion();
+      return;
+    }
+
     if (error != HTTP_SUCCESS) {
       LOG(WARNING) << "Got error " << error << " while downloading photo "
-                   << "for " << contact->provider_id();
-      // TODO(derat): Retry several times for temporary failures?
+                   << "for " << contact->provider_id() << "; giving up";
       photo_download_failed_ = true;
       // Make sure we don't start any more downloads.
       contacts_needing_photo_downloads_.clear();
@@ -537,13 +561,13 @@ class GDataContactsService::DownloadContactsRequest
   // Contacts without photos do not appear in this map.
   ContactPhotoUrls contact_photo_urls_;
 
+  // Invokes StartPhotoDownloads() once per second.
+  base::RepeatingTimer<DownloadContactsRequest> photo_download_timer_;
+
   // Contacts that have photos that we still need to start downloading.
   // When we start a download, the contact is removed from this list.
   std::vector<contacts::Contact*> contacts_needing_photo_downloads_;
 
-  // Maximum number of photos we'll try to download at once.
-  int max_simultaneous_photo_downloads_;
-
   // Number of in-progress photo downloads.
   int num_in_progress_photo_downloads_;
 
@@ -555,7 +579,8 @@ class GDataContactsService::DownloadContactsRequest
 
 GDataContactsService::GDataContactsService(Profile* profile)
     : runner_(new GDataOperationRunner(profile)),
-      max_simultaneous_photo_downloads_(kMaxSimultaneousPhotoDownloads) {
+      max_photo_downloads_per_second_(kMaxPhotoDownloadsPerSecond),
+      photo_download_timer_interval_(base::TimeDelta::FromSeconds(1)) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
@@ -584,8 +609,7 @@ void GDataContactsService::DownloadContacts(SuccessCallback success_callback,
                                   runner_.get(),
                                   success_callback,
                                   failure_callback,
-                                  min_update_time,
-                                  max_simultaneous_photo_downloads_);
+                                  min_update_time);
   VLOG(1) << "Starting contacts download with request " << request;
   requests_.insert(request);
   request->Run();

chrome/browser/chromeos/gdata/gdata_contacts_service.h

@@ -68,8 +68,11 @@ class GDataContactsService : public GDataContactsServiceInterface {
 
   GDataAuthService* auth_service_for_testing();
 
-  void set_max_simultaneous_photo_downloads_for_testing(int max_downloads) {
-    max_simultaneous_photo_downloads_ = max_downloads;
+  void set_max_photo_downloads_per_second_for_testing(int max_downloads) {
+    max_photo_downloads_per_second_ = max_downloads;
+  }
+  void set_photo_download_timer_interval_for_testing(base::TimeDelta interval) {
+    photo_download_timer_interval_ = interval;
   }
   void set_feed_url_for_testing(const GURL& url) {
     feed_url_for_testing_ = url;
@@ -98,15 +101,20 @@ class GDataContactsService : public GDataContactsServiceInterface {
   // In-progress download requests.  Pointers are owned by this class.
   std::set<DownloadContactsRequest*> requests_;
 
+  // Maximum number of photos we'll try to download per second (per
+  // DownloadContacts() request).
+  int max_photo_downloads_per_second_;
+
+  // Amount of time that we'll wait between waves of photo download requests.
+  // This is usually one second (see |max_photo_downloads_per_second_|) but can
+  // be set to a lower value for tests to make them complete more quickly.
+  base::TimeDelta photo_download_timer_interval_;
+
   // If non-empty, URL that will be used to fetch the feed.  URLs contained
   // within the feed will also be modified to use the host and port from this
   // member.
   GURL feed_url_for_testing_;
 
-  // Maximum number of photos we'll try to download at once (per
-  // DownloadContacts() request).
-  int max_simultaneous_photo_downloads_;
-
   // Callback that's invoked to rewrite photo URLs for tests.
   // This is needed for tests that serve static feed data from a host/port
   // that's only known at runtime.

chrome/browser/chromeos/gdata/gdata_contacts_service_browsertest.cc

@@ -83,6 +83,8 @@ class GDataContactsServiceTest : public InProcessBrowserTest {
     service_->set_rewrite_photo_url_callback_for_testing(
         base::Bind(&GDataContactsServiceTest::RewritePhotoUrl,
                    base::Unretained(this)));
+    service_->set_photo_download_timer_interval_for_testing(
+        base::TimeDelta::FromMilliseconds(10));
   }
 
   virtual void CleanUpOnMainThread() {
@@ -159,7 +161,11 @@ IN_PROC_BROWSER_TEST_F(GDataContactsServiceTest, BrokenFeeds) {
   EXPECT_FALSE(Download("no_feed.json", base::Time(), &contacts));
   EXPECT_FALSE(Download("no_category.json", base::Time(), &contacts));
   EXPECT_FALSE(Download("wrong_category.json", base::Time(), &contacts));
-  EXPECT_FALSE(Download("feed_photo_404.json", base::Time(), &contacts));
+
+  // Missing photos should be allowed, though (as this can occur in production).
+  EXPECT_TRUE(Download("feed_photo_404.json", base::Time(), &contacts));
+  ASSERT_EQ(static_cast<size_t>(1), contacts->size());
+  EXPECT_FALSE((*contacts)[0]->has_raw_untrusted_photo());
 }
 
 // Check that we're able to download an empty feed and a normal-looking feed
@@ -238,7 +244,7 @@ IN_PROC_BROWSER_TEST_F(GDataContactsServiceTest, Download) {
 IN_PROC_BROWSER_TEST_F(GDataContactsServiceTest, ParallelPhotoDownload) {
   // The feed used for this test contains 8 contacts.
   const int kNumContacts = 8;
-  service()->set_max_simultaneous_photo_downloads_for_testing(2);
+  service()->set_max_photo_downloads_per_second_for_testing(6);
   scoped_ptr<ScopedVector<contacts::Contact> > contacts;
   EXPECT_TRUE(Download("feed_multiple_photos.json", base::Time(), &contacts));
   ASSERT_EQ(static_cast<size_t>(kNumContacts), contacts->size());