Fix FaviconHandler when it has multiple icon URLs and one of them is invalid

components/favicon/core/favicon_handler_unittest.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 "components/favicon/core/favicon_handler.h"
 
+#include<set>
+#include<vector>
+
 #include "base/memory/scoped_ptr.h"
 #include "components/favicon/core/favicon_driver.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/layout.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/favicon_size.h"
 #include "ui/gfx/image/image.h"
 
 namespace favicon {
@@ skipping 45 matching lines @@
                             favicon_base::FAVICON,
                             false /* expired */,
                             favicon_bitmap_results);
 }
 
 // This class is used to save the download request for verifying with test case.
 // It also will be used to invoke the onDidDownload callback.
 class DownloadHandler {
  public:
   explicit DownloadHandler(FaviconHandler* favicon_handler)
-      : favicon_handler_(favicon_handler), failed_(false) {}
+      : favicon_handler_(favicon_handler), callback_invoked_(false) {}
 
   ~DownloadHandler() {}
 
   void Reset() {
     download_.reset();
-    failed_ = false;
+    callback_invoked_ = false;
+    // Does not affect |should_fail_download_icon_urls_| and
+    // |failed_download_icon_urls_|.
+  }
+
+  // Make downloads for any of |icon_urls| fail.
+  void FailDownloadForIconURLs(const std::set<GURL>& icon_urls) {
+    should_fail_download_icon_urls_ = icon_urls;
+  }
+
+  // Returns whether a download for |icon_url| did fail.
+  bool DidFailDownloadForIconURL(const GURL& icon_url) const {
+    return failed_download_icon_urls_.count(icon_url);
   }
 
   void AddDownload(
       int download_id,
       const GURL& image_url,
       const std::vector<int>& image_sizes,
       int max_image_size) {
     download_.reset(new Download(
-        download_id, image_url, image_sizes, max_image_size, false));
+        download_id, image_url, image_sizes, max_image_size));
   }
 
   void InvokeCallback();
 
-  void set_failed(bool failed) { failed_ = failed; }
-
   bool HasDownload() const { return download_.get(); }
   const GURL& GetImageUrl() const { return download_->image_url; }
   void SetImageSizes(const std::vector<int>& sizes) {
     download_->image_sizes = sizes; }
 
  private:
   struct Download {
     Download(int id,
              GURL url,
              const std::vector<int>& sizes,
-             int max_size,
-             bool failed)
+             int max_size)
         : download_id(id),
           image_url(url),
           image_sizes(sizes),
           max_image_size(max_size) {}
     ~Download() {}
     int download_id;
     GURL image_url;
     std::vector<int> image_sizes;
     int max_image_size;
   };
 
   FaviconHandler* favicon_handler_;
   scoped_ptr<Download> download_;
