Fix favicon loads leading to sync resurrecting bookmarks

As of today, BookmarkModelObserver does not distinguish two very
different events:
1. A favicon has actually changed for a bookmark.
2. A favicon for a bookmark has been loaded.

They both get reported identically and in sync's case it needs to know
whether a bookmark should be reuploaded (committed) or not. Treating all
favicon-just-loaded events as local changes, as prior to this patch, is
prone to undesired uploads that can lead to resurrected bookmarks and
other sync conflicts.

Before this patch, sync could identify some trivial cases, based on the
fact that BookmarkSpecifics (hashed) hasn't changed at all. In such
cases, BookmarkModelObserverImpl::BookmarkNodeFaviconChanged() didn't
make a distinction, but the commit was later optimized away and never
made it to the protocol.

This mechanism is notably fragile in case BookmarkSpecifics go through
code changes, such as new proto fields being introduced (unrelated to
favicons).

With this patch, the underlying design flaw is addressed and instead
the two scenarios (favicon-just-loaded vs actual favicon change) are
distinguished more reliably. A new hash is introduced in per-bookmark
sync metadata that represents the last known content of the image,
which allows determining whether the image has changed and a commit
is needed.

Change-Id: Ib88bed201b3f95be083026073a3c9422ab403b2b
Bug: 1064945
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2141985Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#757861}

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

@@ -1227,6 +1227,18 @@ bool BookmarksTitleChecker::IsExitConditionSatisfied(std::ostream* os) {
   return expected_count_ == actual_count;
 }
 
