[Sync::USS] Fix Bookmarks PersistProgressMarkerOnRestart test for USS

The test PersistProgressMarkerOnRestart used to fail for USS Bookmarks
because the metadata weren't persisted properly.

This CL makes sure that the processor would outlive the bookmark model
such that the metadata is available at the time when the model is
getting destructed and persisted on disk.

Bug: 816866
Change-Id: Ief259bd75ee66a6f9bb1dd70fc2ef6a446ebbd7a
Reviewed-on: https://chromium-review.googlesource.com/c/1349314
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611703}

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

@@ -579,12 +579,9 @@ IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTest,
   // We use the following checker to simply wait for an non-empty snapshot.
   EXPECT_TRUE(UpdatedProgressMarkerChecker(GetSyncService(0)).Wait());
 
-  // TODO(mamir): The expectation below should pass but doesn't with USS.
-  // Investigate the cause and fix the underlying issue.
-  // EXPECT_EQ(
-  //    0,
-  //    histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
-  //                                       /*REMOTE_INITIAL_UPDATE=*/5));
+  EXPECT_EQ(
+      0, histogram_tester.GetBucketCount("Sync.ModelTypeEntityChange3.BOOKMARK",
+                                         /*REMOTE_INITIAL_UPDATE=*/5));
 }
 
 INSTANTIATE_TEST_CASE_P(USS,

components/sync_bookmarks/bookmark_model_observer_impl.cc

@@ -20,9 +20,12 @@ namespace sync_bookmarks {
 
 BookmarkModelObserverImpl::BookmarkModelObserverImpl(
     const base::RepeatingClosure& nudge_for_commit_closure,
+    base::OnceClosure on_bookmark_model_being_deleted_closure,
     SyncedBookmarkTracker* bookmark_tracker)
     : bookmark_tracker_(bookmark_tracker),
-      nudge_for_commit_closure_(nudge_for_commit_closure) {
+      nudge_for_commit_closure_(nudge_for_commit_closure),
+      on_bookmark_model_being_deleted_closure_(
+          std::move(on_bookmark_model_being_deleted_closure)) {
   DCHECK(bookmark_tracker_);
 }
 
@@ -36,7 +39,7 @@ void BookmarkModelObserverImpl::BookmarkModelLoaded(
 
 void BookmarkModelObserverImpl::BookmarkModelBeingDeleted(
     bookmarks::BookmarkModel* model) {
-  // This class isn't responsible for any deletion-related logic.
+  std::move(on_bookmark_model_being_deleted_closure_).Run();
 }
 
 void BookmarkModelObserverImpl::BookmarkNodeMoved(
@@ -87,7 +90,7 @@ void BookmarkModelObserverImpl::BookmarkNodeAdded(
     DLOG(WARNING) << "Bookmark parent lookup failed";
     return;
   }
-  // Similar to the diectory implementation here:
+  // Similar to the directory implementation here:
   // https://cs.chromium.org/chromium/src/components/sync/syncable/mutable_entry.cc?l=237&gsn=CreateEntryKernel
   // Assign a temp server id for the entity. Will be overriden by the actual
   // server id upon receiving commit response.

components/sync_bookmarks/bookmark_model_observer_impl.h

@@ -29,6 +29,7 @@ class BookmarkModelObserverImpl : public bookmarks::BookmarkModelObserver {
   // |bookmark_tracker_| must not be null and must outlive this object.
   BookmarkModelObserverImpl(
       const base::RepeatingClosure& nudge_for_commit_closure,
+      base::OnceClosure on_bookmark_model_being_deleted_closure,
       SyncedBookmarkTracker* bookmark_tracker);
   ~BookmarkModelObserverImpl() override;
 
@@ -86,6 +87,10 @@ class BookmarkModelObserverImpl : public bookmarks::BookmarkModelObserver {
   // be committed.
   const base::RepeatingClosure nudge_for_commit_closure_;
 
+  // The callback used to inform the processor that the bookmark is getting
+  // deleted.
+  base::OnceClosure on_bookmark_model_being_deleted_closure_;
+
   DISALLOW_COPY_AND_ASSIGN(BookmarkModelObserverImpl);
 };
 

components/sync_bookmarks/bookmark_model_observer_impl_unittest.cc

@@ -36,10 +36,12 @@ const size_t kMaxEntries = 1000;
 class BookmarkModelObserverImplTest : public testing::Test {
  public:
   BookmarkModelObserverImplTest()
-      : bookmark_model_(bookmarks::TestBookmarkClient::CreateModel()),
-        bookmark_tracker_(std::vector<NodeMetadataPair>(),
+      : bookmark_tracker_(std::vector<NodeMetadataPair>(),
                           std::make_unique<sync_pb::ModelTypeState>()),
-        observer_(nudge_for_commit_closure_.Get(), &bookmark_tracker_) {
+        observer_(nudge_for_commit_closure_.Get(),
+                  /*on_bookmark_model_being_deleted_closure=*/base::DoNothing(),
+                  &bookmark_tracker_),
+        bookmark_model_(bookmarks::TestBookmarkClient::CreateModel()) {
     bookmark_model_->AddObserver(&observer_);
     sync_pb::EntitySpecifics specifics;
     specifics.mutable_bookmark()->set_title(kBookmarkBarTag);
@@ -53,6 +55,10 @@ class BookmarkModelObserverImplTest : public testing::Test {
         specifics);
   }
 
+  ~BookmarkModelObserverImplTest() {
+    bookmark_model_->RemoveObserver(&observer_);
+  }
+
   void SimulateCommitResponseForAllLocalChanges() {
     for (const SyncedBookmarkTracker::Entity* entity :
          bookmark_tracker()->GetEntitiesWithLocalChanges(kMaxEntries)) {
@@ -80,11 +86,11 @@ class BookmarkModelObserverImplTest : public testing::Test {
   }
 
  private:
-  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model_;
   NiceMock<base::MockCallback<base::RepeatingClosure>>
       nudge_for_commit_closure_;
   SyncedBookmarkTracker bookmark_tracker_;
   BookmarkModelObserverImpl observer_;
+  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model_;
 };
 
 TEST_F(BookmarkModelObserverImplTest,
@@ -475,13 +481,17 @@ TEST_F(BookmarkModelObserverImplTest, ShouldNotSyncUnsyncableBookmarks) {
   model->Remove(unsyncable_node);
   // Only bookmark bar should be tracked.
   EXPECT_THAT(bookmark_tracker()->TrackedEntitiesCountForTest(), 1U);
+  model->RemoveObserver(observer());
 }
 
 TEST_F(BookmarkModelObserverImplTest, ShouldAddChildrenInArbitraryOrder) {
   SyncedBookmarkTracker bookmark_tracker(
       std::vector<NodeMetadataPair>(),
       std::make_unique<sync_pb::ModelTypeState>());
-  BookmarkModelObserverImpl observer(base::DoNothing(), &bookmark_tracker);
+  BookmarkModelObserverImpl observer(
+      /*nudge_for_commit_closure=*/base::DoNothing(),
+      /*on_bookmark_model_being_deleted_closure=*/base::DoNothing(),
+      &bookmark_tracker);
   const bookmarks::BookmarkNode* bookmark_bar_node =
       bookmark_model()->bookmark_bar_node();
   // Add the bookmark bar to the tracker.
@@ -528,6 +538,23 @@ TEST_F(BookmarkModelObserverImplTest, ShouldAddChildrenInArbitraryOrder) {
   EXPECT_TRUE(PositionOf(nodes[3]).LessThan(PositionOf(nodes[4])));
 }
 
+TEST_F(BookmarkModelObserverImplTest,
+       ShouldCallOnBookmarkModelBeingDeletedClosure) {
+  SyncedBookmarkTracker bookmark_tracker(
+      std::vector<NodeMetadataPair>(),
+      std::make_unique<sync_pb::ModelTypeState>());
+
+  NiceMock<base::MockCallback<base::OnceClosure>>
+      on_bookmark_model_being_deleted_closure_mock;
+
+  BookmarkModelObserverImpl observer(
+      /*nudge_for_commit_closure=*/base::DoNothing(),
+      on_bookmark_model_being_deleted_closure_mock.Get(), &bookmark_tracker);
+
+  EXPECT_CALL(on_bookmark_model_being_deleted_closure_mock, Run());
+  observer.BookmarkModelBeingDeleted(/*model=*/nullptr);
+}
+
 }  // namespace
 
 }  // namespace sync_bookmarks

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -372,6 +372,15 @@ void BookmarkModelTypeProcessor::NudgeForCommitIfNeeded() {
   }
 }
 
+void BookmarkModelTypeProcessor::OnBookmarkModelBeingDeleted() {
+  DCHECK(bookmark_model_);
+  DCHECK(bookmark_model_observer_);
+  bookmark_model_->RemoveObserver(bookmark_model_observer_.get());
+  bookmark_model_ = nullptr;
+  bookmark_model_observer_.reset();
+  DisconnectSync();
+}
+
 void BookmarkModelTypeProcessor::StartTrackingMetadata(
     std::vector<NodeMetadataPair> nodes_metadata,
     std::unique_ptr<sync_pb::ModelTypeState> model_type_state) {
@@ -381,6 +390,8 @@ void BookmarkModelTypeProcessor::StartTrackingMetadata(
   bookmark_model_observer_ = std::make_unique<BookmarkModelObserverImpl>(
       base::BindRepeating(&BookmarkModelTypeProcessor::NudgeForCommitIfNeeded,
                           base::Unretained(this)),
+      base::BindOnce(&BookmarkModelTypeProcessor::OnBookmarkModelBeingDeleted,
+                     base::Unretained(this)),
       bookmark_tracker_.get());
   bookmark_model_->AddObserver(bookmark_model_observer_.get());
 }

components/sync_bookmarks/bookmark_model_type_processor.h

@@ -97,6 +97,9 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
   // entities.
   void NudgeForCommitIfNeeded();
 
+  // Performs the required clean up when bookmark model is being deleted.
+  void OnBookmarkModelBeingDeleted();
+
   // Instantiates the required objects to track metadata and starts observing
   // changes from the bookmark model.
   void StartTrackingMetadata(

components/sync_bookmarks/bookmark_sync_service.cc

@@ -22,10 +22,6 @@ BookmarkSyncService::BookmarkSyncService(
 
 BookmarkSyncService::~BookmarkSyncService() {}
 
-void BookmarkSyncService::Shutdown() {
-  bookmark_model_type_processor_.reset();
-}
-
 std::string BookmarkSyncService::EncodeBookmarkSyncMetadata() {
   if (!bookmark_model_type_processor_) {
     return std::string();

components/sync_bookmarks/bookmark_sync_service.h

@@ -37,7 +37,6 @@ class BookmarkSyncService : public KeyedService {
 
   // KeyedService implemenation.
   ~BookmarkSyncService() override;
-  void Shutdown() override;
 
   // Analgous to Encode/Decode methods in BookmarkClient.
   std::string EncodeBookmarkSyncMetadata();