Reland "Extend bookmark sync code with support for favicon deletion"

This is a reland of d63652c8

The new version reverts a change in tests (removal of
urls_with_favicons_) which presumably introduces flakiness on Mac.
My hypothesis is that using urls_with_favicons_ hides a race
condition that predates my patch, because the failing test is
unrelated to favicons, TwoClientBookmarksSyncTest.Sanity, and the
failing logs surface title mismatches as well:
[...:bookmarks_helper.cc(377)] Title mismatch: Yahoo vs. Google

Original change's description:
> Extend bookmark sync code with support for favicon deletion
> 
> Recent patches have introduced new favicon scenarios by handling cases
> where a site's content changes to not contain favicons.
> 
> Updating sync code to handle these events is needed to prevent sync
> from resurrecting the favicon, due to not knowing about the deletion
> and hence sync-ing it back to the local cache.
> 
> One notable change is that the new logic will introduce the concept of
> sync-ed bookmark protos having neither an icon URL (field |icon_url|)
> nor favicon image data (field |favicon|). Ancient versions of chrome
> (prior to M25) didn't sync these fields and would be incompatible
> (although not really badly) but too old for us to care.
> 
> Manually tested sync-ing across two patched instances of Chrome desktop
> (Linux):
> - Propagate deletions of bookmark favicons (after page content changes).
> - Restoring deleted bookmark favicons (after page content changes).
> 
> Bug: 85806
> Change-Id: Ia3534c6094983658f59f973777b5ed649c8769e8
> Reviewed-on: https://chromium-review.googlesource.com/704737
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Nicolas Zea <zea@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#514424}

Bug: 85806
Change-Id: If4d05da9e13c1d9d5abb19fd8a7c1d463af609a4
Reviewed-on: https://chromium-review.googlesource.com/757516Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Nicolas Zea <zea@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514934}

chrome/browser/sync/test/integration/bookmarks_helper.cc