+BookmarkFaviconLoadedChecker::BookmarkFaviconLoadedChecker(int profile_index,
+                                                           const GURL& page_url)
+    : SingleBookmarkModelStatusChangeChecker(profile_index),
+      bookmark_node_(GetUniqueNodeByURL(profile_index, page_url)) {
+  DCHECK_NE(nullptr, bookmark_node_);
+}
+
+bool BookmarkFaviconLoadedChecker::IsExitConditionSatisfied(std::ostream* os) {
+  *os << "Waiting for the favicon to be loaded for " << bookmark_node_->url();
+  return bookmark_node_->is_favicon_loaded();
+}
+
 ServerBookmarksEqualityChecker::ServerBookmarksEqualityChecker(
     syncer::ProfileSyncService* service,
     fake_server::FakeServer* fake_server,

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

@@ -6,7 +6,9 @@
 #define CHROME_BROWSER_SYNC_TEST_INTEGRATION_BOOKMARKS_HELPER_H_
 
 #include <memory>
+#include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include "base/callback_forward.h"
@@ -420,6 +422,22 @@ class BookmarksTitleChecker : public SingleBookmarkModelStatusChangeChecker {
   const int expected_count_;
 };
 
+// Checker used to wait until the favicon of a bookmark has been loaded. It
+// doesn't itself trigger the load of the favicon.
+class BookmarkFaviconLoadedChecker
+    : public SingleBookmarkModelStatusChangeChecker {
+ public:
+  // There must be exactly one bookmark for |page_url| in the BookmarkModel in
+  // |profile_index|.
+  BookmarkFaviconLoadedChecker(int profile_index, const GURL& page_url);
+
+  // StatusChangeChecker implementation.
+  bool IsExitConditionSatisfied(std::ostream* os) override;
+
+ private:
+  const bookmarks::BookmarkNode* const bookmark_node_;
+};
+
 // Checker used to block until the bookmarks on the server match a given set of
 // expected bookmarks.
 class ServerBookmarksEqualityChecker : public SingleClientStatusChangeChecker {

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

@@ -37,6 +37,7 @@ using bookmarks::BookmarkNode;
 using bookmarks::UrlAndTitle;
 using bookmarks_helper::AddFolder;
 using bookmarks_helper::AddURL;
+using bookmarks_helper::BookmarkFaviconLoadedChecker;
 using bookmarks_helper::BookmarksGUIDChecker;
 using bookmarks_helper::BookmarksMatchVerifierChecker;
 using bookmarks_helper::BookmarksTitleChecker;
@@ -51,6 +52,7 @@ using bookmarks_helper::CreateFavicon;
 using bookmarks_helper::GetBookmarkBarNode;
 using bookmarks_helper::GetBookmarkModel;
 using bookmarks_helper::GetOtherNode;
+using bookmarks_helper::GetUniqueNodeByURL;
 using bookmarks_helper::ModelMatchesVerifier;
 using bookmarks_helper::Move;
 using bookmarks_helper::Remove;
@@ -1137,6 +1139,61 @@ IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTest,
   EXPECT_EQ(1u, GetBookmarkBarNode(kSingleProfileIndex)->children().size());
 }
 
+IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTest,
+                       PRE_ShouldNotReploadUponFaviconLoad) {
+  const GURL url = GURL("http://www.foo.com");
+  fake_server::EntityBuilderFactory entity_builder_factory;
+  fake_server::BookmarkEntityBuilder bookmark_builder =
+      entity_builder_factory.NewBookmarkEntityBuilder("Foo Title");
+
+  // Create a legacy bookmark on the server (no GUID field populated). The fact
+  // that it's a legacy bookmark means any locally-produced specifics would be
+  // different for this bookmark (new fields like GUID would be populated).
+  std::unique_ptr<syncer::LoopbackServerEntity> bookmark =
+      bookmark_builder.BuildBookmark(url, /*is_legacy=*/true);
+  ASSERT_TRUE(bookmark.get()->GetSpecifics().bookmark().guid().empty());
+  fake_server_->InjectEntity(std::move(bookmark));
+
+  // Start syncing.
+  DisableVerifier();
+  ASSERT_TRUE(SetupSync());
+  ASSERT_TRUE(BookmarksUrlChecker(kSingleProfileIndex, url, 1).Wait());
+}
+
+IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTest,
+                       ShouldNotReploadUponFaviconLoad) {
+  const GURL url = GURL("http://www.foo.com");
+
+  base::HistogramTester histogram_tester;
+  DisableVerifier();
+  ASSERT_TRUE(SetupClients());
+#if defined(OS_CHROMEOS)
+  // signin::SetRefreshTokenForPrimaryAccount() is needed on ChromeOS in order
+  // to get a non-empty refresh token on startup.
+  GetClient(0)->SignInPrimaryAccount();
+#endif  // defined(OS_CHROMEOS)
+  ASSERT_TRUE(GetClient(kSingleProfileIndex)->AwaitEngineInitialization());
+
+  // Make sure the favicon gets loaded.
+  const BookmarkNode* bookmark_node =
+      GetUniqueNodeByURL(kSingleProfileIndex, url);
+  ASSERT_NE(nullptr, bookmark_node);
+  GetBookmarkModel(kSingleProfileIndex)->GetFavicon(bookmark_node);
+  ASSERT_TRUE(BookmarkFaviconLoadedChecker(kSingleProfileIndex, url).Wait());
+
+  // Make sure all local commits make it to the server.
+  ASSERT_TRUE(
+      UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
+
+  // Verify that the bookmark hasn't been uploaded (no local updates issued). No
+  // commits are expected despite the fact that the server-side bookmark is a
+  // legacy bookmark without the most recent fields (e.g. GUID), because loading
+  // favicons should not lead to commits unless the favicon itself changed.
+  EXPECT_EQ(
+      0, histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
+                                         /*LOCAL_UPDATE=*/2));
+}
+
 IN_PROC_BROWSER_TEST_P(
     SingleClientBookmarksSyncTestWithEnabledReuploadRemoteBookmarks,
     ShouldReuploadFullTitleAfterInitialMerge) {

components/sync/protocol/entity_metadata.proto

@@ -71,4 +71,9 @@ message EntityMetadata {
   // unique_position.proto for more information about its internal
   // representation.
   optional UniquePosition unique_position = 11;
+
+  // Used only for bookmarks. It's analogous to |specifics_hash| but it
+  // exclusively hashes the content of the favicon image, as represented in
+  // proto field BookmarkSpecifics.favicon, using base::PersistentHash().
+  optional fixed32 bookmark_favicon_hash = 12;
 }

components/sync_bookmarks/BUILD.gn

@@ -62,7 +62,9 @@ source_set("unit_tests") {
     "//components/prefs:test_support",
     "//components/sync:test_support",
     "//components/undo",
+    "//skia",
     "//testing/gmock",
     "//testing/gtest",
+    "//ui/gfx",
   ]
 }

