Add support to process favicons from Web Manifests

components/favicon/core/favicon_handler.cc

Index: components/favicon/core/favicon_handler.cc
diff --git a/components/favicon/core/favicon_handler.cc b/components/favicon/core/favicon_handler.cc
index 1434eb5ec11d59fde4792447bb79c712f4ed7163..89d415263cfb99312bbe9e86ca9a4369514d79b6 100644
--- a/components/favicon/core/favicon_handler.cc
+++ b/components/favicon/core/favicon_handler.cc
@@ -11,6 +11,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/feature_list.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/metrics/histogram_macros.h"
 #include "build/build_config.h"
@@ -23,6 +24,10 @@
 #include "ui/gfx/image/image_util.h"
 
 namespace favicon {
+
+const base::Feature kFaviconsFromWebManifest{"FaviconsFromWebManifest",
+                                             base::FEATURE_DISABLED_BY_DEFAULT};
+
 namespace {
 
 const int kNonTouchLargestIconSize = 192;
@@ -228,7 +233,9 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   initial_history_result_expired_or_incomplete_ = false;
   redownload_icons_ = false;
   got_favicon_from_history_ = false;
+  manifest_download_request_.Cancel();
   image_download_request_.Cancel();
+  manifest_url_.reset();
   candidates_.clear();
   notification_icon_url_ = GURL();
   notification_icon_type_ = favicon_base::INVALID_ICON;
@@ -316,10 +323,88 @@ void FaviconHandler::NotifyFaviconUpdated(const GURL& icon_url,
 
 void FaviconHandler::OnUpdateCandidates(
     const GURL& page_url,
-    const std::vector<FaviconURL>& candidates) {
+    const std::vector<FaviconURL>& candidates,
+    const base::Optional<GURL>& manifest_url) {

[pkotwicz, 2017/05/12 06:13:29]

I found two bugs :(

Scenario for Bug #1
1) OnUpdateCandidates() is initially called with no Web Manifest URL and the
database does not know about any of the favicon URLs
2) OnUpdateCandidates() is called with the same favicon URLs and a manifest URL.
The database has bitmap data for the manifest URL. The second
OnUpdateCandidates() call is called while a favicon download is in progress

The second call to OnUpdateCandidates() does not cancel the download because
OnGotFinalIconURLCandidates() is not called from
OnFaviconDataForManifestFromFaviconService().

I think that it would be clearer if OnGotFinalIconURLCandidates() was always
called.

Expected:
FaviconService::SetFavicons() is not called.
Actual:
FaviconService::SetFavicons() is called.

Scenario for Bug #2
1) OnUpdateCandidates() is initially called with no Web Manifest URL and the
database does not know about any of the favicon URLs
2) OnUpdateCandidates() is called with the same favicon URLs and a manifest URL.
The Web Manifest contains no icons. The second OnUpdateCandidates() call is
called while a "favicon URL database request is in progress"

Expected:
NotifyFaviconUpdated() is called
Actual:
NotifyFaviconUpdated() is not called

[mastiz, 2017/05/12 13:31:33]

I will work on this, I think you're right bugs exist. It also seems to me that
those are corner cases in reality, so what would you think of getting the
feature in and polishing them in the next two weeks?

[mastiz, 2017/05/15 14:07:00]

I added the two corresponding tests which now pass, please review carefully:
#1: AddKnownManifestViaJavascriptWhileImageDownload.
#2: AddManifestWithoutIconsViaJavascriptWhileDatabaseLookup.

+  DCHECK_EQ(page_url, url_);
   if (page_url != url_)
     return;
 
+  if (base::FeatureList::IsEnabled(kFaviconsFromWebManifest)) {
+    // |candidates| or |manifest_url| could have been modified via Javascript.

[pkotwicz, 2017/05/12 06:13:29]

Nit: You can move the if() statement on line 334 outside of the containing if()
statement

[mastiz, 2017/05/12 13:31:33]

Done.

+    if (manifest_url_ && manifest_url_ == manifest_url)
+      return;
+
+    manifest_url_ = manifest_url;
+  }
+
+  non_manifest_original_candidates_ = candidates;
+
+  // Check if the manifest was previously blacklisted (e.g. returned a 404) and
+  // ignore the manifest URL if that's the case.
+  if (manifest_url_ && service_->WasUnableToDownloadFavicon(*manifest_url_)) {
+    DVLOG(1) << "Skip failed Manifest: " << *manifest_url_;
+    manifest_url_.reset();
+  }
+
+  if (manifest_url_) {
+    cancelable_task_tracker_for_candidates_.TryCancelAll();

[pkotwicz, 2017/05/12 06:13:29]

For the sake of sanity, should we cancel |manifest_download_request_| here? It
does not make a practical difference but it spares me from considering the case
of an in progress Web Manifest download.

[mastiz, 2017/05/12 13:31:33]

Done.

+    // See if there is a cached favicon for the manifest.
+    GetFaviconAndUpdateMappingsUnlessIncognito(
+        /*icon_url=*/*manifest_url_, favicon_base::FAVICON,
+        base::Bind(&FaviconHandler::OnFaviconDataForManifestFromFaviconService,

[pkotwicz, 2017/05/12 07:20:26]

The following scenario is buggy:
1) User is using non incognito browser
2) User visits a page
- with a Web Manifest which is specified but does not contain any icon data
- with a single favicon URL specified
3) The database has a mapping between the page URL to the favicon URL
4) OnFaviconDataForInitialURLFromFaviconService() runs before
OnUpdateCandidates()