@@ -90,7 +90,6 @@ class FaviconChangeObserver : public bookmarks::BookmarkModelObserver {
     wait_for_load_ = true;
     content::RunMessageLoop();
     ASSERT_TRUE(node_->is_favicon_loaded());
-    ASSERT_FALSE(model_->GetFavicon(node_).IsEmpty());
   }
   void WaitForSetFavicon() {
     wait_for_load_ = false;
@@ -278,16 +277,46 @@ void ExpireFaviconImpl(Profile* profile, const BookmarkNode* node) {
   favicon_service->SetFaviconOutOfDateForPage(node->url());
 }
 
-// Called asynchronously from CheckFaviconExpired() with the favicon data from
-// the database.
-void OnGotFaviconForExpiryCheck(
+// Used to call FaviconService APIs synchronously by making |callback| quit a
+// RunLoop.
+void OnGotFaviconData(
     const base::Closure& callback,
+    favicon_base::FaviconRawBitmapResult* output_bitmap_result,
     const favicon_base::FaviconRawBitmapResult& bitmap_result) {
-  ASSERT_TRUE(bitmap_result.is_valid());
-  ASSERT_TRUE(bitmap_result.expired);
+  *output_bitmap_result = bitmap_result;
   callback.Run();
 }
 
+// Deletes favicon mappings for |profile| and |node|. |profile| may be
+// |test()->verifier()|.
+void DeleteFaviconMappingsImpl(Profile* profile,
+                               const BookmarkNode* node,
+                               bookmarks_helper::FaviconSource favicon_source) {
+  BookmarkModel* model = BookmarkModelFactory::GetForBrowserContext(profile);
+
+  FaviconChangeObserver observer(model, node);
+  favicon::FaviconService* favicon_service =
+      FaviconServiceFactory::GetForProfile(profile,
+                                           ServiceAccessType::EXPLICIT_ACCESS);
+
+  if (favicon_source == bookmarks_helper::FROM_UI) {
+    favicon_service->DeleteFaviconMappings({node->url()},
+                                           favicon_base::IconType::kFavicon);
+  } else {
+    browser_sync::ProfileSyncService* pss =
+        ProfileSyncServiceFactory::GetForProfile(profile);
+    sync_bookmarks::BookmarkChangeProcessor::ApplyBookmarkFavicon(
+        node, pss->GetSyncClient(), /*icon_url=*/GURL(),
+        scoped_refptr<base::RefCountedString>(new base::RefCountedString()));
+  }
+
+  // Wait for the favicon for |node| to be invalidated.
+  observer.WaitForSetFavicon();
+  // Wait for the BookmarkModel to fetch the updated favicon and for the new
+  // favicon to be sent to BookmarkChangeProcessor.
+  GetFaviconData(model, node);
+}
+
 // Wait for all currently scheduled tasks on the history thread for all
 // profiles to complete and any notifications sent to the UI thread to have
 // finished processing.
@@ -633,12 +662,55 @@ void CheckFaviconExpired(int profile, const GURL& icon_url) {
           sync_datatype_helper::test()->GetProfile(profile),
           ServiceAccessType::EXPLICIT_ACCESS);
   base::CancelableTaskTracker task_tracker;
+  favicon_base::FaviconRawBitmapResult bitmap_result;
   favicon_service->GetRawFavicon(
       icon_url, favicon_base::IconType::kFavicon, 0,
-      base::Bind(&OnGotFaviconForExpiryCheck, run_loop.QuitClosure()),
+      base::Bind(&OnGotFaviconData, run_loop.QuitClosure(), &bitmap_result),
       &task_tracker);
+  run_loop.Run();
+
+  ASSERT_TRUE(bitmap_result.is_valid());
+  ASSERT_TRUE(bitmap_result.expired);
+}
+
+void CheckHasNoFavicon(int profile, const GURL& page_url) {
+  base::RunLoop run_loop;
 
+  favicon::FaviconService* favicon_service =
+      FaviconServiceFactory::GetForProfile(
+          sync_datatype_helper::test()->GetProfile(profile),
+          ServiceAccessType::EXPLICIT_ACCESS);
+  base::CancelableTaskTracker task_tracker;
+  favicon_base::FaviconRawBitmapResult bitmap_result;
+  favicon_service->GetRawFaviconForPageURL(
+      page_url, {favicon_base::IconType::kFavicon}, 0,
+      base::Bind(&OnGotFaviconData, run_loop.QuitClosure(), &bitmap_result),
+      &task_tracker);
   run_loop.Run();
+
+  ASSERT_FALSE(bitmap_result.is_valid());
+}
+
+void DeleteFaviconMappings(int profile,
+                           const BookmarkNode* node,
+                           FaviconSource favicon_source) {
+  BookmarkModel* model = GetBookmarkModel(profile);
+  ASSERT_EQ(bookmarks::GetBookmarkNodeByID(model, node->id()), node)
+      << "Node " << node->GetTitle() << " does not belong to "
+      << "Profile " << profile;
+  ASSERT_EQ(BookmarkNode::URL, node->type())
+      << "Node " << node->GetTitle() << " must be a url.";
+
+  if (sync_datatype_helper::test()->use_verifier()) {
+    const BookmarkNode* v_node = nullptr;
+    FindNodeInVerifier(model, node, &v_node);
+    DeleteFaviconMappingsImpl(sync_datatype_helper::test()->verifier(), v_node,
+                              favicon_source);
+  }
+  DeleteFaviconMappingsImpl(sync_datatype_helper::test()->GetProfile(profile),
+                            node, favicon_source);
+
+  WaitForHistoryToProcessPendingTasks();
 }
 
 const BookmarkNode* SetURL(int profile,

chrome/browser/sync/test/integration/bookmarks_helper.h

@@ -115,6 +115,14 @@ void ExpireFavicon(int profile, const bookmarks::BookmarkNode* node);
 // Checks whether the favicon at |icon_url| for |profile| is expired;
 void CheckFaviconExpired(int profile, const GURL& icon_url);
 
+// Deletes the favicon mappings for |node| in the bookmark model for |profile|.
+void DeleteFaviconMappings(int profile,
+                           const bookmarks::BookmarkNode* node,
+                           FaviconSource favicon_source);
+
+// Checks whether |page_url| for |profile| has no favicon mappings.
+void CheckHasNoFavicon(int profile, const GURL& page_url);
+
 // Changes the url of the node |node| in the bookmark model of profile
 // |profile| to |new_url|. Returns a pointer to the node with the changed url.
 const bookmarks::BookmarkNode* SetURL(int profile,

chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc

@@ -22,10 +22,12 @@ using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
 using bookmarks_helper::AddFolder;
 using bookmarks_helper::AddURL;
+using bookmarks_helper::CheckHasNoFavicon;
 using bookmarks_helper::CountBookmarksWithTitlesMatching;
 using bookmarks_helper::CountBookmarksWithUrlsMatching;
 using bookmarks_helper::CountFoldersWithTitlesMatching;
 using bookmarks_helper::Create1xFaviconFromPNGFile;
+using bookmarks_helper::CreateFavicon;
 using bookmarks_helper::GetBookmarkBarNode;
 using bookmarks_helper::GetBookmarkModel;
 using bookmarks_helper::GetOtherNode;
@@ -267,6 +269,34 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
   EXPECT_TRUE(original_favicon_bytes->Equals(final_favicon_bytes));
 }
 
+// Test that a client deletes favicons from sync when they have been removed
+// from the local database.
+IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest, DeleteFaviconFromSync) {
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(ModelMatchesVerifier(kSingleProfileIndex));
+
+  const GURL page_url("http://www.google.com");
+  const GURL icon_url("http://www.google.com/favicon.ico");
+  const BookmarkNode* bookmark = AddURL(kSingleProfileIndex, "title", page_url);
+  SetFavicon(0, bookmark, icon_url, CreateFavicon(SK_ColorWHITE),
+             bookmarks_helper::FROM_UI);
+  ASSERT_TRUE(
+      UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
+  ASSERT_TRUE(ModelMatchesVerifier(kSingleProfileIndex));
+
+  // Simulate receiving a favicon deletion from sync.
+  DeleteFaviconMappings(kSingleProfileIndex, bookmark,
+                        bookmarks_helper::FROM_SYNC);
+
+  ASSERT_TRUE(
+      UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
+  ASSERT_TRUE(ModelMatchesVerifier(kSingleProfileIndex));
+
+  CheckHasNoFavicon(kSingleProfileIndex, page_url);
+  EXPECT_TRUE(
+      GetBookmarkModel(kSingleProfileIndex)->GetFavicon(bookmark).IsEmpty());
+}
+
 IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
                        BookmarkAllNodesRemovedEvent) {
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";

chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc

@@ -37,12 +37,14 @@ using bookmarks_helper::AddURL;
 using bookmarks_helper::AllModelsMatch;
 using bookmarks_helper::AllModelsMatchVerifier;
 using bookmarks_helper::CheckFaviconExpired;
+using bookmarks_helper::CheckHasNoFavicon;
 using bookmarks_helper::ContainsDuplicateBookmarks;
 using bookmarks_helper::CountAllBookmarks;
 using bookmarks_helper::CountBookmarksWithTitlesMatching;
 using bookmarks_helper::CountBookmarksWithUrlsMatching;
 using bookmarks_helper::CountFoldersWithTitlesMatching;
 using bookmarks_helper::CreateFavicon;
+using bookmarks_helper::DeleteFaviconMappings;
 using bookmarks_helper::ExpireFavicon;
 using bookmarks_helper::GetBookmarkBarNode;
 using bookmarks_helper::GetManagedNode;
@@ -306,6 +308,35 @@ IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
   ASSERT_TRUE(BookmarksMatchVerifierChecker().Wait());
 }
 
+IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, SC_DeleteFavicon) {
+  const GURL page_url("http://www.google.com/a");
+  const GURL icon_url("http://www.google.com/favicon.ico");
+
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(AllModelsMatchVerifier());
+
+  const BookmarkNode* bookmark0 = AddURL(0, kGenericURLTitle, page_url);
+  ASSERT_NE(nullptr, bookmark0);
+
+  SetFavicon(0, bookmark0, icon_url, CreateFavicon(SK_ColorWHITE),
+             bookmarks_helper::FROM_UI);
+  ASSERT_TRUE(BookmarksMatchVerifierChecker().Wait());
+
+  DeleteFaviconMappings(0, bookmark0, bookmarks_helper::FROM_UI);
+  ASSERT_TRUE(BookmarksMatchVerifierChecker().Wait());
+
+  // Set the title for |page_url|. This should not revert the deletion of
+  // favicon mappings.
+  const char kNewTitle[] = "New Title";
+  ASSERT_STRNE(kGenericURLTitle, kNewTitle);
+  const BookmarkNode* bookmark1 = GetUniqueNodeByURL(1, page_url);
+  SetTitle(1, bookmark1, std::string(kNewTitle));
+  ASSERT_TRUE(BookmarksMatchVerifierChecker().Wait());
+
+  // |page_url| should still have no mapping.
+  CheckHasNoFavicon(0, page_url);
+}
+
 IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, SC_AddNonHTTPBMs) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(AllModelsMatchVerifier());

