[Sync:USS] Fix BookmarkModelTypeProcessor::OnSyncStopping

Before this CL:
OnSyncStopping assumed that the process is already tracking the metadata
which isn't necessarily true. The only guarantees is that the model has
been loaded.

After this CL: we relax the guarantees in OnSyncStopping and it requires
only the model to be loaded

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

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -340,11 +340,10 @@ void BookmarkModelTypeProcessor::ConnectIfReady() {
 void BookmarkModelTypeProcessor::OnSyncStopping(
     syncer::SyncStopMetadataFate metadata_fate) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  // Disabling sync for a type shouldn't happen before the model is ready to
-  // sync and metadata are tracked.
-  DCHECK(bookmark_tracker_);
+  // Disabling sync for a type shouldn't happen before the model is loaded
+  // because OnSyncStopping() is not allowed to be called before
+  // OnSyncStarting() has completed..
   DCHECK(bookmark_model_);
-  DCHECK(bookmark_model_observer_);
   DCHECK(!start_callback_);
 
   cache_guid_.clear();
@@ -358,9 +357,12 @@ void BookmarkModelTypeProcessor::OnSyncStopping(
     case syncer::CLEAR_METADATA: {
       // Stop observing local changes. We'll start observing local changes again
       // when Sync is (re)started in StartTrackingMetadata().
-      bookmark_model_->RemoveObserver(bookmark_model_observer_.get());
-      bookmark_model_observer_.reset();
-      bookmark_tracker_.reset();
+      if (bookmark_tracker_) {
+        DCHECK(bookmark_model_observer_);
+        bookmark_model_->RemoveObserver(bookmark_model_observer_.get());
+        bookmark_model_observer_.reset();
+        bookmark_tracker_.reset();
+      }
       schedule_save_closure_.Run();
       break;
     }

components/sync_bookmarks/bookmark_model_type_processor_unittest.cc

@@ -21,6 +21,7 @@
 
 using base::ASCIIToUTF16;
 using testing::Eq;
+using testing::IsNull;
 using testing::NiceMock;
 using testing::NotNull;
 
@@ -358,6 +359,22 @@ TEST_F(BookmarkModelTypeProcessorTest,
               Eq(kEncryptionKeyName));
 }
 
+// Verifies that the processor doesn't crash if sync is stopped before receiving
+// remote updates or tracking metadata.
+TEST_F(BookmarkModelTypeProcessorTest, ShouldStopBeforeReceivingRemoteUpdates) {
+  ASSERT_THAT(processor()->GetTrackerForTest(), IsNull());
+  processor()->OnSyncStopping(syncer::CLEAR_METADATA);
+  EXPECT_THAT(processor()->GetTrackerForTest(), IsNull());
+}
+
+TEST_F(BookmarkModelTypeProcessorTest, ShouldStopAfterReceivingRemoteUpdates) {
+  // Initialize the process to make sure the tracker has been created.
+  InitWithSyncedBookmarks({}, processor());
+  ASSERT_THAT(processor()->GetTrackerForTest(), NotNull());
+  processor()->OnSyncStopping(syncer::CLEAR_METADATA);
+  EXPECT_THAT(processor()->GetTrackerForTest(), IsNull());
+}
+
 }  // namespace
 
 }  // namespace sync_bookmarks