- The GetFaviconAndUpdateMappingsUnlessIncognito() database request on line 352
will delete the "correct page URL -> favicon URL mapping" and replace it with a
mapping from "page URL -> manifest URL" (because of
UpdateFaviconMappingsAndFetch())
- OnGotInitialHistoryDataAndIconURLCandidates() won't do any database lookups or
downloads because OnFaviconDataForInitialURLFromFaviconService() would have done
the only call to NotifyFaviconUpdated() (because the Web Manifest is empty)

I suggest changing FaviconHandler::OnUpdateCandidates() to

FaviconHandler::OnUpdateCandidates(const GURL& page_url,
                                   const std::vector<FaviconURL>& candidates,
                                   const GURL& manifest_url) {
  DCHECK_EQ(page_url, url_);
  if (page_url != url_)
    return;

  non_manifest_original_candidates_ = candidates;
  original_manifest_ = manifest;

  if (got_favicon_from_history_)
    OnGotInitialHistoryDataAndIconURLCandidates();
}

OnGotInitialHistoryDataAndIconURLCandidates() would do most of the processing
currently in FaviconHandler::OnUpdateCandidates() 

[pkotwicz, 2017/05/12 15:37:42]

I think that this bug affects https://www.twitter.com/manifest.json

[mastiz, 2017/05/15 14:07:00]

Added UnknownManifestWithoutIconsAndRegularIconInHistory which now passes.

In our offline discussion, you did mention something which I haven't addressed:
"I would rather delay the database fetch for the "manifest URL" till the
database fetch for the "page URL" has completed". I'm not even sure I understood
the proposal.

Possibly related, I added a TODO in the test reflecting a possible issue with
this approach. The only clean alternative I see here is to avoid
UpdateFaviconMappingsAndFetch() until we know a manifest contains icons and use
GetFavicon() instead. Would you prefer that?

[pkotwicz, 2017/05/16 06:36:35]

Yes I would prefer that. That works well with my plan with my plan of adding
FaviconService::UpdateFaviconMappings()

I had something like this in mind:

FaviconHandler::OnUpdateCandidates() {
  if (manifest_url_ == manifest_url &&
      std::equal(candidates.begin(), candidates.end(),
          non_manifest_original_candidates_.begin(),
          &FaviconURLEquals)) {
    return;
  }

  non_manifest_original_candidates_ = candidates;
  ...
  best_favicon_ = DownloadedFavicon();

  if (base::FeatureList::IsEnabled(kFaviconsFromWebManifest))
    manifest_url_ = manifest_url;

  if (got_favicon_from_history_) {
    DoNextStepOfProcessing();
  }
}

void DoNextStepOfProcessing() {
  if (manifest_url_.is_empty()) {
    OnGotFinalIconURLCandidates(non_manifest_original_candidates_);
    return;
  }

  // See if there is a cached favicon for the manifest. This will update the DB
  // mappings which is an optimistic approach, since the download of the
  // manifest might fail or it might list no icons (although
  // WasUnableToDownloadFavicon() above attempts to capture those cases).
  GetFaviconAndUpdateMappingsUnlessIncognito(
      /*icon_url=*/manifest_url_, favicon_base::FAVICON,
      base::Bind(&FaviconHandler::OnFaviconDataForManifestFromFaviconService,
                 base::Unretained(this)));
}

[mastiz, 2017/05/16 11:05:22]

I have some trouble following this discussion, might be worth a VC (luckily, we
have the weekly sync today).

Looks like got_favicon_from_history_ is the key behind the idea of "I would
rather delay the database fetch for the "manifest URL". However, remember that
the History task runner is a sequenced task runner and requests cannot overtake
each other, so OnFaviconDataForManifestFromFaviconService() is guaranteed to be
called after OnFaviconDataForInitialURLFromFaviconService(). I added a DCHECK
and a comment to reflect this.

Besides, I start to doubt the original description of the bug is legit. Won't
UpdateFaviconMappingsAndFetch(kManifestURL) be ignored because kManifestURL is
not known?

Anyway, this now has a browser test too:
LoadRegularIconDespiteWebManifestWithoutIcons

[mastiz, 2017/05/16 17:53:45]

As discussed offline: the original bug is not an actual issue so I reverted some
precautions, which are reflected in the unit test
(UnknownManifestWithoutIconsAndRegularIconInHistory). There was some red-herring
due to the browser test failing but that was an issue with the browser test
which I now fixed via a periodic check according to our offline discussion.