components/sync_bookmarks/DEPS

@@ -7,6 +7,7 @@ include_rules = [
   "+components/prefs",
   "+components/sync",
   "+components/undo",
+  "+third_party/skia/include",
   "+ui/base",
   "+ui/gfx",
 ]

components/sync_bookmarks/bookmark_model_observer_impl.cc

@@ -15,7 +15,6 @@
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/engine/non_blocking_sync_common.h"
 #include "components/sync_bookmarks/bookmark_specifics_conversions.h"
-#include "components/sync_bookmarks/synced_bookmark_tracker.h"
 
 namespace sync_bookmarks {
 
@@ -184,25 +183,9 @@ void BookmarkModelObserverImpl::BookmarkNodeChanged(
     return;
   }
 
-  const base::Time modification_time = base::Time::Now();
   sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
       node, model, /*force_favicon_load=*/true, entity->has_final_guid());
-
-  // TODO(crbug.com/516866): The below CHECKs are added to debug some crashes.
-  // Should be removed after figuring out the reason for the crash.
-  CHECK_EQ(entity, bookmark_tracker_->GetEntityForBookmarkNode(node));
-  if (entity->MatchesSpecificsHash(specifics)) {
-    // We should push data to the server only if there is an actual change in
-    // the data. We could hit this code path without having actual changes
-    // (e.g.upon a favicon load).
-    return;
-  }
-  bookmark_tracker_->Update(entity, entity->metadata()->server_version(),
-                            modification_time,
-                            entity->metadata()->unique_position(), specifics);
-  // Mark the entity that it needs to be committed.
-  bookmark_tracker_->IncrementSequenceNumber(entity);
-  nudge_for_commit_closure_.Run();
+  ProcessUpdate(entity, specifics);
 }
 
 void BookmarkModelObserverImpl::BookmarkMetaInfoChanged(
@@ -229,7 +212,26 @@ void BookmarkModelObserverImpl::BookmarkNodeFaviconChanged(
     model->GetFavicon(node);
     return;
   }
-  BookmarkNodeChanged(model, node);
+
+  const SyncedBookmarkTracker::Entity* entity =
+      bookmark_tracker_->GetEntityForBookmarkNode(node);
+  if (!entity) {
+    // This should be practically unreachable but in theory it's possible that a
+    // favicon changes *during* the creation of a bookmark (by another
+    // observer). See analogous codepath in BookmarkNodeChanged().
+    return;
+  }
+
+  const sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
+      node, model, /*force_favicon_load=*/false, entity->has_final_guid());
+
+  if (entity->MatchesFaviconHash(specifics.bookmark().favicon())) {
+    // The favicon content didn't actually change, which means this event is
+    // almost certainly the result of favicon loading having completed.
+    return;
+  }
+
+  ProcessUpdate(entity, specifics);
 }
 
 void BookmarkModelObserverImpl::BookmarkNodeChildrenReordered(
@@ -339,6 +341,34 @@ syncer::UniquePosition BookmarkModelObserverImpl::ComputePosition(
       suffix);
 }
 