-  bool failed_;
+  bool callback_invoked_;
+
+  // The icon URLs for which the download should fail.
+  std::set<GURL> should_fail_download_icon_urls_;
+
+  // The icon URLs for which the download did fail. This should be a subset of
+  // |should_fail_download_icon_urls_|.
+  std::set<GURL> failed_download_icon_urls_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadHandler);
 };
 
 // This class is used to save the history request for verifying with test case.
 // It also will be used to simulate the history response.
 class HistoryRequestHandler {
  public:
   HistoryRequestHandler(const GURL& page_url,
                         const GURL& icon_url,
@@ skipping 222 matching lines @@
   void GetFaviconForURLFromFaviconService(
       const GURL& page_url,
       int icon_types,
       const favicon_base::FaviconResultsCallback& callback,
       base::CancelableTaskTracker* tracker) override {
     history_handler_.reset(new HistoryRequestHandler(page_url, GURL(),
                                                      icon_types, callback));
   }
 
   int DownloadFavicon(const GURL& image_url, int max_bitmap_size) override {
+    // Do not do a download if downloading |image_url| failed previously. This
+    // emulates the behavior of FaviconDriver::StartDownload()
+    if (download_handler_->DidFailDownloadForIconURL(image_url)) {
+      download_handler_->AddDownload(download_id_, image_url,
+                                     std::vector<int>(), 0);
+      return 0;
+    }
+
     download_id_++;
     std::vector<int> sizes;
     sizes.push_back(0);
     download_handler_->AddDownload(
         download_id_, image_url, sizes, max_bitmap_size);
     return download_id_;
   }
 
   void SetHistoryFavicons(const GURL& page_url,
                           const GURL& icon_url,
@@ skipping 24 matching lines @@
 
 namespace {
 
 void HistoryRequestHandler::InvokeCallback() {
   if (!callback_.is_null()) {
     callback_.Run(history_results_);
   }
 }
 
 void DownloadHandler::InvokeCallback() {
+  if (callback_invoked_)
+    return;
+
   std::vector<gfx::Size> original_bitmap_sizes;
   std::vector<SkBitmap> bitmaps;
-  if (!failed_) {
+  if (should_fail_download_icon_urls_.count(download_->image_url)) {
+    failed_download_icon_urls_.insert(download_->image_url);
+  } else {
     for (std::vector<int>::const_iterator i = download_->image_sizes.begin();
          i != download_->image_sizes.end(); ++i) {
       int original_size = (*i > 0) ? *i : gfx::kFaviconSize;
       int downloaded_size = original_size;
       if (download_->max_image_size != 0 &&
           downloaded_size > download_->max_image_size) {
         downloaded_size = download_->max_image_size;
       }
       SkBitmap bitmap;
       FillDataToBitmap(downloaded_size, downloaded_size, &bitmap);
       bitmaps.push_back(bitmap);
       original_bitmap_sizes.push_back(gfx::Size(original_size, original_size));
     }
   }
   favicon_handler_->OnDidDownloadFavicon(download_->download_id,
                                          download_->image_url, bitmaps,
                                          original_bitmap_sizes);
+  callback_invoked_ = true;
 }
 
 class FaviconHandlerTest : public testing::Test {
  public:
   FaviconHandlerTest() {
   }
 
   ~FaviconHandlerTest() override {}
 
   // Simulates requesting a favicon for |page_url| given:
@@ skipping 17 matching lines @@
       favicon_handler->history_handler()->history_results_.clear();
       favicon_handler->history_handler()->InvokeCallback();
       ASSERT_TRUE(download_handler->HasDownload());
       EXPECT_EQ(download_handler->GetImageUrl(),
                 candidate_icons[i].icon_url);
       std::vector<int> sizes;
       sizes.push_back(candidate_icon_sizes[i]);
       download_handler->SetImageSizes(sizes);
       download_handler->InvokeCallback();
 
+      download_handler->Reset();
+
       if (favicon_driver->num_active_favicon())
         return;
     }
   }
 
   void UpdateFaviconURL(TestFaviconDriver* favicon_driver,
                         TestFaviconHandler* favicon_handler,
                         const GURL& page_url,
                         const std::vector<FaviconURL>& candidate_icons) {
     favicon_driver->ResetNumActiveFavicon();
@@ skipping 337 matching lines @@
   EXPECT_FALSE(driver.GetActiveFaviconImage().IsEmpty());
 }
 
 TEST_F(FaviconHandlerTest, Download2ndFaviconURLCandidate) {
   const GURL page_url("http://www.google.com");
   const GURL icon_url("http://www.google.com/favicon");
   const GURL new_icon_url("http://www.google.com/new_favicon");
 
   TestFaviconDriver driver;
   TestFaviconHandler helper(page_url, &driver, FaviconHandler::TOUCH, false);
+  std::set<GURL> fail_downloads;
+  fail_downloads.insert(icon_url);
+  helper.download_handler()->FailDownloadForIconURLs(fail_downloads);
 
   helper.FetchFavicon(page_url);
   HistoryRequestHandler* history_handler = helper.history_handler();
   // Ensure the data given to history is correct.
   ASSERT_TRUE(history_handler);
   EXPECT_EQ(page_url, history_handler->page_url_);
   EXPECT_EQ(GURL(), history_handler->icon_url_);
   EXPECT_EQ(favicon_base::TOUCH_PRECOMPOSED_ICON | favicon_base::TOUCH_ICON,
             history_handler->icon_type_);
 
@@ skipping 41 matching lines @@
   // Should request download favicon.
   DownloadHandler* download_handler = helper.download_handler();
   EXPECT_TRUE(helper.download_handler()->HasDownload());
 
   // Verify the download request.
   EXPECT_EQ(icon_url, download_handler->GetImageUrl());
 
   // Reset the history_handler to verify whether favicon is request from
   // history.
   helper.set_history_handler(nullptr);
-  // Smulates download failed.
-  download_handler->set_failed(true);
   download_handler->InvokeCallback();
 
   // Left 1 url.
   EXPECT_EQ(1U, helper.urls().size());
   ASSERT_TRUE(helper.current_candidate());
   EXPECT_EQ(new_icon_url, helper.current_candidate()->icon_url);
   EXPECT_EQ(favicon_base::TOUCH_ICON, helper.current_candidate()->icon_type);
 
   // Favicon should be requested from history.
   history_handler = helper.history_handler();
@@ skipping 135 matching lines @@
   SetFaviconRawBitmapResult(latest_icon_url,
                             favicon_base::TOUCH_ICON,
                             false /* expired */,
                             &handler->history_results_);
   handler->InvokeCallback();
 
   // No download request.
   EXPECT_FALSE(download_handler->HasDownload());
 }
 
-#if !defined(OS_ANDROID)
-
 // Test the favicon which is selected when the web page provides several
 // favicons and none of the favicons are cached in history.
 // The goal of this test is to be more of an integration test than
 // SelectFaviconFramesTest.*.
 TEST_F(FaviconHandlerTest, MultipleFavicons) {
   const GURL kPageURL("http://www.google.com");
   const FaviconURL kSourceIconURLs[] = {
       FaviconURL(GURL("http://www.google.com/a"),
                  favicon_base::FAVICON,
                  std::vector<gfx::Size>()),
@@ skipping 82 matching lines @@
                                 kSourceIconURLs + arraysize(kSizes4));
   DownloadTillDoneIgnoringHistory(
       &driver4, &handler4, kPageURL, urls4, kSizes4);
   EXPECT_TRUE(driver4.GetActiveFaviconValidity());
   expected_index = 0u;
   EXPECT_EQ(17, kSizes4[expected_index]);
   EXPECT_EQ(kSourceIconURLs[expected_index].icon_url,
             driver4.GetActiveFaviconURL());
 }
 
-#endif
+// Test that the best favicon is selected when:
+// - The page provides several favicons.
+// - Downloading one of the page's icon URLs previously returned a 404.
+// - None of the favicons are cached in the Favicons database.
+TEST_F(FaviconHandlerTest, MultipleFavicons404) {
+  const GURL kPageURL("http://www.google.com");
+  const GURL k404IconURL("http://www.google.com/404.png");
+  const FaviconURL k404FaviconURL(
+      k404IconURL, favicon_base::FAVICON, std::vector<gfx::Size>());
+  const FaviconURL kFaviconURLs[] = {
+      FaviconURL(GURL("http://www.google.com/a"),
+                 favicon_base::FAVICON,
+                 std::vector<gfx::Size>()),
+      k404FaviconURL,
+      FaviconURL(GURL("http://www.google.com/c"),
+                 favicon_base::FAVICON,
+                 std::vector<gfx::Size>()),
+  };
+
+  TestFaviconDriver driver;
+  TestFaviconHandler handler(kPageURL, &driver, FaviconHandler::FAVICON, false);
+  DownloadHandler* download_handler = handler.download_handler();
+
+  std::set<GURL> k404URLs;
+  k404URLs.insert(k404IconURL);
+  download_handler->FailDownloadForIconURLs(k404URLs);
+
+  // Make the initial download for |k404IconURL| fail.
+  const int kSizes1[] = { 0 };
+  std::vector<FaviconURL> urls1(1u, k404FaviconURL);
+  DownloadTillDoneIgnoringHistory(
+      &driver, &handler, kPageURL, urls1, kSizes1);
+  EXPECT_TRUE(download_handler->DidFailDownloadForIconURL(k404IconURL));
+
+  // Do a fetch now that the initial download for |k404IconURL| has failed. The
+  // behavior is different because OnDidDownloadFavicon() is invoked
+  // synchronously from DownloadFavicon().
+  const int kSizes2[] = { 10, 0, 16 };
+  std::vector<FaviconURL> urls2(kFaviconURLs,
+                                kFaviconURLs + arraysize(kFaviconURLs));
+  DownloadTillDoneIgnoringHistory(
+      &driver, &handler, kPageURL, urls2, kSizes2);
+
+  EXPECT_EQ(0u, handler.image_urls().size());
+  EXPECT_TRUE(driver.GetActiveFaviconValidity());
+  EXPECT_FALSE(driver.GetActiveFaviconImage().IsEmpty());
+  int expected_index = 2u;
+  EXPECT_EQ(16, kSizes2[expected_index]);
+  EXPECT_EQ(kFaviconURLs[expected_index].icon_url,
+            driver.GetActiveFaviconURL());
+}
+
+// Test that no favicon is selected when:
+// - The page provides several favicons.
+// - Downloading the page's icons has previously returned a 404.
+// - None of the favicons are cached in the Favicons database.
+TEST_F(FaviconHandlerTest, MultipleFaviconsAll404) {
+  const GURL kPageURL("http://www.google.com");
+  const GURL k404IconURL1("http://www.google.com/4041.png");
+  const GURL k404IconURL2("http://www.google.com/4042.png");
+  const FaviconURL kFaviconURLs[] = {
+      FaviconURL(k404IconURL1,
+                 favicon_base::FAVICON,
+                 std::vector<gfx::Size>()),
+      FaviconURL(k404IconURL2,
+                 favicon_base::FAVICON,
+                 std::vector<gfx::Size>()),
+  };
+
+  TestFaviconDriver driver;
+  TestFaviconHandler handler(kPageURL, &driver, FaviconHandler::FAVICON, false);
+  DownloadHandler* download_handler = handler.download_handler();
+
+  std::set<GURL> k404URLs;
+  k404URLs.insert(k404IconURL1);
+  k404URLs.insert(k404IconURL2);
+  download_handler->FailDownloadForIconURLs(k404URLs);
+
+  // Make the initial downloads for |kFaviconURLs| fail.
+  for (const FaviconURL& favicon_url : kFaviconURLs) {
+    const int kSizes[] = { 0 };
+    std::vector<FaviconURL> urls(1u, favicon_url);
+    DownloadTillDoneIgnoringHistory(&driver, &handler, kPageURL, urls, kSizes);
+  }
+  EXPECT_TRUE(download_handler->DidFailDownloadForIconURL(k404IconURL1));
+  EXPECT_TRUE(download_handler->DidFailDownloadForIconURL(k404IconURL2));
+
+  // Do a fetch now that the initial downloads for |kFaviconURLs| have failed.
+  // The behavior is different because OnDidDownloadFavicon() is invoked
+  // synchronously from DownloadFavicon().
+  const int kSizes[] = { 0, 0 };
+  std::vector<FaviconURL> urls(kFaviconURLs,
+                               kFaviconURLs + arraysize(kFaviconURLs));
+  DownloadTillDoneIgnoringHistory(&driver, &handler, kPageURL, urls, kSizes);
+
+  EXPECT_EQ(0u, handler.image_urls().size());
+  EXPECT_FALSE(driver.GetActiveFaviconValidity());
+  EXPECT_TRUE(driver.GetActiveFaviconImage().IsEmpty());
+}
+
+// Test that no favicon is selected when the page's only icon uses an invalid
+// URL syntax.
+TEST_F(FaviconHandlerTest, FaviconInvalidURL) {
+  const GURL kPageURL("http://www.google.com");
+  const GURL kInvalidFormatURL("invalid");
+  ASSERT_TRUE(kInvalidFormatURL.is_empty());
+
+  FaviconURL favicon_url(kInvalidFormatURL, favicon_base::FAVICON,
+                         std::vector<gfx::Size>());
+
+  TestFaviconDriver driver;
+  TestFaviconHandler handler(kPageURL, &driver, FaviconHandler::FAVICON, false);
+  UpdateFaviconURL(&driver, &handler, kPageURL,
+                   std::vector<FaviconURL>(1u, favicon_url));
+  EXPECT_EQ(0u, handler.image_urls().size());
+}
 
 TEST_F(FaviconHandlerTest, TestSortFavicon) {
   const GURL kPageURL("http://www.google.com");
   std::vector<gfx::Size> icon1;
   icon1.push_back(gfx::Size(1024, 1024));
   icon1.push_back(gfx::Size(512, 512));
 
   std::vector<gfx::Size> icon2;
   icon2.push_back(gfx::Size(15, 15));
   icon2.push_back(gfx::Size(16, 16));
@@ skipping 72 matching lines @@
       FaviconURL(GURL("http://www.google.com/d"),
                  favicon_base::FAVICON,
                  std::vector<gfx::Size>()),
       FaviconURL(GURL("http://www.google.com/e"),
                  favicon_base::FAVICON,
                  std::vector<gfx::Size>())};
 
   TestFaviconDriver driver1;
   TestFaviconHandler handler1(kPageURL, &driver1, FaviconHandler::FAVICON,
                               true);
+
+  std::set<GURL> fail_icon_urls;
+  for (size_t i = 0; i < arraysize(kSourceIconURLs); ++i) {
+    fail_icon_urls.insert(kSourceIconURLs[i].icon_url);
+  }
+  handler1.download_handler()->FailDownloadForIconURLs(fail_icon_urls);
+
   std::vector<FaviconURL> urls1(kSourceIconURLs,
                                 kSourceIconURLs + arraysize(kSourceIconURLs));
   UpdateFaviconURL(&driver1, &handler1, kPageURL, urls1);
 
   // Simulate the download failed, to check whether the icons were requested
   // to download according their size.
   struct ExpectedResult {
     // The size of image_urls_.
     size_t image_urls_size;
     // The favicon's index in kSourceIconURLs.
@@ skipping 20 matching lines @@
 
     // Simulate no favicon from history.
     handler1.history_handler()->history_results_.clear();
     handler1.history_handler()->InvokeCallback();
 
     // Verify download request
     ASSERT_TRUE(handler1.download_handler()->HasDownload());
     EXPECT_EQ(kSourceIconURLs[results[i].favicon_index].icon_url,
               handler1.download_handler()->GetImageUrl());
 
-    // Simulate the download failed.
-    handler1.download_handler()->set_failed(true);
     handler1.download_handler()->InvokeCallback();
+    handler1.download_handler()->Reset();
   }
 }
 
 TEST_F(FaviconHandlerTest, TestSelectLargestFavicon) {
   const GURL kPageURL("http://www.google.com");
 
   std::vector<gfx::Size> one_icon;
   one_icon.push_back(gfx::Size(15, 15));
 
   std::vector<gfx::Size> two_icons;
@@ skipping 149 matching lines @@
   // Verify the first icon was request to download
   ASSERT_TRUE(handler1.download_handler()->HasDownload());
   EXPECT_EQ(kSourceIconURLs[0].icon_url,
             handler1.download_handler()->GetImageUrl());
 
   // Give the incorrect size.
   std::vector<int> sizes;
   sizes.push_back(actual_size1);
   handler1.download_handler()->SetImageSizes(sizes);
   handler1.download_handler()->InvokeCallback();
+  handler1.download_handler()->Reset();
 
   // Simulate no favicon from history.
   handler1.history_handler()->history_results_.clear();
   handler1.history_handler()->InvokeCallback();
 
   // Verify the 2nd icon was request to download
   ASSERT_TRUE(handler1.download_handler()->HasDownload());
   EXPECT_EQ(kSourceIconURLs[1].icon_url,
             handler1.download_handler()->GetImageUrl());
 
   // Very the best candidate is icon1
   EXPECT_EQ(kSourceIconURLs[0].icon_url,
             handler1.best_favicon_candidate().image_url);
   EXPECT_EQ(gfx::Size(actual_size1, actual_size1),
             handler1.best_favicon_candidate().image.Size());
 
   // Give the incorrect size.
   sizes.clear();
   sizes.push_back(actual_size2);
   handler1.download_handler()->SetImageSizes(sizes);
   handler1.download_handler()->InvokeCallback();
+  handler1.download_handler()->Reset();
 
   // Verify icon2 has been saved into history.
   EXPECT_EQ(kSourceIconURLs[1].icon_url, handler1.history_handler()->icon_url_);
   EXPECT_EQ(gfx::Size(actual_size2, actual_size2),
             handler1.history_handler()->size_);
 }
 
 class FaviconHandlerActiveFaviconValidityParamTest :
       public FaviconHandlerTest,
       public ::testing::WithParamInterface<bool> {
@@ skipping 70 matching lines @@
   EXPECT_FALSE(driver1.update_active_favicon());
   EXPECT_EQ(3u, driver1.num_favicon_available());
 }
 
 INSTANTIATE_TEST_CASE_P(FaviconHandlerTestActiveFaviconValidityTrueOrFalse,
                         FaviconHandlerActiveFaviconValidityParamTest,
                         ::testing::Bool());
 
 }  // namespace
 }  // namespace favicon