UserEventSyncBridge overrides ModelTypeSyncBridge::DisableSync() and never calls the base implementation, leading to the processor not knowing about the event. The result is that sync metadata is not deleted when sync is disabled.
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b142aad18f8bdabcd353747f4ef61861efcec860 commit b142aad18f8bdabcd353747f4ef61861efcec860 Author: Mikel Astiz <mastiz@chromium.org> Date: Wed Mar 07 14:48:15 2018 Fix assuming model is ready to sync during DisableSync() In tests (I believe the scenario cannot be reproduced otherwise), there are situations where DisableSync() is exercised before the model is ready to sync. Hence, the bridge should propagate the readiness state to the newly created processor during DisableSync(), without making the assumption that it must have been ready. The proposed implementation is not elegant, because the semantics of IsTrackingMetadata() are somewhat abused to assume it reflects model readiness (which is the case for the only production change processor). However, we will replace this is in a follow-up patch anyway, so it's best to keep the changes as small as possible. Bug: 819233 Change-Id: Idecf65ade939120fc89bace63b1844176cde9eb3 Reviewed-on: https://chromium-review.googlesource.com/951786 Reviewed-by: Jan Krcal <jkrcal@chromium.org> Commit-Queue: Mikel Astiz <mastiz@chromium.org> Cr-Commit-Position: refs/heads/master@{#541431} [modify] https://crrev.com/b142aad18f8bdabcd353747f4ef61861efcec860/components/sync/model/model_type_sync_bridge.cc [modify] https://crrev.com/b142aad18f8bdabcd353747f4ef61861efcec860/components/sync/model/model_type_sync_bridge_unittest.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3266122e390fb6037fe0a2158c3244602894197a commit 3266122e390fb6037fe0a2158c3244602894197a Author: Mikel Astiz <mastiz@chromium.org> Date: Thu Mar 08 15:55:05 2018 Fix UserEventSyncBridge not calling processor's DisableSync The call is implemented in the base class's implementation, ModelTypeSyncBridge::DisableSync(). Without such a call, the processor never takes care of deleting the metadata. Because UserEventSyncBridge is a commit-only type (in fact the only one), it forces initial_sync_done when loading metadata. This is now moved to the processor, because it's common for all commit-only types and it also needs to be taken care of if DisableSync() is followed by enable-sync. In order to test this, some refactoring of tests was needed (and I chose to introduce MockModelTypeChangeProcessor), because: a) The former tests had bugs (didn't actually verify some values, because callbacks not being run was treated as success) b) The DisableSync() flow (currently) destroys and recreates the change processor, which requires some forwarding proxy to allow tests to verify the state across destructions of the processor. Bug: 819233 Change-Id: I0f1524850ef65b795b7f7082ce4e85171b8e2898 Reviewed-on: https://chromium-review.googlesource.com/951604 Commit-Queue: Mikel Astiz <mastiz@chromium.org> Reviewed-by: Jan Krcal <jkrcal@chromium.org> Cr-Commit-Position: refs/heads/master@{#541805} [modify] https://crrev.com/3266122e390fb6037fe0a2158c3244602894197a/chrome/browser/sync/test/integration/single_client_user_events_sync_test.cc [modify] https://crrev.com/3266122e390fb6037fe0a2158c3244602894197a/components/sync/BUILD.gn [add] https://crrev.com/3266122e390fb6037fe0a2158c3244602894197a/components/sync/model/mock_model_type_change_processor.cc [add] https://crrev.com/3266122e390fb6037fe0a2158c3244602894197a/components/sync/model/mock_model_type_change_processor.h [modify] https://crrev.com/3266122e390fb6037fe0a2158c3244602894197a/components/sync/model/model_type_sync_bridge_unittest.cc [modify] https://crrev.com/3266122e390fb6037fe0a2158c3244602894197a/components/sync/model_impl/client_tag_based_model_type_processor.cc [modify] https://crrev.com/3266122e390fb6037fe0a2158c3244602894197a/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc [modify] https://crrev.com/3266122e390fb6037fe0a2158c3244602894197a/components/sync/user_events/user_event_sync_bridge.cc [modify] https://crrev.com/3266122e390fb6037fe0a2158c3244602894197a/components/sync/user_events/user_event_sync_bridge_unittest.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c38383dc5c8277199514abb4d1d30f897989a582 commit c38383dc5c8277199514abb4d1d30f897989a582 Author: Mikel Astiz <mastiz@chromium.org> Date: Mon Mar 12 11:33:32 2018 Improve deletion of [meta]data during DisableSync We introduce a dedicated method in ModelTypeStore to avoid round trips across threads when a datatype's desire is to delete it all (data and metadata). Benefits include: - Simpler code: bridge's don't need to read+delete. - (Hopefully) more robust against crashes during DisableSync(). - Less error-prone: we've had examples in the past where overriding DisableSync() forgot to call the base class's implementation, leading to metadata staying around. Bug: 819233 , 820049 Change-Id: If6b7b04ee73b8c6a28aef7380a7eb91b866f1567 Reviewed-on: https://chromium-review.googlesource.com/955624 Commit-Queue: Mikel Astiz <mastiz@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#542461} [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/device_info/device_info_sync_bridge.cc [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/device_info/device_info_sync_bridge.h [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/device_info/device_info_sync_bridge_unittest.cc [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model/model_type_store.h [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model/model_type_sync_bridge.cc [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model/model_type_sync_bridge.h [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/client_tag_based_model_type_processor.cc [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/model_type_store_backend.cc [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/model_type_store_backend.h [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/model_type_store_backend_unittest.cc [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/model_type_store_impl.cc [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/model_type_store_impl.h [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/model_impl/model_type_store_impl_unittest.cc [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/user_events/user_event_sync_bridge.cc [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/user_events/user_event_sync_bridge.h [modify] https://crrev.com/c38383dc5c8277199514abb4d1d30f897989a582/components/sync/user_events/user_event_sync_bridge_unittest.cc
Comment 1 by bugdroid1@chromium.org
, Mar 7 2018