+void BookmarkModelObserverImpl::ProcessUpdate(
+    const SyncedBookmarkTracker::Entity* entity,
+    const sync_pb::EntitySpecifics& specifics) {
+  DCHECK(entity);
+
+  // Data should be committed to the server only if there is an actual change,
+  // determined here by comparing hashes.
+  if (entity->MatchesSpecificsHash(specifics)) {
+    // Specifics haven't actually changed, so the local change can be ignored.
+    //
+    // This is an opportunity to populate the favicon hash in sync metadata if
+    // it hasn't been populated yet. This is needed because the proto field that
+    // stores favicon hashes was introduced late. The fact that hashed specifics
+    // match implies that the favicon (which is part of specifics) must also
+    // match, hence the proto field can be safely populated.
+    bookmark_tracker_->PopulateFaviconHashIfUnset(
+        entity, specifics.bookmark().favicon());
+    return;
+  }
+
+  bookmark_tracker_->Update(entity, entity->metadata()->server_version(),
+                            /*modification_time=*/base::Time::Now(),
+                            entity->metadata()->unique_position(), specifics);
+  // Mark the entity that it needs to be committed.
+  bookmark_tracker_->IncrementSequenceNumber(entity);
+  nudge_for_commit_closure_.Run();
+}
+
 void BookmarkModelObserverImpl::ProcessDelete(
     const bookmarks::BookmarkNode* parent,
     const bookmarks::BookmarkNode* node) {

components/sync_bookmarks/bookmark_model_observer_impl.h

@@ -11,6 +11,7 @@
 #include "base/callback.h"
 #include "components/bookmarks/browser/bookmark_model_observer.h"
 #include "components/bookmarks/browser/bookmark_node.h"
+#include "components/sync_bookmarks/synced_bookmark_tracker.h"
 #include "url/gurl.h"
 
 namespace syncer {
@@ -19,8 +20,6 @@ class UniquePosition;
 
 namespace sync_bookmarks {
 
-class SyncedBookmarkTracker;
-
 // Class for listening to local changes in the bookmark model and updating
 // metadata in SyncedBookmarkTracker, such that ultimately the processor exposes
 // those local changes to the sync engine.
@@ -72,6 +71,12 @@ class BookmarkModelObserverImpl : public bookmarks::BookmarkModelObserver {
                                          size_t index,
                                          const std::string& sync_id);
 
+  // Process a modification of a local node and updates |bookmark_tracker_|
+  // accordingly. No-op if the commit can be optimized away, i.e. if |specifics|
+  // are identical to the previously-known specifics (in hashed form).
+  void ProcessUpdate(const SyncedBookmarkTracker::Entity* entity,
+                     const sync_pb::EntitySpecifics& specifics);
+
   // Processes the deletion of a bookmake node and updates the
   // |bookmark_tracker_| accordingly. If |node| is a bookmark, it gets marked
   // as deleted and that it requires a commit. If it's a folder, it recurses

components/sync_bookmarks/synced_bookmark_tracker.cc

@@ -9,6 +9,7 @@
 #include <unordered_map>
 
 #include "base/base64.h"
+#include "base/hash/hash.h"
 #include "base/hash/sha1.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
@@ -122,6 +123,13 @@ bool SyncedBookmarkTracker::Entity::MatchesSpecificsHash(
   return hash == metadata_->specifics_hash();
 }
 
+bool SyncedBookmarkTracker::Entity::MatchesFaviconHash(
+    const std::string& favicon_png_bytes) const {
+  DCHECK(!metadata_->is_deleted());
+  return metadata_->bookmark_favicon_hash() ==
+         base::PersistentHash(favicon_png_bytes);
+}
+
 bool SyncedBookmarkTracker::Entity::has_final_guid() const {
   return metadata_->has_client_tag_hash();
 }
@@ -139,6 +147,15 @@ void SyncedBookmarkTracker::Entity::set_final_guid(const std::string& guid) {
       syncer::ClientTagHash::FromUnhashed(syncer::BOOKMARKS, guid).value());
 }
 
+void SyncedBookmarkTracker::Entity::PopulateFaviconHashIfUnset(
+    const std::string& favicon_png_bytes) {
+  if (metadata_->has_bookmark_favicon_hash()) {
+    return;
+  }
+
+  metadata_->set_bookmark_favicon_hash(base::PersistentHash(favicon_png_bytes));
+}
+
 size_t SyncedBookmarkTracker::Entity::EstimateMemoryUsage() const {
   using base::trace_event::EstimateMemoryUsage;
   size_t memory_usage = 0;
@@ -241,6 +258,8 @@ const SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::Add(
                                     syncer::BOOKMARKS, bookmark_node->guid())
                                     .value());
   HashSpecifics(specifics, metadata->mutable_specifics_hash());
+  metadata->set_bookmark_favicon_hash(
+      base::PersistentHash(specifics.bookmark().favicon()));
   auto entity = std::make_unique<Entity>(bookmark_node, std::move(metadata));
   CHECK_EQ(0U, bookmark_node_to_entities_map_.count(bookmark_node));
   bookmark_node_to_entities_map_[bookmark_node] = entity.get();
@@ -268,6 +287,8 @@ void SyncedBookmarkTracker::Update(
   *mutable_entity->metadata()->mutable_unique_position() = unique_position;
   HashSpecifics(specifics,
                 mutable_entity->metadata()->mutable_specifics_hash());
+  mutable_entity->metadata()->set_bookmark_favicon_hash(
+      base::PersistentHash(specifics.bookmark().favicon()));
   // TODO(crbug.com/516866): in case of conflict, the entity might exist in
   // |ordered_local_tombstones_| as well if it has been locally deleted.
 }
@@ -284,6 +305,13 @@ void SyncedBookmarkTracker::PopulateFinalGuid(const Entity* entity,
   AsMutableEntity(entity)->set_final_guid(guid);
 }
 
+void SyncedBookmarkTracker::PopulateFaviconHashIfUnset(
+    const Entity* entity,
+    const std::string& favicon_png_bytes) {
+  DCHECK(entity);
+  AsMutableEntity(entity)->PopulateFaviconHashIfUnset(favicon_png_bytes);
+}
+
 void SyncedBookmarkTracker::MarkCommitMayHaveStarted(const Entity* entity) {
   DCHECK(entity);
   AsMutableEntity(entity)->set_commit_may_have_started(true);
@@ -297,6 +325,7 @@ void SyncedBookmarkTracker::MarkDeleted(const Entity* entity) {
 
   Entity* mutable_entity = AsMutableEntity(entity);
   mutable_entity->metadata()->set_is_deleted(true);
+  mutable_entity->metadata()->clear_bookmark_favicon_hash();
   // Clear all references to the deleted bookmark node.
   bookmark_node_to_entities_map_.erase(mutable_entity->bookmark_node());
   mutable_entity->clear_bookmark_node();
@@ -731,6 +760,10 @@ size_t SyncedBookmarkTracker::TrackedUncommittedTombstonesCountForDebugging()
   return ordered_local_tombstones_.size();
 }
 
+void SyncedBookmarkTracker::ClearSpecificsHashForTest(const Entity* entity) {
+  AsMutableEntity(entity)->metadata()->clear_specifics_hash();
+}
+
 void SyncedBookmarkTracker::CheckAllNodesTracked(
     const bookmarks::BookmarkModel* bookmark_model) const {
   // TODO(crbug.com/516866): The method is added to debug some crashes.

components/sync_bookmarks/synced_bookmark_tracker.h

@@ -56,6 +56,10 @@ class SyncedBookmarkTracker {
     // Check whether |specifics| matches the stored specifics_hash.
     bool MatchesSpecificsHash(const sync_pb::EntitySpecifics& specifics) const;
 
+    // Check whether |favicon_png_bytes| matches the stored
+    // bookmark_favicon_hash.
+    bool MatchesFaviconHash(const std::string& favicon_png_bytes) const;
+
     // Returns null for tombstones.
     const bookmarks::BookmarkNode* bookmark_node() const {
       return bookmark_node_;
@@ -105,6 +109,8 @@ class SyncedBookmarkTracker {
     // otherwise).
     void set_final_guid(const std::string& guid);
 
+    void PopulateFaviconHashIfUnset(const std::string& favicon_png_bytes);
+
     // Returns the estimate of dynamically allocated memory in bytes.
     size_t EstimateMemoryUsage() const;
 
@@ -173,6 +179,11 @@ class SyncedBookmarkTracker {
   // Populates a bookmark's final GUID. |entity| must be owned by this tracker.
   void PopulateFinalGuid(const Entity* entity, const std::string& guid);
 
+  // Populates the metadata field representing the hashed favicon. This method
+  // is effectively used to backfill the proto field, which was introduced late.
+  void PopulateFaviconHashIfUnset(const Entity* entity,
+                                  const std::string& favicon_png_bytes);
+
   // Marks an existing entry that a commit request might have been sent to the
   // server. |entity| must be owned by this tracker.
   void MarkCommitMayHaveStarted(const Entity* entity);
@@ -250,6 +261,9 @@ class SyncedBookmarkTracker {
   // confirmed the deletion yet.
   size_t TrackedUncommittedTombstonesCountForDebugging() const;
 
+  // Clears the specifics hash for |entity|, useful for testing.
+  void ClearSpecificsHashForTest(const Entity* entity);
+
   // Checks whther all nodes in |bookmark_model| that *should* be tracked as per
   // CanSyncNode() are tracked.
   void CheckAllNodesTracked(

components/sync_bookmarks/synced_bookmark_tracker_unittest.cc

@@ -765,6 +765,123 @@ TEST(SyncedBookmarkTrackerTest,
       /*sample=*/ExpectedCorruptionReason::NO_CORRUPTION, /*count=*/1);
 }
 
+TEST(SyncedBookmarkTrackerTest,
+     ShouldPopulateFaviconHashForNewlyAddedEntities) {
+  std::unique_ptr<SyncedBookmarkTracker> tracker =
+      SyncedBookmarkTracker::CreateEmpty(sync_pb::ModelTypeState());
+
+  const std::string kSyncId = "SYNC_ID";
+  const std::string kTitle = "Title";
+  const GURL kUrl("http://www.foo.com");
+  const int64_t kId = 1;
+  const int64_t kServerVersion = 1000;
+  const base::Time kCreationTime = base::Time::Now();
+  const syncer::UniquePosition kUniquePosition =
+      syncer::UniquePosition::InitialPosition(
+          syncer::UniquePosition::RandomSuffix());
+  const std::string kFaviconPngBytes = "fakefaviconbytes";
+
+  sync_pb::EntitySpecifics specifics = GenerateSpecifics(kTitle, kUrl.spec());
+  specifics.mutable_bookmark()->set_favicon(kFaviconPngBytes);
+
+  bookmarks::BookmarkNode node(kId, base::GenerateGUID(), kUrl);
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker->Add(&node, kSyncId, kServerVersion, kCreationTime,
+                   kUniquePosition.ToProto(), specifics);
+
+  EXPECT_TRUE(entity->metadata()->has_bookmark_favicon_hash());
+  EXPECT_TRUE(entity->MatchesFaviconHash(kFaviconPngBytes));
+  EXPECT_FALSE(entity->MatchesFaviconHash("otherhash"));
+}
+
+TEST(SyncedBookmarkTrackerTest, ShouldPopulateFaviconHashUponUpdate) {
+  const std::string kSyncId = "SYNC_ID";
+  const std::string kTitle = "Title";
+  const GURL kUrl("http://www.foo.com");
+  const int64_t kServerVersion = 1000;
+  const base::Time kModificationTime = base::Time::Now();
+  const syncer::UniquePosition kUniquePosition =
+      syncer::UniquePosition::InitialPosition(
+          syncer::UniquePosition::RandomSuffix());
+  const std::string kFaviconPngBytes = "fakefaviconbytes";
+
+  std::unique_ptr<bookmarks::BookmarkModel> model =
+      bookmarks::TestBookmarkClient::CreateModel();
+
+  const bookmarks::BookmarkNode* bookmark_bar_node = model->bookmark_bar_node();
+  const bookmarks::BookmarkNode* node =
+      model->AddURL(/*parent=*/bookmark_bar_node, /*index=*/0,
+                    base::ASCIIToUTF16("Title"), GURL("http://www.url.com"));
+
+  sync_pb::BookmarkModelMetadata model_metadata =
+      CreateMetadataForPermanentNodes(model.get());
+
+  // Add entry for the URL node.
+  *model_metadata.add_bookmarks_metadata() =
+      CreateNodeMetadata(node->id(), kSyncId);
+
+  std::unique_ptr<SyncedBookmarkTracker> tracker =
+      SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata(
+          model.get(), std::move(model_metadata));
+  ASSERT_THAT(tracker, NotNull());
+
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker->GetEntityForSyncId(kSyncId);
+  ASSERT_THAT(entity, NotNull());
+  ASSERT_FALSE(entity->metadata()->has_bookmark_favicon_hash());
+  ASSERT_FALSE(entity->MatchesFaviconHash(kFaviconPngBytes));
+
+  sync_pb::EntitySpecifics specifics = GenerateSpecifics(kTitle, kUrl.spec());
+  specifics.mutable_bookmark()->set_favicon(kFaviconPngBytes);
+
+  tracker->Update(entity, kServerVersion, kModificationTime,
+                  kUniquePosition.ToProto(), specifics);
+
+  EXPECT_TRUE(entity->metadata()->has_bookmark_favicon_hash());
+  EXPECT_TRUE(entity->MatchesFaviconHash(kFaviconPngBytes));
+  EXPECT_FALSE(entity->MatchesFaviconHash("otherhash"));
+}
+
+TEST(SyncedBookmarkTrackerTest, ShouldPopulateFaviconHashExplicitly) {
+  const std::string kSyncId = "SYNC_ID";
+  const std::string kFaviconPngBytes = "fakefaviconbytes";
+
+  std::unique_ptr<bookmarks::BookmarkModel> model =
+      bookmarks::TestBookmarkClient::CreateModel();
+
+  const bookmarks::BookmarkNode* bookmark_bar_node = model->bookmark_bar_node();
+  const bookmarks::BookmarkNode* node =
+      model->AddURL(/*parent=*/bookmark_bar_node, /*index=*/0,
+                    base::ASCIIToUTF16("Title"), GURL("http://www.url.com"));
+
+  sync_pb::BookmarkModelMetadata model_metadata =
+      CreateMetadataForPermanentNodes(model.get());
+
+  // Add entry for the URL node.
+  *model_metadata.add_bookmarks_metadata() =
+      CreateNodeMetadata(node->id(), kSyncId);
+
+  std::unique_ptr<SyncedBookmarkTracker> tracker =
+      SyncedBookmarkTracker::CreateFromBookmarkModelAndMetadata(
+          model.get(), std::move(model_metadata));
+  ASSERT_THAT(tracker, NotNull());
+
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker->GetEntityForSyncId(kSyncId);
+  ASSERT_THAT(entity, NotNull());
+  ASSERT_FALSE(entity->metadata()->has_bookmark_favicon_hash());
+  ASSERT_FALSE(entity->MatchesFaviconHash(kFaviconPngBytes));
+
+  tracker->PopulateFaviconHashIfUnset(entity, kFaviconPngBytes);
+  EXPECT_TRUE(entity->metadata()->has_bookmark_favicon_hash());
+  EXPECT_TRUE(entity->MatchesFaviconHash(kFaviconPngBytes));
+  EXPECT_FALSE(entity->MatchesFaviconHash("otherhash"));
+
+  // Further calls should be ignored.
+  tracker->PopulateFaviconHashIfUnset(entity, "otherpngbytes");
+  EXPECT_TRUE(entity->MatchesFaviconHash(kFaviconPngBytes));
+}
+
 }  // namespace
 
 }  // namespace sync_bookmarks