components/bookmarks/browser/bookmark_model.cc

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <functional>
+#include <string>
 #include <utility>
 
 #include "base/bind.h"
@@ -1025,6 +1026,9 @@ void BookmarkModel::OnFaviconDataAvailable(
     // Couldn't load the touch icon, fallback to the regular favicon.
     DCHECK(client_->PreferTouchIcon());
     LoadFavicon(node, favicon_base::IconType::kFavicon);
+  } else {
+    // No favicon available, but we still notify observers.
+    FaviconLoaded(node);
   }
 }
 

components/sync_bookmarks/bookmark_change_processor.cc

@@ -449,10 +449,10 @@ void BookmarkChangeProcessor::BookmarkNodeFaviconChanged(
     return;
   }
 
-  // Ignore changes with empty images. This can happen if the favicon is
-  // still being loaded.
-  const gfx::Image& favicon = model->GetFavicon(node);
-  if (favicon.IsEmpty()) {
+  // Ignore favicons that are being loaded.
+  if (!node->is_favicon_loaded()) {
+    // Sutble way to trigger a load of the favicon.
+    model->GetFavicon(node);
     return;
   }
 
@@ -844,7 +844,7 @@ const BookmarkNode* BookmarkChangeProcessor::CreateBookmarkNode(
 
 // static
 // Sets the favicon of the given bookmark node from the given sync node.
-bool BookmarkChangeProcessor::SetBookmarkFavicon(
+void BookmarkChangeProcessor::SetBookmarkFavicon(
     const syncer::BaseNode* sync_node,
     const BookmarkNode* bookmark_node,
     BookmarkModel* bookmark_model,
@@ -852,23 +852,12 @@ bool BookmarkChangeProcessor::SetBookmarkFavicon(
   const sync_pb::BookmarkSpecifics& specifics =
       sync_node->GetBookmarkSpecifics();
   const std::string& icon_bytes_str = specifics.favicon();
-  if (icon_bytes_str.empty())
-    return false;
-
   scoped_refptr<base::RefCountedString> icon_bytes(
       new base::RefCountedString());
   icon_bytes->data().assign(icon_bytes_str);
-  GURL icon_url(specifics.icon_url());
-
-  // Old clients may not be syncing the favicon URL. If the icon URL is not
-  // synced, use the page URL as a fake icon URL as it is guaranteed to be
-  // unique.
-  if (icon_url.is_empty())
-    icon_url = bookmark_node->url();
-
-  ApplyBookmarkFavicon(bookmark_node, sync_client, icon_url, icon_bytes);
 
-  return true;
+  ApplyBookmarkFavicon(bookmark_node, sync_client, GURL(specifics.icon_url()),
+                       icon_bytes);
 }
 
 // static
@@ -943,14 +932,36 @@ void BookmarkChangeProcessor::ApplyBookmarkFavicon(
   history::HistoryService* history = sync_client->GetHistoryService();
   favicon::FaviconService* favicon_service = sync_client->GetFaviconService();
 
+  // Some tests (that use FakeSyncClient) use no services.
+  if (history == nullptr)
+    return;
+
+  DCHECK(favicon_service);
   history->AddPageNoVisitForBookmark(bookmark_node->url(),
                                      bookmark_node->GetTitle());
+
+  GURL icon_url_to_use = icon_url;
+
+  if (icon_url.is_empty()) {
+    if (bitmap_data->size() == 0) {
+      // Empty icon URL and no bitmap data means no icon mapping.
+      favicon_service->DeleteFaviconMappings({bookmark_node->url()},
+                                             favicon_base::IconType::kFavicon);
+      return;
+    } else {
+      // Ancient clients (prior to M25) may not be syncing the favicon URL. If
+      // the icon URL is not synced, use the page URL as a fake icon URL as it
+      // is guaranteed to be unique.
+      icon_url_to_use = bookmark_node->url();
+    }
+  }
+
   // The client may have cached the favicon at 2x. Use MergeFavicon() as not to
   // overwrite the cached 2x favicon bitmap. Sync favicons are always
   // gfx::kFaviconSize in width and height. Store the favicon into history
   // as such.
   gfx::Size pixel_size(gfx::kFaviconSize, gfx::kFaviconSize);
-  favicon_service->MergeFavicon(bookmark_node->url(), icon_url,
+  favicon_service->MergeFavicon(bookmark_node->url(), icon_url_to_use,
                                 favicon_base::IconType::kFavicon, bitmap_data,
                                 pixel_size);
 }
@@ -962,16 +973,21 @@ void BookmarkChangeProcessor::SetSyncNodeFavicon(
     syncer::WriteNode* sync_node) {
   scoped_refptr<base::RefCountedMemory> favicon_bytes(nullptr);
   EncodeFavicon(bookmark_node, model, &favicon_bytes);
+  sync_pb::BookmarkSpecifics updated_specifics(
+      sync_node->GetBookmarkSpecifics());
+
   if (favicon_bytes.get() && favicon_bytes->size()) {
-    sync_pb::BookmarkSpecifics updated_specifics(
-        sync_node->GetBookmarkSpecifics());
     updated_specifics.set_favicon(favicon_bytes->front(),
                                   favicon_bytes->size());
     updated_specifics.set_icon_url(bookmark_node->icon_url()
                                        ? bookmark_node->icon_url()->spec()
                                        : std::string());
-    sync_node->SetBookmarkSpecifics(updated_specifics);
+  } else {
+    updated_specifics.clear_favicon();
+    updated_specifics.clear_icon_url();
   }
+
+  sync_node->SetBookmarkSpecifics(updated_specifics);
 }
 
 bool BookmarkChangeProcessor::CanSyncNode(const BookmarkNode* node) {

components/sync_bookmarks/bookmark_change_processor.h

@@ -119,10 +119,9 @@ class BookmarkChangeProcessor : public bookmarks::BookmarkModelObserver,
       int index);
 
   // Sets the favicon of the given bookmark node from the given sync node.
-  // Returns whether the favicon was set in the bookmark node.
   // |profile| is the profile that contains the HistoryService and BookmarkModel
   // for the bookmark in question.
-  static bool SetBookmarkFavicon(const syncer::BaseNode* sync_node,
+  static void SetBookmarkFavicon(const syncer::BaseNode* sync_node,
                                  const bookmarks::BookmarkNode* bookmark_node,
                                  bookmarks::BookmarkModel* model,
                                  syncer::SyncClient* sync_client);