Do not fetch profile image if the profile picture URL is invalid.

ProfileDownloader is responsible to download the picture of an account.
To do the fetch, it uses the account picture URL that is fetched by the
AccountFetcherService (from https://www.googleapis.com/oauth2/v1/userinfo )
and stored in AccountTrackerService as an AccountInfo. If the user info
dictionary fetched from the userinfo endpoint does not contain any picture
URL, then it sets the picture URL to "NO_PICTURE_URL" in the AccountInfo.

ProfileDownloader does not check whether the profile picture URL
is invalid before creating the URLFetcher to fetch this image. So if
the picture URL of the account is "NO_PICTURE_URL", then it will attempt
to fetch it via an invalid URL which leads to the crash described in the bug.

This CL gracefully handles the case when the picture URL is invalid.

Bug: 854907
Change-Id: Iea79b120cd8e4e830fa1a4cc074793418914b155
Reviewed-on: https://chromium-review.googlesource.com/1111846
Commit-Queue: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569628}

chrome/browser/profiles/profile_downloader.cc

@@ -177,6 +177,14 @@ void ProfileDownloader::FetchImageData() {
     return;
   }
 
+  if (account_info_.picture_url == AccountTrackerService::kNoPictureURLFound) {
+    VLOG(1) << "No picture URL for account " << account_info_.email
+            << ". Using the default profile picture.";
+    picture_status_ = PICTURE_DEFAULT;
+    delegate_->OnProfileDownloadSuccess(this);
+    return;
+  }
+
   std::string image_url_with_size = GetProfilePictureURL();
   if (!image_url_with_size.empty() &&
       image_url_with_size == delegate_->GetCachedPictureURL()) {
@@ -186,6 +194,17 @@ void ProfileDownloader::FetchImageData() {
     return;
   }
 
+  GURL image_url_to_fetch(image_url_with_size);
+  if (!image_url_to_fetch.is_valid()) {
+    VLOG(1) << "Profile picture URL with size |" << image_url_to_fetch << "| "
+            << "is not valid (the account picture URL is "
+            << "|" << account_info_.picture_url << "|)";
+    delegate_->OnProfileDownloadFailure(
+        this,
+        ProfileDownloaderDelegate::FailureReason::INVALID_PROFILE_PICTURE_URL);
+    return;
+  }
+
   // Create traffic annotation tag.
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("signed_in_profile_avatar", R"(
@@ -209,10 +228,10 @@ void ProfileDownloader::FetchImageData() {
             "profile image."
         })");
 
-  VLOG(1) << "Loading profile image from " << image_url_with_size;
+  VLOG(1) << "Loading profile image from " << image_url_to_fetch;
 
   auto resource_request = std::make_unique<network::ResourceRequest>();
-  resource_request->url = GURL(image_url_with_size);
+  resource_request->url = image_url_to_fetch;
   resource_request->load_flags =
       net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES;
   if (!auth_token_.empty()) {

chrome/browser/profiles/profile_downloader.h

@@ -79,7 +79,10 @@ class ProfileDownloader : public ImageDecoder::ImageRequest,
   friend class ProfileDownloaderTest;
   FRIEND_TEST_ALL_PREFIXES(ProfileDownloaderTest, AccountInfoReady);
   FRIEND_TEST_ALL_PREFIXES(ProfileDownloaderTest, AccountInfoNotReady);
-  FRIEND_TEST_ALL_PREFIXES(ProfileDownloaderTest, DefaultURL);
+  FRIEND_TEST_ALL_PREFIXES(ProfileDownloaderTest,
+                           AccountInfoNoPictureDoesNotCrash);
+  FRIEND_TEST_ALL_PREFIXES(ProfileDownloaderTest,
+                           AccountInfoInvalidPictureURLDoesNotCrash);
 
   void FetchImageData();
 

chrome/browser/profiles/profile_downloader_delegate.h

@@ -18,10 +18,11 @@ class ProfileDownloaderDelegate {
  public:
   // Error codes passed to OnProfileDownloadFailure.
   enum FailureReason {
-    TOKEN_ERROR,         // Cannot fetch OAuth2 token.
-    NETWORK_ERROR,       // Network failure while downloading profile.
-    SERVICE_ERROR,       // Service returned an error or malformed reply.
-    IMAGE_DECODE_FAILED  // Cannot decode fetched image.
+    TOKEN_ERROR,          // Cannot fetch OAuth2 token.
+    NETWORK_ERROR,        // Network failure while downloading profile.
+    SERVICE_ERROR,        // Service returned an error or malformed reply.
+    IMAGE_DECODE_FAILED,  // Cannot decode fetched image.
+    INVALID_PROFILE_PICTURE_URL  // The profile picture URL is invalid.
   };
 
   virtual ~ProfileDownloaderDelegate() {}

chrome/browser/profiles/profile_downloader_unittest.cc

@@ -29,7 +29,8 @@ const std::string kTestHostedDomain = "google.com";
 const std::string kTestFullName = "full_name";
 const std::string kTestGivenName = "given_name";
 const std::string kTestLocale = "locale";
-const std::string kTestPictureURL = "http://www.google.com/";
+const std::string kTestValidPictureURL = "http://www.google.com/";
+const std::string kTestInvalidPictureURL = "invalid_picture_url";
 
 } // namespace
 
@@ -66,16 +67,12 @@ class ProfileDownloaderTest : public testing::Test,
       ProfileDownloader* downloader,
       ProfileDownloaderDelegate::FailureReason reason) override {}
 
-  void SimulateUserInfoSuccess() {
+  void SimulateUserInfoSuccess(const std::string& picture_url) {
     account_fetcher_service_->FakeUserInfoFetchSuccess(
-      account_tracker_service_->PickAccountIdForAccount(kTestGaia, kTestEmail),
-      kTestEmail,
-      kTestGaia,
-      kTestHostedDomain,
-      kTestFullName,
-      kTestGivenName,
-      kTestLocale,
-      kTestPictureURL);
+        account_tracker_service_->PickAccountIdForAccount(kTestGaia,
+                                                          kTestEmail),
+        kTestEmail, kTestGaia, kTestHostedDomain, kTestFullName, kTestGivenName,
+        kTestLocale, picture_url);
   }
 
   AccountTrackerService* account_tracker_service_;
@@ -88,13 +85,13 @@ class ProfileDownloaderTest : public testing::Test,
 TEST_F(ProfileDownloaderTest, AccountInfoReady) {
   std::string account_id =
       account_tracker_service_->SeedAccountInfo(kTestGaia, kTestEmail);
-  SimulateUserInfoSuccess();
+  SimulateUserInfoSuccess(kTestValidPictureURL);
 
   ASSERT_EQ(ProfileDownloader::PICTURE_FAILED,
             profile_downloader_->GetProfilePictureStatus());
   profile_downloader_->StartForAccount(account_id);
   profile_downloader_->StartFetchingImage();
-  ASSERT_EQ(kTestPictureURL, profile_downloader_->GetProfilePictureURL());
+  ASSERT_EQ(kTestValidPictureURL, profile_downloader_->GetProfilePictureURL());
 }
 
 TEST_F(ProfileDownloaderTest, AccountInfoNotReady) {
@@ -105,6 +102,34 @@ TEST_F(ProfileDownloaderTest, AccountInfoNotReady) {
             profile_downloader_->GetProfilePictureStatus());
   profile_downloader_->StartForAccount(account_id);
   profile_downloader_->StartFetchingImage();
-  SimulateUserInfoSuccess();
-  ASSERT_EQ(kTestPictureURL, profile_downloader_->GetProfilePictureURL());
+  SimulateUserInfoSuccess(kTestValidPictureURL);
+  ASSERT_EQ(kTestValidPictureURL, profile_downloader_->GetProfilePictureURL());
+}
+
+// Regression test for http://crbug.com/854907
+TEST_F(ProfileDownloaderTest, AccountInfoNoPictureDoesNotCrash) {
+  std::string account_id =
+      account_tracker_service_->SeedAccountInfo(kTestGaia, kTestEmail);
+  SimulateUserInfoSuccess(AccountTrackerService::kNoPictureURLFound);
+
+  profile_downloader_->StartForAccount(account_id);
+  profile_downloader_->StartFetchingImage();
+
+  EXPECT_TRUE(profile_downloader_->GetProfilePictureURL().empty());
+  ASSERT_EQ(ProfileDownloader::PICTURE_DEFAULT,
+            profile_downloader_->GetProfilePictureStatus());
+}
+
+// Regression test for http://crbug.com/854907
+TEST_F(ProfileDownloaderTest, AccountInfoInvalidPictureURLDoesNotCrash) {
+  std::string account_id =
+      account_tracker_service_->SeedAccountInfo(kTestGaia, kTestEmail);
+  SimulateUserInfoSuccess(kTestInvalidPictureURL);
+
+  profile_downloader_->StartForAccount(account_id);
+  profile_downloader_->StartFetchingImage();
+
+  EXPECT_TRUE(profile_downloader_->GetProfilePictureURL().empty());
+  ASSERT_EQ(ProfileDownloader::PICTURE_FAILED,
+            profile_downloader_->GetProfilePictureStatus());
 }