[Sync] Patch a crash in AutofillWalletSyncableService

This is a speculative patch for a bug causing a crash in
AutofillWalletSyncableService. This CL stops the generic change
processor from talking to the syncable service when the service gets
stopped.

GenericChangeProcessor only talks to the service because it gets pinged
by SyncBackendRegistrar. The registrar should not talk to the
GenericChangeProcessor after a data type gets deactivated. Normally, a
data type should get deactivated before it gets stopped. However, we
see crashes in Canary that seem to contradict this expected behavior.

Bug: 880029
Change-Id: I64b9ea1c3521a347983a530bc16c1f8e266769b6
Reviewed-on: https://chromium-review.googlesource.com/1230714
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592485}

components/sync/driver/generic_change_processor.cc

@@ -571,6 +571,10 @@ bool GenericChangeProcessor::CryptoReadyIfNecessary() {
   return !encrypted_types.Has(type_) || trans.GetCryptographer()->is_ready();
 }
 
+void GenericChangeProcessor::Disconnect() {
+  local_service_.reset();
+}
+
 void GenericChangeProcessor::StartImpl() {}
 
 UserShare* GenericChangeProcessor::share_handle() const {

components/sync/driver/generic_change_processor.h

@@ -92,6 +92,12 @@ class GenericChangeProcessor : public ChangeProcessor,
   virtual bool SyncModelHasUserCreatedNodes(bool* has_nodes);
   virtual bool CryptoReadyIfNecessary();
 
+  // Disconnect from the syncable service. After a call to this function, no
+  // further calls to the syncable service are placed by the processor.
+  // TODO(crbug.com/880029): This is a speculative fix for the bug. Remove if it
+  // does not work or if a deeper root cause is found.
+  void Disconnect();
+
  protected:
   // ChangeProcessor interface.
   void StartImpl() override;  // Does nothing.
@@ -125,7 +131,7 @@ class GenericChangeProcessor : public ChangeProcessor,
   const ModelType type_;
 
   // The SyncableService this change processor will forward changes on to.
-  const base::WeakPtr<SyncableService> local_service_;
+  base::WeakPtr<SyncableService> local_service_;
 
   // A SyncMergeResult used to track the changes made during association. The
   // owner will invalidate the weak pointer when association is complete. While

components/sync/driver/shared_change_processor.cc

@@ -310,8 +310,17 @@ void SharedChangeProcessor::RecordAssociationTime(base::TimeDelta time) {
 }
 
 void SharedChangeProcessor::StopLocalService() {
-  if (local_service_)
+  if (local_service_) {
     local_service_->StopSyncing(type_);
+
+    // Also make the generic processor stop talking to the server (if not
+    // nullptr which is the case in some unit-tests).
+    // TODO(crbug.com/880029): This is a speculative fix for the bug. Remove if
+    // it does not work or if the real root cause is found.
+    if (generic_change_processor_) {
+      generic_change_processor_->Disconnect();
+    }
+  }
   local_service_.reset();
 }
 

components/sync/driver/shared_change_processor.h

@@ -114,6 +114,9 @@ class SharedChangeProcessor
                                          const std::string& message);
 
   // Calls local_service_->StopSyncing() and releases our reference to it.
+  // TODO(crbug.com/880029): As a speculative fix for the bug, this call also
+  // tells the generic change processor to stop talking to the syncable service.
+  // Remove if it does not work or if a deeper root cause is found.
   void StopLocalService();
 
   ChangeProcessor* generic_change_processor();