+                   base::Unretained(this)));
+  } else {
+    // If no manifest available, proceed with the regular candidates only.
+    OnGotFinalIconURLCandidates(candidates);
+  }
+}
+
+void FaviconHandler::OnFaviconDataForManifestFromFaviconService(
+    const std::vector<favicon_base::FaviconRawBitmapResult>&
+        favicon_bitmap_results) {
+  bool has_valid_result = HasValidResult(favicon_bitmap_results);
+  bool has_expired_or_incomplete_result =
+      !has_valid_result || HasExpiredOrIncompleteResult(preferred_icon_size(),
+                                                        favicon_bitmap_results);
+
+  if (has_valid_result) {
+    // There is a valid favicon. Notify any observers. It is useful to notify
+    // the observers even if the favicon is expired or incomplete (incorrect
+    // size) because temporarily showing the user an expired favicon or
+    // streched favicon is preferable to showing the user the default favicon.
+    NotifyFaviconUpdated(favicon_bitmap_results);
+  }
+
+  if (has_expired_or_incomplete_result) {
+    manifest_download_request_.Reset(base::Bind(
+        &FaviconHandler::OnDidDownloadManifest, base::Unretained(this)));
+    delegate_->DownloadManifest(*manifest_url_,
+                                manifest_download_request_.callback());
+  }
+}
+
+void FaviconHandler::OnDidDownloadManifest(
+    const std::vector<FaviconURL>& candidates) {
+  // Mark manifest download as finished.
+  manifest_download_request_.Cancel();
+
+  if (!candidates.empty()) {
+    OnGotFinalIconURLCandidates(candidates);
+    return;
+  }
+
+  // If either the downloading of the manifest failed, OR the manifest contains
+  // no icons, proceed with the list of icons listed in the HTML.
+  DVLOG(1) << "Could not fetch Manifest icons from " << *manifest_url_
+           << ", falling back to inlined ones, which are "
+           << non_manifest_original_candidates_.size();
+
+  manifest_url_.reset();
+  OnGotFinalIconURLCandidates(non_manifest_original_candidates_);
+}
+
+void FaviconHandler::OnGotFinalIconURLCandidates(
+    const std::vector<FaviconURL>& candidates) {
   std::vector<FaviconCandidate> sorted_candidates;
   const std::vector<int> desired_pixel_sizes =
       GetDesiredPixelSizes(handler_type_);
@@ -438,9 +523,10 @@ void FaviconHandler::OnDidDownloadFavicon(
                                          num_image_download_requests_);
     // We have either found the ideal candidate or run out of candidates.
     if (best_favicon_.candidate.icon_type != favicon_base::INVALID_ICON) {
-      // No more icons to request, set the favicon from the candidate.
-      SetFavicon(best_favicon_.candidate.icon_url, best_favicon_.image,
-                 best_favicon_.candidate.icon_type);
+      // No more icons to request, set the favicon from the candidate. Note that
+      // manifest URLs override icon URLs if available.

[pkotwicz, 2017/05/12 06:13:29]

Nit: "Note that manifest URLs override icon URLs if available." -> "The Manifest
URL, if available, is used instead of the icon URL."

There is only one "manifest URL"

[mastiz, 2017/05/12 13:31:33]

Done.

+      SetFavicon(manifest_url_.value_or(best_favicon_.candidate.icon_url),
+                 best_favicon_.image, best_favicon_.candidate.icon_type);
     }
     // Clear download related state.
     current_candidate_index_ = candidates_.size();
@@ -458,6 +544,7 @@ const std::vector<GURL> FaviconHandler::GetIconURLs() const {
 
 bool FaviconHandler::HasPendingTasksForTest() {
   return !image_download_request_.IsCancelled() ||
+         !manifest_download_request_.IsCancelled() ||
          cancelable_task_tracker_for_page_url_.HasTrackedTasks() ||
          cancelable_task_tracker_for_candidates_.HasTrackedTasks();
 }
@@ -482,15 +569,20 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
   redownload_icons_ = initial_history_result_expired_or_incomplete_ &&
                       !favicon_bitmap_results.empty();
 
-  if (has_valid_result && (!current_candidate() ||
-                           DoUrlsAndIconsMatch(current_candidate()->icon_url,
-                                               current_candidate()->icon_type,
-                                               favicon_bitmap_results))) {
-    // The db knows the favicon (although it may be out of date) and the entry
-    // doesn't have an icon. Set the favicon now, and if the favicon turns out
-    // to be expired (or the wrong url) we'll fetch later on. This way the
-    // user doesn't see a flash of the default favicon.
-    NotifyFaviconUpdated(favicon_bitmap_results);
+  if (has_valid_result) {

[pkotwicz, 2017/05/12 06:13:29]

Optional: In a separate CL you can remove the if() statement on line 573 and
always call NotifyFaviconUpdated() if |has_valid_result| == true

The rationale is that in the common case where current_candidate() == null we
call NotifyFaviconUpdated() (despite it being possible that the favicon URLs
once we get them won't match what is in the database).

It should be ok to call NotifyFaviconUpdated() if:
- we get the favicon URLs prior to the database request completing
- the data stored in the database does not match the favicon URLs from the
renderer

+    if (!current_candidate() ||
+        DoUrlsAndIconsMatch(current_candidate()->icon_url,
+                            current_candidate()->icon_type,
+                            favicon_bitmap_results) ||
+        (manifest_url_ &&
+         DoUrlsAndIconsMatch(*manifest_url_, favicon_base::FAVICON,
+                             favicon_bitmap_results))) {
+      // The db knows the favicon (although it may be out of date) and the entry
+      // doesn't have an icon. Set the favicon now, and if the favicon turns out
+      // to be expired (or the wrong url) we'll fetch later on. This way the
+      // user doesn't see a flash of the default favicon.
+      NotifyFaviconUpdated(favicon_bitmap_results);
+    }
   }
 
   if (current_candidate())
@@ -498,32 +590,40 @@ void FaviconHandler::OnFaviconDataForInitialURLFromFaviconService(
 }
 
 void FaviconHandler::DownloadCurrentCandidateOrAskFaviconService() {
-  GURL icon_url = current_candidate()->icon_url;
-  favicon_base::IconType icon_type = current_candidate()->icon_type;
-
-  if (redownload_icons_) {
+  const GURL icon_url = current_candidate()->icon_url;
+  const favicon_base::IconType icon_type = current_candidate()->icon_type;
+  // If the icons listed in a manifest are being processed, skip the cache
+  // lookup for |icon_url| since the manifest's URL is used for caching anyway,
+  // and this lookup has happened earlier.

[pkotwicz, 2017/05/12 06:13:29]

How about: "since the manifest's URL is using for caching anyway," -> "since the
manifest's URL is used for caching, not the icon URL,"

I think that my version is clearer

[mastiz, 2017/05/12 13:31:33]

Done.

+  if (redownload_icons_ || manifest_url_) {
     // We have the mapping, but the favicon is out of date. Download it now.
     ScheduleImageDownload(icon_url, icon_type);
   } else {
-    // We don't know the favicon, but we may have previously downloaded the
-    // favicon for another page that shares the same favicon. Ask for the
-    // favicon given the favicon URL.
-    if (delegate_->IsOffTheRecord()) {
-      service_->GetFavicon(
-          icon_url, icon_type, preferred_icon_size(),
-          base::Bind(&FaviconHandler::OnFaviconData, base::Unretained(this)),
-          &cancelable_task_tracker_for_candidates_);
-    } else {
-      // Ask the history service for the icon. This does two things:
-      // 1. Attempts to fetch the favicon data from the database.
-      // 2. If the favicon exists in the database, this updates the database to
-      //    include the mapping between the page url and the favicon url.
-      // This is asynchronous. The history service will call back when done.
-      service_->UpdateFaviconMappingsAndFetch(
-          url_, icon_url, icon_type, preferred_icon_size(),
-          base::Bind(&FaviconHandler::OnFaviconData, base::Unretained(this)),
-          &cancelable_task_tracker_for_candidates_);
-    }
+    GetFaviconAndUpdateMappingsUnlessIncognito(
+        icon_url, icon_type,
+        base::Bind(&FaviconHandler::OnFaviconData, base::Unretained(this)));
+  }
+}
+
+void FaviconHandler::GetFaviconAndUpdateMappingsUnlessIncognito(
+    const GURL& icon_url,
+    favicon_base::IconType icon_type,
+    const favicon_base::FaviconResultsCallback& callback) {
+  // We don't know the favicon, but we may have previously downloaded the
+  // favicon for another page that shares the same favicon. Ask for the
+  // favicon given the favicon URL.
+  if (delegate_->IsOffTheRecord()) {
+    service_->GetFavicon(icon_url, icon_type, preferred_icon_size(), callback,
+                         &cancelable_task_tracker_for_candidates_);
+  } else {
+    // Ask the history service for the icon. This does two things:
+    // 1. Attempts to fetch the favicon data from the database.
+    // 2. If the favicon exists in the database, this updates the database to
+    //    include the mapping between the page url and the favicon url.
+    // This is asynchronous. The history service will call back when done.
+    service_->UpdateFaviconMappingsAndFetch(
+        url_, icon_url, icon_type, preferred_icon_size(), callback,
+        &cancelable_task_tracker_for_candidates_);
   }
 }