New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 870624 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 7
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 2
Type: Feature
Rollout-Type: Default

Blocked on:
issue 895899

Blocking:
issue 902704



Sign in to add a comment

Introduce wrapper to mass migrate SyncableService-s to pseudo-USS

Project Member Reported by mastiz@chromium.org, Aug 3

Issue description

We'd like to wrap the legacy architecture (SyncableService) in the more modern USS architecture, in order to share the most important codepaths, and eventually get rid of the legacy directory altogether.

This will allow to:
- Unify logic across datatypes by avoiding subtle behavioral differences across architectures.
- Remove lots of code (~25K LoC), hence reduce maintenance cost and eng ramp-up time.
- Improve resource footprint (less RAM).
- Reduce the gap for datatypes to actually migrate to USS.
- Unblock a massive simplification of DataTypeManager and related classes, including controllers.
- Surface direct dependencies to Directory code outside datatypes (e.g. sync engine writting the cache GUID), which is a potential issue for ongoing efforts.

Design Doc (Googlers only): https://docs.google.com/document/d/14ScYZ0sop921gjBwXuReIEuQJlwftqkuSM1jMK_A_x4
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0ef4f8ce4c7c3306ea51f02d3983c19b02d30dd6

commit 0ef4f8ce4c7c3306ea51f02d3983c19b02d30dd6
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Sep 25 16:45:33 2018

Add support to resolve conflicts into deletions

A datatype should be allowed to resolve conflicts in a way that
deletions are preferred. None of the USS datatypes has done this until
now, but we're experimenting with EXTENSIONS being integrated, and we
do have such a policy for EXTENSIONS.

Bug:  870624 
Change-Id: Ib8869e4f31050def316d94161b7ea02a2dbd09a1
Reviewed-on: https://chromium-review.googlesource.com/1243185
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593968}
[modify] https://crrev.com/0ef4f8ce4c7c3306ea51f02d3983c19b02d30dd6/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/0ef4f8ce4c7c3306ea51f02d3983c19b02d30dd6/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/76ea70c9c868fc1b407ef10fea747971db89f294

commit 76ea70c9c868fc1b407ef10fea747971db89f294
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Sep 27 09:38:20 2018

Allow remote SyncData without valid ID

SyncableServices don't actually consume this field, the only caller
to GetId() being GenericChangeProcessor. Changing the API towards
SyncableServices would require a huge refactoring, so let's instead
allow that SyncData may not be populated, and DCHECK-fail lazily on
read (in case a SyncableService implementation is changed in the
future to consume this field).

The rationale behind is that future patches will integrate
SyncableService instantes within the USS framework, where the ID
doesn't have any semantics.

As implementation detail: because local vs remote was previously
implemented verifying id_ != kInvalidId, we need to instead
introduce a dedicated bool member.

Bug:  870624 
Change-Id: I75aa92e69a861ea6af58327a14406bc086233043
Reviewed-on: https://chromium-review.googlesource.com/1205393
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594646}
[modify] https://crrev.com/76ea70c9c868fc1b407ef10fea747971db89f294/components/sync/model/sync_data.cc
[modify] https://crrev.com/76ea70c9c868fc1b407ef10fea747971db89f294/components/sync/model/sync_data.h
[modify] https://crrev.com/76ea70c9c868fc1b407ef10fea747971db89f294/components/sync/model/sync_data_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/540d0c4c4c584db21290c81d1fbfbbf6c96456e1

commit 540d0c4c4c584db21290c81d1fbfbbf6c96456e1
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Sep 27 12:01:01 2018

Fix handling of errors while canceling start in ModelTypeController

The scenario should be rare but still worth proper handling: if a
datatype's start is being cancelled, we may still receive an error
because initialization failed. That should be treated similarly as the
regular error flow, and enter the FAILED state.

As bonus point, unit tests are added for the more conventional cases
too, although their behavior hasn't changed in this patch.

Bug:  870624 
Change-Id: I00094a4309bc40c50e088c12291e6406c8bbb5d9
Reviewed-on: https://chromium-review.googlesource.com/1248741
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594680}
[modify] https://crrev.com/540d0c4c4c584db21290c81d1fbfbbf6c96456e1/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/540d0c4c4c584db21290c81d1fbfbbf6c96456e1/components/sync/driver/model_type_controller_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed5d9496bc9e842479193ddf5425e5fa4ce56920

commit ed5d9496bc9e842479193ddf5425e5fa4ce56920
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Sep 27 13:42:24 2018

Simplify SyncableService by avoiding unnecessary base interface

There is no need to cast SyncableService instances to interface
SyncChangeProcessor, neither do these services need to implement any
functions in there, since they are not exercised.

One exception is SyncChangeProcessor::UpdateDataTypeContext(), which
is called from SharedChangeProcessor, but there is no SyncableService
that implements any functionality in such function.

In order to keep the patch less intrusive, GetAllSyncData() is kept
around, now moved to SyncableService, since all implementations provide
such functionality. However, we don't seem to call that function from
anywhere, so a TODO has been added to simplify that in future patches.

TBR=groby@chromium.org

Bug:  870624 
Change-Id: I54b33cdd1ecc5584a806d3ff0fc0f3f6708467ca
Reviewed-on: https://chromium-review.googlesource.com/1243809
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594702}
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/chrome/browser/extensions/api/storage/sync_storage_backend.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/chrome/browser/spellchecker/spellcheck_custom_dictionary.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/chrome/browser/supervised_user/supervised_user_settings_service.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/chrome/browser/supervised_user/supervised_user_whitelist_service.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/chrome/browser/supervised_user/supervised_user_whitelist_service.h
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/chrome/browser/supervised_user/supervised_user_whitelist_service_unittest.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/chrome/browser/themes/theme_syncable_service.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/autofill/core/browser/webdata/autofill_profile_syncable_service.h
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/autofill/core/browser/webdata/autofill_wallet_syncable_service.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/password_manager/core/browser/password_syncable_service.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/password_manager/core/browser/password_syncable_service_unittest.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/search_engines/template_url_service.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/search_engines/template_url_service.h
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/sync/driver/shared_change_processor.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/sync/model/fake_syncable_service.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/sync/model/sync_change.h
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/sync/model/sync_change_processor.h
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/sync/model/sync_change_processor_wrapper_for_test.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/sync/model/sync_change_processor_wrapper_for_test.h
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/sync/model/syncable_service.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/sync/model/syncable_service.h
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/sync_preferences/pref_model_associator.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/sync_preferences/pref_service_syncable_unittest.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/sync_sessions/sessions_sync_manager.cc
[modify] https://crrev.com/ed5d9496bc9e842479193ddf5425e5fa4ce56920/components/sync_sessions/sessions_sync_manager_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/715c7e1847e2641d47049b7878ea7ea6dbe04998

commit 715c7e1847e2641d47049b7878ea7ea6dbe04998
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Sep 27 21:58:37 2018

Allow bridges without support for computing client tags

We ancitipate future changes where certain bridges will lack the support
for client tag hashes. Hence, let's accomodate that in the bridge
interface and update the processor to honor the new contract.

Bug:  870624 ,881289
Change-Id: Ic871d7a9fd7cf9c9a989624e1691409aed400e39
Reviewed-on: https://chromium-review.googlesource.com/1249023
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Florian Uunk <feuunk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594885}
[modify] https://crrev.com/715c7e1847e2641d47049b7878ea7ea6dbe04998/components/sync/model/model_type_change_processor.cc
[modify] https://crrev.com/715c7e1847e2641d47049b7878ea7ea6dbe04998/components/sync/model/model_type_change_processor.h
[modify] https://crrev.com/715c7e1847e2641d47049b7878ea7ea6dbe04998/components/sync/model/model_type_sync_bridge.cc
[modify] https://crrev.com/715c7e1847e2641d47049b7878ea7ea6dbe04998/components/sync/model/model_type_sync_bridge.h
[modify] https://crrev.com/715c7e1847e2641d47049b7878ea7ea6dbe04998/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/715c7e1847e2641d47049b7878ea7ea6dbe04998/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/715c7e1847e2641d47049b7878ea7ea6dbe04998/docs/sync/uss/client_tag_based_model_type_processor.md

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c8259e6ce46cc4513c09d82ce4ca589174fa798

commit 2c8259e6ce46cc4513c09d82ce4ca589174fa798
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Sep 28 08:16:20 2018

Introduce generic bridge for SyncableService-s

The class allows legacy datatypes (non-USS), which implement
SyncableService, to be integrated within the USS architecture, making
it possible to reuse central objects like
ClientTagBasedModelTypeProcessor and ModelTypeWorker.

Design Doc (Googlers only):
https://docs.google.com/document/d/14ScYZ0sop921gjBwXuReIEuQJlwftqkuSM1jMK_A_x4

In this first patch, we introduce the bridge itself. Follow-up patches
will introduce the necessary plumbing to exercise it.

Expected main benefits:
- Unify logic across datatypes by avoiding subtle behavioral differences
  across architectures.
- Remove lots of code (~25K LoC), hence:
  a) reduce Chrome binary size.
  b) reduce maintenance cost.
  c) eng ramp-up time.
- Improve resource footprint (less RAM, ~50% savings).
- Reduce the gap for datatypes to actually migrate to USS.
- Unblock multiple cleanup work, including a massive simplification of
  DataTypeManager and related classes, including controllers.

Bug:  870624 
Change-Id: I1bd7f553bf22886c5136e7e12f13b37a3dc77a39
Reviewed-on: https://chromium-review.googlesource.com/1164742
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595017}
[modify] https://crrev.com/2c8259e6ce46cc4513c09d82ce4ca589174fa798/components/sync/BUILD.gn
[modify] https://crrev.com/2c8259e6ce46cc4513c09d82ce4ca589174fa798/components/sync/model/model_type_sync_bridge.h
[modify] https://crrev.com/2c8259e6ce46cc4513c09d82ce4ca589174fa798/components/sync/model_impl/client_tag_based_model_type_processor.cc
[add] https://crrev.com/2c8259e6ce46cc4513c09d82ce4ca589174fa798/components/sync/model_impl/syncable_service_based_bridge.cc
[add] https://crrev.com/2c8259e6ce46cc4513c09d82ce4ca589174fa798/components/sync/model_impl/syncable_service_based_bridge.h
[add] https://crrev.com/2c8259e6ce46cc4513c09d82ce4ca589174fa798/components/sync/model_impl/syncable_service_based_bridge_unittest.cc
[add] https://crrev.com/2c8259e6ce46cc4513c09d82ce4ca589174fa798/components/sync/protocol/persisted_entity_data.proto
[modify] https://crrev.com/2c8259e6ce46cc4513c09d82ce4ca589174fa798/components/sync/protocol/protocol_sources.gni

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a974891da2e28b162d791d1ae3f9e9e4c28670e6

commit a974891da2e28b162d791d1ae3f9e9e4c28670e6
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Sep 28 10:00:52 2018

Add plumbing and sync integration tests for pseudo-USS

A USS bridge has been recently introduced in
https://chromium-review.googlesource.com/c/chromium/src/+/1164742 to
allow integrating legacy SyncableService sync datatypes into the most
modern USS architecture.

In this patch, a controller is introduced to do the necessary plumbing
and use the new codepath behind feature toggles. We do this for a first
set of datatypes that are simplest, i.e. the ones that live in the UI
thread and without complex logic in their controllers. Follow-up
patches will add support for more.

Bug:  870624 
Change-Id: Ibdcc01133103296bd9c53e165a37b63cc32dcb87
Reviewed-on: https://chromium-review.googlesource.com/1251102
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595036}
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/chrome/browser/BUILD.gn
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/chrome/browser/sync/chrome_sync_client.cc
[add] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/chrome/browser/sync/glue/extension_model_type_controller.cc
[add] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/chrome/browser/sync/glue/extension_model_type_controller.h
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/chrome/browser/sync/test/integration/single_client_apps_sync_test.cc
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/chrome/browser/sync/test/integration/single_client_dictionary_sync_test.cc
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/chrome/browser/sync/test/integration/single_client_extensions_sync_test.cc
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/chrome/browser/sync/test/integration/single_client_themes_sync_test.cc
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/chrome/browser/sync/test/integration/two_client_apps_sync_test.cc
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/chrome/browser/sync/test/integration/two_client_dictionary_sync_test.cc
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/chrome/browser/sync/test/integration/two_client_extensions_sync_test.cc
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/chrome/browser/sync/test/integration/two_client_themes_sync_test.cc
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/components/sync/BUILD.gn
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/components/sync/driver/sync_driver_switches.cc
[modify] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/components/sync/driver/sync_driver_switches.h
[add] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/components/sync/driver/syncable_service_based_model_type_controller.cc
[add] https://crrev.com/a974891da2e28b162d791d1ae3f9e9e4c28670e6/components/sync/driver/syncable_service_based_model_type_controller.h

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8252b8b10a63c9a41f505eccb1ee912ce70f5c7e

commit 8252b8b10a63c9a41f505eccb1ee912ce70f5c7e
Author: Daniel Bratell <bratell@opera.com>
Date: Fri Sep 28 11:02:00 2018

[jumbo] One kInvalidId constant should be enough.

Jumbo compilation in components/sync broke when a kInvalidId
clone appeared with the same name. This build unbreaking patch
renames it to kInvalidNodeId. A longer term fix is probably to
move the constant to modules/sync/base or another shared location.

Regression from https://chromium-review.googlesource.com/c/chromium/src/+/1164742

TBR=mastiz@chromium.org,treib@chromium.org

Bug:  870624 
Change-Id: Ie300cf3338c8ffb93a2a0b3b656838bdbf10248a
Reviewed-on: https://chromium-review.googlesource.com/1250965
Commit-Queue: Daniel Bratell <bratell@opera.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#595052}
[modify] https://crrev.com/8252b8b10a63c9a41f505eccb1ee912ce70f5c7e/components/sync/model_impl/syncable_service_based_bridge.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9e24e880c1ed7fb3ebc723c9494d79bd52973edb

commit 9e24e880c1ed7fb3ebc723c9494d79bd52973edb
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Oct 02 05:51:02 2018

Integrate history delete directive sync in USS

We leverage the recently introduced layer that allows integrating
legacy SyncableService implementations within the newest architecture,
USS. This requires forking the custom controller, because a different
class hierarchy is used.

The new codepath is behind feature toggle
kSyncPseudoUSSHistoryDeleteDirectives.

Bug:  870624 
Change-Id: Ief8fb0d5f5d0472aeea1e0edf42e905632bc33fb
Reviewed-on: https://chromium-review.googlesource.com/1251262
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595735}
[add] https://crrev.com/9e24e880c1ed7fb3ebc723c9494d79bd52973edb/chrome/browser/sync/test/integration/single_client_history_delete_directives_sync_test.cc
[modify] https://crrev.com/9e24e880c1ed7fb3ebc723c9494d79bd52973edb/chrome/test/BUILD.gn
[modify] https://crrev.com/9e24e880c1ed7fb3ebc723c9494d79bd52973edb/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/9e24e880c1ed7fb3ebc723c9494d79bd52973edb/components/history/core/browser/BUILD.gn
[modify] https://crrev.com/9e24e880c1ed7fb3ebc723c9494d79bd52973edb/components/history/core/browser/OWNERS
[modify] https://crrev.com/9e24e880c1ed7fb3ebc723c9494d79bd52973edb/components/history/core/browser/history_delete_directives_data_type_controller.h
[add] https://crrev.com/9e24e880c1ed7fb3ebc723c9494d79bd52973edb/components/history/core/browser/history_delete_directives_model_type_controller.cc
[add] https://crrev.com/9e24e880c1ed7fb3ebc723c9494d79bd52973edb/components/history/core/browser/history_delete_directives_model_type_controller.h
[modify] https://crrev.com/9e24e880c1ed7fb3ebc723c9494d79bd52973edb/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/9e24e880c1ed7fb3ebc723c9494d79bd52973edb/components/sync/driver/sync_driver_switches.cc
[modify] https://crrev.com/9e24e880c1ed7fb3ebc723c9494d79bd52973edb/components/sync/driver/sync_driver_switches.h

Project Member

Comment 10 by bugdroid1@chromium.org, Oct 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c

commit bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Oct 02 06:21:23 2018

Add more sync integration tests for pseudo-USS

We add integration test coverage for PREFERENCES under pseudo-USS, and
factor out some duplicated code to a newly introduced FeatureToggler, a
helper class to run tests with a feature switch enabled and disabled.

Bug:  870624 
Change-Id: I6b1b446439969d5c6e948e21ca160876e14c5cdb
Reviewed-on: https://chromium-review.googlesource.com/1253762
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595740}
[add] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/feature_toggler.cc
[add] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/feature_toggler.h
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/single_client_apps_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/single_client_dictionary_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/single_client_directory_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/single_client_extensions_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/single_client_history_delete_directives_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/single_client_preferences_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/single_client_themes_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/two_client_apps_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/two_client_dictionary_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/two_client_extensions_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/two_client_preferences_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/browser/sync/test/integration/two_client_themes_sync_test.cc
[modify] https://crrev.com/bb23ebe4a89edf46d3e1a69da5426603f2a9ee1c/chrome/test/BUILD.gn

Project Member

Comment 11 by bugdroid1@chromium.org, Oct 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ef56e36ab714424a32c3c9091eb3df2fb68d1ca2

commit ef56e36ab714424a32c3c9091eb3df2fb68d1ca2
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Oct 02 09:23:23 2018

Integrate supervised users sync datatypes in pseudo-USS

This is part of a patch series where we leverage the recently introduced
layer that allows integrating legacy SyncableService implementations
within the newest architecture, USS. This requires forking custom
controllers because a different class hierarchy is used.

Bug:  870624 
Change-Id: I0931243109eef8f757692c5a932f03004f8ce039
Reviewed-on: https://chromium-review.googlesource.com/1254211
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595771}
[modify] https://crrev.com/ef56e36ab714424a32c3c9091eb3df2fb68d1ca2/chrome/browser/BUILD.gn
[add] https://crrev.com/ef56e36ab714424a32c3c9091eb3df2fb68d1ca2/chrome/browser/supervised_user/supervised_user_sync_model_type_controller.cc
[add] https://crrev.com/ef56e36ab714424a32c3c9091eb3df2fb68d1ca2/chrome/browser/supervised_user/supervised_user_sync_model_type_controller.h
[modify] https://crrev.com/ef56e36ab714424a32c3c9091eb3df2fb68d1ca2/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/ef56e36ab714424a32c3c9091eb3df2fb68d1ca2/components/sync/driver/sync_driver_switches.cc
[modify] https://crrev.com/ef56e36ab714424a32c3c9091eb3df2fb68d1ca2/components/sync/driver/sync_driver_switches.h

Project Member

Comment 12 by bugdroid1@chromium.org, Oct 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f7a75a90ed0d2ef3bf52f70ac44fa630cb90049e

commit f7a75a90ed0d2ef3bf52f70ac44fa630cb90049e
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Oct 04 18:08:53 2018

Integrate SEARCH_ENGINES sync into pseudo-USS

This is part of a patch series where we leverage the recently introduced
layer that allows integrating legacy SyncableService implementations
within the newest architecture, USS.

SEARCH_ENGINES is the first datatype where the legacy controller
(SearchEngineDataTypeController) implements logic to make sure no
interactions with the SyncableService can occur before
TemplateURLService is fully loaded.

Implementing similar logic in pseudo-USS is nontrivial because:
1. ModelTypeController doesn't support it because it is designed for
   full-blown USS, where such logic is usually implemented in the bridge
   itself.

2. SyncableService doesn't support it, because in the legacy
   architecture the controllers took care (AsyncDirectoryTypeController).

For pseudo-USS, this being the first datatype, the least intrusive
solution is proposed, which involves a dedicated
ModelTypeControllerDelegate. We may change this in the future if more
datatypes have similar needs (e.g. ArcPackageSyncDataTypeController).

Bug:  870624 
Change-Id: I28bb028de99d98608bdf07a06db1883221b8161e
Reviewed-on: https://chromium-review.googlesource.com/c/1261435
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596756}
[modify] https://crrev.com/f7a75a90ed0d2ef3bf52f70ac44fa630cb90049e/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/f7a75a90ed0d2ef3bf52f70ac44fa630cb90049e/chrome/browser/sync/test/integration/single_client_search_engines_sync_test.cc
[modify] https://crrev.com/f7a75a90ed0d2ef3bf52f70ac44fa630cb90049e/chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc
[modify] https://crrev.com/f7a75a90ed0d2ef3bf52f70ac44fa630cb90049e/components/search_engines/BUILD.gn
[add] https://crrev.com/f7a75a90ed0d2ef3bf52f70ac44fa630cb90049e/components/search_engines/search_engine_model_type_controller.cc
[add] https://crrev.com/f7a75a90ed0d2ef3bf52f70ac44fa630cb90049e/components/search_engines/search_engine_model_type_controller.h
[modify] https://crrev.com/f7a75a90ed0d2ef3bf52f70ac44fa630cb90049e/components/sync/driver/sync_driver_switches.cc
[modify] https://crrev.com/f7a75a90ed0d2ef3bf52f70ac44fa630cb90049e/components/sync/driver/sync_driver_switches.h
[modify] https://crrev.com/f7a75a90ed0d2ef3bf52f70ac44fa630cb90049e/components/sync/driver/syncable_service_based_model_type_controller.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ce9d19206ccbbaabd4cc6d90738c941f74ae1a9e

commit ce9d19206ccbbaabd4cc6d90738c941f74ae1a9e
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Oct 05 08:42:16 2018

Plumb crashpad dumper for pseudo-USS types

If a model (or SyncableService) reports an error, let's plumb it through
to base::debug::DumpWithoutCrashing() via ReportUnrecoverableError(),
which does it only for canary&dev channels, with random sampling. This
achieves feature parity with the directory-based setup and may allow
future investigations if we see datatypes with high error rates.

Bug:  870624 
Change-Id: Idb3d00b178167a8b7b9eabb560858197e03aecb2
Reviewed-on: https://chromium-review.googlesource.com/c/1264198
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597035}
[modify] https://crrev.com/ce9d19206ccbbaabd4cc6d90738c941f74ae1a9e/chrome/browser/supervised_user/supervised_user_sync_model_type_controller.cc
[modify] https://crrev.com/ce9d19206ccbbaabd4cc6d90738c941f74ae1a9e/chrome/browser/supervised_user/supervised_user_sync_model_type_controller.h
[modify] https://crrev.com/ce9d19206ccbbaabd4cc6d90738c941f74ae1a9e/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/ce9d19206ccbbaabd4cc6d90738c941f74ae1a9e/chrome/browser/sync/glue/extension_model_type_controller.cc
[modify] https://crrev.com/ce9d19206ccbbaabd4cc6d90738c941f74ae1a9e/chrome/browser/sync/glue/extension_model_type_controller.h
[modify] https://crrev.com/ce9d19206ccbbaabd4cc6d90738c941f74ae1a9e/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/ce9d19206ccbbaabd4cc6d90738c941f74ae1a9e/components/history/core/browser/sync/history_delete_directives_model_type_controller.cc
[modify] https://crrev.com/ce9d19206ccbbaabd4cc6d90738c941f74ae1a9e/components/history/core/browser/sync/history_delete_directives_model_type_controller.h
[modify] https://crrev.com/ce9d19206ccbbaabd4cc6d90738c941f74ae1a9e/components/sync/driver/syncable_service_based_model_type_controller.cc
[modify] https://crrev.com/ce9d19206ccbbaabd4cc6d90738c941f74ae1a9e/components/sync/driver/syncable_service_based_model_type_controller.h

Project Member

Comment 14 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e14824d2a104f2dd0bbf0b648067f506e4c17e10

commit e14824d2a104f2dd0bbf0b648067f506e4c17e10
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Oct 05 08:48:00 2018

Account memory overhead for SyncableServiceBasedBridge

UMA metrics Sync.ModelTypeMemoryKB.<datatype> are often used to
compare two implementations of sync (pre-USS and USS), to analyze impact
on memory use, and expected to improve with USS.

With the recently introduced SyncableServiceBasedBridge, making a fair
comparison requires accounting for the memory consumed by the bridge
itself, which keeps an in-memory copy of all entities (similarly to the
legacy directory itself). In this patch, we implement such accounting,
by introducing a new method in ModelTypeSyncBridge.

Bug:  870624 
Change-Id: Idd632f7bccdd39a530aa2a1c024e55604bd9ba17
Reviewed-on: https://chromium-review.googlesource.com/c/1264375
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597038}
[modify] https://crrev.com/e14824d2a104f2dd0bbf0b648067f506e4c17e10/components/sync/model/model_type_sync_bridge.cc
[modify] https://crrev.com/e14824d2a104f2dd0bbf0b648067f506e4c17e10/components/sync/model/model_type_sync_bridge.h
[modify] https://crrev.com/e14824d2a104f2dd0bbf0b648067f506e4c17e10/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/e14824d2a104f2dd0bbf0b648067f506e4c17e10/components/sync/model_impl/syncable_service_based_bridge.cc
[modify] https://crrev.com/e14824d2a104f2dd0bbf0b648067f506e4c17e10/components/sync/model_impl/syncable_service_based_bridge.h
[modify] https://crrev.com/e14824d2a104f2dd0bbf0b648067f506e4c17e10/components/sync/protocol/proto_memory_estimations.cc
[modify] https://crrev.com/e14824d2a104f2dd0bbf0b648067f506e4c17e10/components/sync/protocol/proto_visitors.h

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03ae022ac28ac2943271214847251f50efd17ee5

commit 03ae022ac28ac2943271214847251f50efd17ee5
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Oct 08 10:15:35 2018

Fix sync metadata persistence for pseudo-USS

All changes to the in-memory sync metadata should be reflected in
persisted sync metadata, and a bug in the implementation prevented that.
The end result is that all sync entities would be refetched on every
restart, for the experimental pseudo-USS codepath (behind feature
toggle).

Given that we've already done some experimentation on canary channel,
we introduce the logic to recover cleanly from the resulting
inconsistent state, which seems like a desirable safeguard anyway.

Bug:  870624 
Change-Id: I28b170a6ff62ecb78c94c7026796df56687507dc
Reviewed-on: https://chromium-review.googlesource.com/c/1267498
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597516}
[modify] https://crrev.com/03ae022ac28ac2943271214847251f50efd17ee5/components/sync/model_impl/syncable_service_based_bridge.cc
[modify] https://crrev.com/03ae022ac28ac2943271214847251f50efd17ee5/components/sync/model_impl/syncable_service_based_bridge_unittest.cc

Labels: -Type-Feature OS-Android OS-Chrome OS-iOS OS-Linux OS-Mac OS-Windows Type-Launch
Status: Started (was: Assigned)
Project Member

Comment 17 by bugdroid1@chromium.org, Oct 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9f5600fbe039f734be99ed0a6e9336bc8e892130

commit 9f5600fbe039f734be99ed0a6e9336bc8e892130
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Oct 11 15:40:17 2018

Fix history delete directive cleanup in pseudo-USS

Ancient datatype-specific quirks are common in sync, and here's another
one: HISTORY_DELETE_DIRECTIVES datatype has special-handling in
GenericChangeProcessor() to handle directive deletion: instead of
tombstones uploaded to the server, the entries should be untracked
locally but nothing sent to the server.

The patch also fixes a DCHECK failure affecting pseudo-USS, because
such events (local "deletions") are signaled by the SyncableService as
remote changes / SyncDataRemote. Such is the implementation in
GenericChangeProcessor, so we follow the same approach in the
pseudo-USS bridge for compatibility.

Bug:  870624 
Change-Id: Ic009b13c6e4aa7c3982d01b9ebd7511c6a0e3185
Reviewed-on: https://chromium-review.googlesource.com/c/1276607
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598773}
[modify] https://crrev.com/9f5600fbe039f734be99ed0a6e9336bc8e892130/chrome/browser/sync/test/integration/single_client_history_delete_directives_sync_test.cc
[modify] https://crrev.com/9f5600fbe039f734be99ed0a6e9336bc8e892130/components/sync/model/sync_data.cc
[modify] https://crrev.com/9f5600fbe039f734be99ed0a6e9336bc8e892130/components/sync/model_impl/syncable_service_based_bridge.cc

Project Member

Comment 18 by bugdroid1@chromium.org, Oct 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7343cdc9fedaee70ef42201472499f04bbd8756

commit d7343cdc9fedaee70ef42201472499f04bbd8756
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Oct 12 20:05:40 2018

Add UMA metric to monitor I/O per USS datatype

This became specially relevant recently due to ongoing adoption of
ModelTypeStoreBackend for allmost all sync datatypes. Let's keep an eye
of whether any of them is hammering disk I/O.

Bug:  870624 
Change-Id: I739ad4feaf9ef0ba3d74d0444d2c9fcb7ef48455
Reviewed-on: https://chromium-review.googlesource.com/c/1275852
Reviewed-by: Florian Uunk <feuunk@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599329}
[modify] https://crrev.com/d7343cdc9fedaee70ef42201472499f04bbd8756/components/sync/model_impl/blocking_model_type_store_impl.cc
[modify] https://crrev.com/d7343cdc9fedaee70ef42201472499f04bbd8756/tools/metrics/histograms/histograms.xml

Project Member

Comment 19 by bugdroid1@chromium.org, Oct 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62f7ba65d7479865d4cc5386be1b7f179046962e

commit 62f7ba65d7479865d4cc5386be1b7f179046962e
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Oct 15 17:34:20 2018

Add extension/app settings to pseudo-USS

These are the first two pseudo-USS types outside the UI thread, which
requires some extra plumbing. Around 100 LoC must be invested to add
support for a variant of the controller that allows construction and
destruction of bridge and processor in the model thread.

In addition, pseudo-USS relies on a directory-like storage
implemented with ModelTypeStore, which needs some quirks to be
supported outside the UI thread: namely, the store factory must be
allowed to run on any thread, and the destruction sequence must be
refcounted, in particular for ModelTypeStoreBackend, since the
destruction order across backend sequences is not deterministic.

Bug:  870624 
Change-Id: I6dbe210277e75f7662a2b744031595c90beb1c62
Reviewed-on: https://chromium-review.googlesource.com/c/1278639
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599662}
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/chrome/browser/BUILD.gn
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/chrome/browser/sync/chrome_sync_client.cc
[add] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/chrome/browser/sync/glue/extension_setting_model_type_controller.cc
[add] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/chrome/browser/sync/glue/extension_setting_model_type_controller.h
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/chrome/browser/sync/test/integration/two_client_extension_settings_and_app_settings_sync_test.cc
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/BUILD.gn
[add] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/driver/non_ui_syncable_service_based_model_type_controller.cc
[add] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/driver/non_ui_syncable_service_based_model_type_controller.h
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/driver/sync_driver_switches.cc
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/driver/sync_driver_switches.h
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/driver/syncable_service_based_model_type_controller.h
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model/model_type_store_service.h
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model/model_type_store_test_util.cc
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model/test_model_type_store_service.h
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model_impl/blocking_model_type_store_impl.cc
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model_impl/blocking_model_type_store_impl.h
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model_impl/model_type_store_backend.cc
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model_impl/model_type_store_backend.h
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model_impl/model_type_store_backend_unittest.cc
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model_impl/model_type_store_impl.cc
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model_impl/model_type_store_service_impl.cc
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model_impl/model_type_store_service_impl.h
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model_impl/syncable_service_based_bridge.cc
[modify] https://crrev.com/62f7ba65d7479865d4cc5386be1b7f179046962e/components/sync/model_impl/syncable_service_based_bridge.h

Blockedon: 895899
Project Member

Comment 21 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/334a73b3008aa84623f819beda6af78d56b30e35

commit 334a73b3008aa84623f819beda6af78d56b30e35
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Oct 19 13:02:16 2018

Add more PASSWORDS integration tests

They verify some otherwise-untested but privacy-sensitive requirements
of what is being sent on the wire, where most (without custom
passphrase) or all (with custom passphrase) proto fields should be
encrypted.

The actual verification is done using the state in the fake server,
which is assumed to store committed content without pruning. This holds
true today and is unlikely to change in the future.

Bug:  856941 , 870624 
Change-Id: I9198ca69c53d55a80f4b2a0c55cd5265e57741aa
Reviewed-on: https://chromium-review.googlesource.com/c/1290809
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601126}
[modify] https://crrev.com/334a73b3008aa84623f819beda6af78d56b30e35/chrome/browser/sync/test/integration/single_client_passwords_sync_test.cc

Project Member

Comment 22 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66a392fa289531d8880ba9ea03148d7d7ea4e274

commit 66a392fa289531d8880ba9ea03148d7d7ea4e274
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Oct 19 19:27:22 2018

Add encryption support to pseudo-USS

This patch extends SyncableServiceBasedBridge (core to pseudo-USS)
with support for encryption, via a newly introduced interface
(ModelCryptographer), which needs to be usable in the model thread
(where the bridge lives).

This should unblock the migration of PASSWORDS to pseudo-USS, because
password data must be encrypted at all times. This is a first step of
a proposal described in a dedicated section in the Design Doc:
https://docs.google.com/document/d/14ScYZ0sop921gjBwXuReIEuQJlwftqkuSM1jMK_A_x4/edit#heading=h.ev0xr4j8pkot

Actual usage of this functionality will be introduced in follow-up
patches.

Bug:  870624 
Change-Id: I227d429dc952bfe1a3a4fb05cd1ab71cac9ba1c6
Reviewed-on: https://chromium-review.googlesource.com/c/1288594
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601254}
[modify] https://crrev.com/66a392fa289531d8880ba9ea03148d7d7ea4e274/components/sync/driver/non_ui_syncable_service_based_model_type_controller.cc
[modify] https://crrev.com/66a392fa289531d8880ba9ea03148d7d7ea4e274/components/sync/driver/non_ui_syncable_service_based_model_type_controller.h
[modify] https://crrev.com/66a392fa289531d8880ba9ea03148d7d7ea4e274/components/sync/model/sync_data.cc
[modify] https://crrev.com/66a392fa289531d8880ba9ea03148d7d7ea4e274/components/sync/model/sync_data.h
[modify] https://crrev.com/66a392fa289531d8880ba9ea03148d7d7ea4e274/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/66a392fa289531d8880ba9ea03148d7d7ea4e274/components/sync/model_impl/syncable_service_based_bridge.cc
[modify] https://crrev.com/66a392fa289531d8880ba9ea03148d7d7ea4e274/components/sync/model_impl/syncable_service_based_bridge.h
[modify] https://crrev.com/66a392fa289531d8880ba9ea03148d7d7ea4e274/components/sync/model_impl/syncable_service_based_bridge_unittest.cc
[modify] https://crrev.com/66a392fa289531d8880ba9ea03148d7d7ea4e274/components/sync/protocol/password_specifics.proto
[modify] https://crrev.com/66a392fa289531d8880ba9ea03148d7d7ea4e274/components/sync/test/engine/mock_model_type_worker.cc

Project Member

Comment 23 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d370f99315e7b7fb9cfd3e3eb51480d0c4788774

commit d370f99315e7b7fb9cfd3e3eb51480d0c4788774
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Oct 19 19:39:21 2018

Add PASSWORDS sync support to USS codepath

ModelTypeWorker et al need to be updated to support PASSWORDS, which is
the only datatype with a custom encryption mechanism (always encrypted).
This involves implementing logic analogous to the directory counterpart,
which uses passwords-specific proto fields and other requirements like
clearing out certain fields if custom passphrase is set.

The introduced code changes are not exercised in production yet because
there's no USS (or pseudo-USS) implementation for PASSWORDS.

Bug:  856941 , 870624 
Change-Id: Iae7c9a9a76f87dbd2406ed8e56589dc629f07c6a
Reviewed-on: https://chromium-review.googlesource.com/c/1288537
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601257}
[modify] https://crrev.com/d370f99315e7b7fb9cfd3e3eb51480d0c4788774/components/sync/engine/sync_encryption_handler.h
[modify] https://crrev.com/d370f99315e7b7fb9cfd3e3eb51480d0c4788774/components/sync/engine_impl/model_type_registry.cc
[modify] https://crrev.com/d370f99315e7b7fb9cfd3e3eb51480d0c4788774/components/sync/engine_impl/model_type_registry.h
[modify] https://crrev.com/d370f99315e7b7fb9cfd3e3eb51480d0c4788774/components/sync/engine_impl/model_type_worker.cc
[modify] https://crrev.com/d370f99315e7b7fb9cfd3e3eb51480d0c4788774/components/sync/engine_impl/model_type_worker.h
[modify] https://crrev.com/d370f99315e7b7fb9cfd3e3eb51480d0c4788774/components/sync/engine_impl/model_type_worker_unittest.cc
[modify] https://crrev.com/d370f99315e7b7fb9cfd3e3eb51480d0c4788774/components/sync/engine_impl/non_blocking_type_commit_contribution.cc
[modify] https://crrev.com/d370f99315e7b7fb9cfd3e3eb51480d0c4788774/components/sync/engine_impl/non_blocking_type_commit_contribution.h
[modify] https://crrev.com/d370f99315e7b7fb9cfd3e3eb51480d0c4788774/components/sync/engine_impl/non_blocking_type_commit_contribution_unittest.cc
[modify] https://crrev.com/d370f99315e7b7fb9cfd3e3eb51480d0c4788774/components/sync/engine_impl/sync_encryption_handler_impl.cc
[modify] https://crrev.com/d370f99315e7b7fb9cfd3e3eb51480d0c4788774/components/sync/engine_impl/uss_migrator.cc
[modify] https://crrev.com/d370f99315e7b7fb9cfd3e3eb51480d0c4788774/components/sync/engine_impl/uss_migrator_unittest.cc

Project Member

Comment 24 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e636d722af63786861c116f8763ba9568131245d

commit e636d722af63786861c116f8763ba9568131245d
Author: Joe Downing <joedow@chromium.org>
Date: Fri Oct 19 20:09:36 2018

Revert "Add encryption support to pseudo-USS"

This reverts commit 66a392fa289531d8880ba9ea03148d7d7ea4e274.

Reason for revert: Causing component_unittest failures:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Cast%20Audio%20Linux/23527

failing tests:
SyncableServiceBasedBridgeTest.ShouldDecryptPreviousDirectoryDataAfterRestart
SyncableServiceBasedBridgeTest.ShouldDecryptInitialRemoteData
SyncableServiceBasedBridgeTest.ShouldDecryptForRemoteDeletion

Original change's description:
> Add encryption support to pseudo-USS
> 
> This patch extends SyncableServiceBasedBridge (core to pseudo-USS)
> with support for encryption, via a newly introduced interface
> (ModelCryptographer), which needs to be usable in the model thread
> (where the bridge lives).
> 
> This should unblock the migration of PASSWORDS to pseudo-USS, because
> password data must be encrypted at all times. This is a first step of
> a proposal described in a dedicated section in the Design Doc:
> https://docs.google.com/document/d/14ScYZ0sop921gjBwXuReIEuQJlwftqkuSM1jMK_A_x4/edit#heading=h.ev0xr4j8pkot
> 
> Actual usage of this functionality will be introduced in follow-up
> patches.
> 
> Bug:  870624 
> Change-Id: I227d429dc952bfe1a3a4fb05cd1ab71cac9ba1c6
> Reviewed-on: https://chromium-review.googlesource.com/c/1288594
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#601254}

TBR=treib@chromium.org,mastiz@chromium.org,mamir@chromium.org

Change-Id: Iab6463f9541ca242facdc8c5217f1cf4c114ba49
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  870624 
Reviewed-on: https://chromium-review.googlesource.com/c/1292512
Reviewed-by: Joe Downing <joedow@chromium.org>
Commit-Queue: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601270}
[modify] https://crrev.com/e636d722af63786861c116f8763ba9568131245d/components/sync/driver/non_ui_syncable_service_based_model_type_controller.cc
[modify] https://crrev.com/e636d722af63786861c116f8763ba9568131245d/components/sync/driver/non_ui_syncable_service_based_model_type_controller.h
[modify] https://crrev.com/e636d722af63786861c116f8763ba9568131245d/components/sync/model/sync_data.cc
[modify] https://crrev.com/e636d722af63786861c116f8763ba9568131245d/components/sync/model/sync_data.h
[modify] https://crrev.com/e636d722af63786861c116f8763ba9568131245d/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/e636d722af63786861c116f8763ba9568131245d/components/sync/model_impl/syncable_service_based_bridge.cc
[modify] https://crrev.com/e636d722af63786861c116f8763ba9568131245d/components/sync/model_impl/syncable_service_based_bridge.h
[modify] https://crrev.com/e636d722af63786861c116f8763ba9568131245d/components/sync/model_impl/syncable_service_based_bridge_unittest.cc
[modify] https://crrev.com/e636d722af63786861c116f8763ba9568131245d/components/sync/protocol/password_specifics.proto
[modify] https://crrev.com/e636d722af63786861c116f8763ba9568131245d/components/sync/test/engine/mock_model_type_worker.cc

Project Member

Comment 25 by bugdroid1@chromium.org, Oct 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ffadc32258ea77bdb878a58556aeb30f5210bf2b

commit ffadc32258ea77bdb878a58556aeb30f5210bf2b
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Oct 22 05:48:26 2018

Reland "Add encryption support to pseudo-USS"

This is a reland of 66a392fa289531d8880ba9ea03148d7d7ea4e274

Reland fixes unit tests that accidentally used DCHECKs with side
effects, making tests fail in non-debug builds.

Original change's description:
> Add encryption support to pseudo-USS
>
> This patch extends SyncableServiceBasedBridge (core to pseudo-USS)
> with support for encryption, via a newly introduced interface
> (ModelCryptographer), which needs to be usable in the model thread
> (where the bridge lives).
>
> This should unblock the migration of PASSWORDS to pseudo-USS, because
> password data must be encrypted at all times. This is a first step of
> a proposal described in a dedicated section in the Design Doc:
> https://docs.google.com/document/d/14ScYZ0sop921gjBwXuReIEuQJlwftqkuSM1jMK_A_x4/edit#heading=h.ev0xr4j8pkot
>
> Actual usage of this functionality will be introduced in follow-up
> patches.
>
> Bug:  870624 
> Change-Id: I227d429dc952bfe1a3a4fb05cd1ab71cac9ba1c6
> Reviewed-on: https://chromium-review.googlesource.com/c/1288594
> Commit-Queue: Mikel Astiz <mastiz@chromium.org>
> Reviewed-by: Marc Treib <treib@chromium.org>
> Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#601254}

TBR=treib@chromium.org

Bug:  870624 
Change-Id: Ie576b1aebf6bb989bba95d808031ab8433ec8b6a
Reviewed-on: https://chromium-review.googlesource.com/c/1292556
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601479}
[modify] https://crrev.com/ffadc32258ea77bdb878a58556aeb30f5210bf2b/components/sync/driver/non_ui_syncable_service_based_model_type_controller.cc
[modify] https://crrev.com/ffadc32258ea77bdb878a58556aeb30f5210bf2b/components/sync/driver/non_ui_syncable_service_based_model_type_controller.h
[modify] https://crrev.com/ffadc32258ea77bdb878a58556aeb30f5210bf2b/components/sync/model/sync_data.cc
[modify] https://crrev.com/ffadc32258ea77bdb878a58556aeb30f5210bf2b/components/sync/model/sync_data.h
[modify] https://crrev.com/ffadc32258ea77bdb878a58556aeb30f5210bf2b/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/ffadc32258ea77bdb878a58556aeb30f5210bf2b/components/sync/model_impl/syncable_service_based_bridge.cc
[modify] https://crrev.com/ffadc32258ea77bdb878a58556aeb30f5210bf2b/components/sync/model_impl/syncable_service_based_bridge.h
[modify] https://crrev.com/ffadc32258ea77bdb878a58556aeb30f5210bf2b/components/sync/model_impl/syncable_service_based_bridge_unittest.cc
[modify] https://crrev.com/ffadc32258ea77bdb878a58556aeb30f5210bf2b/components/sync/protocol/password_specifics.proto
[modify] https://crrev.com/ffadc32258ea77bdb878a58556aeb30f5210bf2b/components/sync/test/engine/mock_model_type_worker.cc

Project Member

Comment 26 by bugdroid1@chromium.org, Oct 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/17abfac6e2571e9cba320ad9a20449ad6114d2aa

commit 17abfac6e2571e9cba320ad9a20449ad6114d2aa
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Oct 22 21:42:37 2018

Add PASSWORDS sync support to pseudo-USS

PASSWORDS is the only remaining datatype that hasn't been wrapped in
pseudo-USS and also the most difficult one, due to the encryption-
related particularities (entities must be encrypted at all times).
The proposal is described in a dedicated section in the Design Doc:
https://docs.google.com/document/d/14ScYZ0sop921gjBwXuReIEuQJlwftqkuSM1jMK_A_x4/edit#heading=h.ev0xr4j8pkot

The patch includes the USS variant of the datatype controller for
passwords, together with the required plumbing to enable
directory-level (SyncableServiceBasedBridge) encryption of data.
Cryptographer updates are posted from the sync thread directly to the
model thread (password's background thread) with a newly introduced
proxy observer, such that the bridge is up-to-date by the time the
processor receives updates. A refcounted class (interface
ModelCryptographer) is used for the plumbing to the bridge, due to
the complex lifetime conditions of the involved parts.

All this code is behind experimental feature toggle
"SyncPseudoUSSPasswords".

Because of encryption-related particularities of PASSWORDS, special
care is taken with integration tests, and all tests with or without
pseudo-USS enabled, including backward-compatibility tests that exercise
legacy clients talking to pseudo-USS-enabled clients (and vice versa).

Bug:  870624 
Change-Id: I3e9ecc36d0b27c4c28e98fa709e7d13f9fb5a025
Reviewed-on: https://chromium-review.googlesource.com/c/1289889
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601736}
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/chrome/browser/sync/test/integration/single_client_passwords_sync_test.cc
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/chrome/browser/sync/test/integration/sync_test.cc
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/chrome/browser/sync/test/integration/sync_test.h
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/chrome/browser/sync/test/integration/two_client_passwords_sync_test.cc
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/password_manager/core/browser/BUILD.gn
[add] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/password_manager/core/browser/password_model_type_controller.cc
[add] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/password_manager/core/browser/password_model_type_controller.h
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/password_manager/core/browser/password_store.cc
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/password_manager/core/browser/password_store.h
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/sync/driver/data_type_controller.cc
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/sync/driver/data_type_controller.h
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/sync/driver/glue/sync_backend_host_core.cc
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/sync/driver/glue/sync_backend_host_core.h
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/sync/driver/glue/sync_backend_host_impl_unittest.cc
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/sync/driver/sync_driver_switches.cc
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/sync/driver/sync_driver_switches.h
[modify] https://crrev.com/17abfac6e2571e9cba320ad9a20449ad6114d2aa/components/sync/engine/sync_engine.h

Project Member

Comment 27 by bugdroid1@chromium.org, Oct 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26111c2602fc3e1a85d1364d1140f99594d54fce

commit 26111c2602fc3e1a85d1364d1140f99594d54fce
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Oct 23 06:41:51 2018

Add reencryption support to pseudo-USS PASSWORDS

Regular USS datatypes (bridges) don't need to care about encryption,
because the processor and worker take care and the original model
doesn't need to be encrypted.

PASSWORDS on pseudo-USS are special because we need that the "directory"
copy (the one in SyncableServiceBasedBridge) is stored encrypted on
disk too, similarly to how the legacy Directory does it.

In certain cases like when setting up a custom passphrase, the
encryption key changes and the processor&worker react to that by
recommitting all entities with the new encryption requirements. For
pseudo-USS PASSWORDS, it should also reencrypt the local "directory".

In order to do this:
1. A new method is introduced in ModelTypeSyncBridge, for the bridge
   to realize there's an ongoing reencryption.

2. SyncableServiceBasedBridge takes care of using the cryptographer
   (available for PASSWORDS only) to reencrypt all data.

Implementation-wise, the simplest way to achieve that is to modify
the bridge such that in_memory_store_ keeps more information, namely
the whole sync_pb::PersistedEntityData proto.

Bug:  870624 
Change-Id: I1e0d7c972580377618c05b9d1f79c6d72f58022f
Reviewed-on: https://chromium-review.googlesource.com/c/1288629
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601863}
[modify] https://crrev.com/26111c2602fc3e1a85d1364d1140f99594d54fce/chrome/browser/sync/test/integration/single_client_passwords_sync_test.cc
[modify] https://crrev.com/26111c2602fc3e1a85d1364d1140f99594d54fce/components/sync/model/model_type_sync_bridge.cc
[modify] https://crrev.com/26111c2602fc3e1a85d1364d1140f99594d54fce/components/sync/model/model_type_sync_bridge.h
[modify] https://crrev.com/26111c2602fc3e1a85d1364d1140f99594d54fce/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/26111c2602fc3e1a85d1364d1140f99594d54fce/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/26111c2602fc3e1a85d1364d1140f99594d54fce/components/sync/model_impl/syncable_service_based_bridge.cc
[modify] https://crrev.com/26111c2602fc3e1a85d1364d1140f99594d54fce/components/sync/model_impl/syncable_service_based_bridge.h
[modify] https://crrev.com/26111c2602fc3e1a85d1364d1140f99594d54fce/components/sync/model_impl/syncable_service_based_bridge_unittest.cc

Project Member

Comment 28 by bugdroid1@chromium.org, Oct 23

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b3688e8c6353ebb23c20d1c62782a88844def168

commit b3688e8c6353ebb23c20d1c62782a88844def168
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Oct 23 08:01:50 2018

Improve error handling for USS datatypes

ClientTagBasedModelTypeProcessor should never reset |model_error_|,
which is believed to be an accidental behavioral change in
https://chromium-review.googlesource.com/923982

The rationale is, a USS model is not expected to be recoverable after
an error has occurred (e.g. I/O error).

In most cases, this makes little difference because the processor
reports the error to ModelTypeController, and the FAILED state is not
recoverable.

Semi-related, we should immediately disconnect from the sync engine
(ModelTypeWorker), because further remote changes should be ignored.

In addition to all this, we also now expose the error state in
interface ModelTypeChangeProcessor, for bridges that need to be aware
of this error state, and may want to stop further work.

Bug:  870624 
Change-Id: I404ccff60fcadf7f5a2b8fe4a76882ba1c7edd52
Reviewed-on: https://chromium-review.googlesource.com/c/1293579
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601878}
[modify] https://crrev.com/b3688e8c6353ebb23c20d1c62782a88844def168/components/history/core/browser/sync/typed_url_sync_bridge_unittest.cc
[modify] https://crrev.com/b3688e8c6353ebb23c20d1c62782a88844def168/components/sync/model/fake_model_type_change_processor.cc
[modify] https://crrev.com/b3688e8c6353ebb23c20d1c62782a88844def168/components/sync/model/fake_model_type_change_processor.h
[modify] https://crrev.com/b3688e8c6353ebb23c20d1c62782a88844def168/components/sync/model/mock_model_type_change_processor.cc
[modify] https://crrev.com/b3688e8c6353ebb23c20d1c62782a88844def168/components/sync/model/mock_model_type_change_processor.h
[modify] https://crrev.com/b3688e8c6353ebb23c20d1c62782a88844def168/components/sync/model/model_type_change_processor.h
[modify] https://crrev.com/b3688e8c6353ebb23c20d1c62782a88844def168/components/sync/model/recording_model_type_change_processor.cc
[modify] https://crrev.com/b3688e8c6353ebb23c20d1c62782a88844def168/components/sync/model/recording_model_type_change_processor.h
[modify] https://crrev.com/b3688e8c6353ebb23c20d1c62782a88844def168/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/b3688e8c6353ebb23c20d1c62782a88844def168/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/b3688e8c6353ebb23c20d1c62782a88844def168/components/sync/model_impl/syncable_service_based_bridge.cc
[modify] https://crrev.com/b3688e8c6353ebb23c20d1c62782a88844def168/components/sync/model_impl/syncable_service_based_bridge_unittest.cc

Project Member

Comment 29 by bugdroid1@chromium.org, Oct 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/409ffd3edd8afc17d67b1eb6736793e0805aded7

commit 409ffd3edd8afc17d67b1eb6736793e0805aded7
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Oct 24 11:42:25 2018

Fix missing USS controller stop completion

DataTypeManager is allowed to call Stop() while a datatype is
NOT_RUNNING or FAILED. In both cases, the function should be a no-op,
but the completion callback should be called.

Prior to this patch, the completion callback would NOT be run and hence
future sync reconfigurations may never complete, leading to weird
behavior including UI implications (missing updates), UMA metric
biases (error-related scenarios may be underrepresented) and issues
related custom passphrase encryption (which may never start or never
complete, observed in tests and the reason why this bug got surfaced).

Bug:  898453 , 870624 
Change-Id: I47a9c04ab34ac48e0fdf000df2acb70bbf3e4446
Reviewed-on: https://chromium-review.googlesource.com/c/1297419
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602300}
[modify] https://crrev.com/409ffd3edd8afc17d67b1eb6736793e0805aded7/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/409ffd3edd8afc17d67b1eb6736793e0805aded7/components/sync/driver/model_type_controller_unittest.cc

Project Member

Comment 30 by bugdroid1@chromium.org, Oct 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/68de3993386cbbd04e28a061df088a8ea1dfa680

commit 68de3993386cbbd04e28a061df088a8ea1dfa680
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Oct 29 14:28:20 2018

Fix sync controllers outliving their dependencies

We've observed some crashes during shutdown, presumably because the
datatype controllers outlive the KeyedServices they depend on. Instead,
let's destroy all controllers during the KeyedService's Shutdown().

Bug:  870624 
Change-Id: I50aac6207871f51cfabeedc574043a9f32690c1f
Reviewed-on: https://chromium-review.googlesource.com/c/1303363
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603487}
[modify] https://crrev.com/68de3993386cbbd04e28a061df088a8ea1dfa680/components/browser_sync/profile_sync_service.cc

Labels: Rollout-Type-Default
Bulk Edit: Updating Rollout-Type to Default since no Launch/Target Exp bits are set. 
Please update Rollout-Type to Finch and include Launch/Target Exp bits if not intended be launched as default  

Labels: -Type-Launch Type-Feature
Blocking: 902704
Project Member

Comment 34 by bugdroid1@chromium.org, Nov 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b64e8b320d41014ec18c529ea30661944a732ea5

commit b64e8b320d41014ec18c529ea30661944a732ea5
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Nov 08 13:53:40 2018

Untrack/drop delete directives only if committed

syncer::WriteNode::Drop() guarantees that if the entity hasn't been
committed to the server, the call gets ignored (i.e. the entity isn't
actually dropped).

Let's mimic the same behavior in the pseudo-USS codepath.
Unfortunately, in order to do so, interface ModelTypeChangeProcessor
needs to be extended.

A TODO is introduced to improve this implementation in the future, when
pseudo-USS is fully launched. This logic could be simplified a lot as
well as the overall complexity of the SyncableService (logic
implemented in DeleteDirectiveHandler) if everything was moved to
history's backend thread.

Bug:  870624 
Change-Id: Ic064d809371c49f4281d986b4b30bdb3fa37844a
Reviewed-on: https://chromium-review.googlesource.com/c/1326001
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606452}
[modify] https://crrev.com/b64e8b320d41014ec18c529ea30661944a732ea5/components/sync/model/fake_model_type_change_processor.cc
[modify] https://crrev.com/b64e8b320d41014ec18c529ea30661944a732ea5/components/sync/model/fake_model_type_change_processor.h
[modify] https://crrev.com/b64e8b320d41014ec18c529ea30661944a732ea5/components/sync/model/mock_model_type_change_processor.cc
[modify] https://crrev.com/b64e8b320d41014ec18c529ea30661944a732ea5/components/sync/model/mock_model_type_change_processor.h
[modify] https://crrev.com/b64e8b320d41014ec18c529ea30661944a732ea5/components/sync/model/model_type_change_processor.h
[modify] https://crrev.com/b64e8b320d41014ec18c529ea30661944a732ea5/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/b64e8b320d41014ec18c529ea30661944a732ea5/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/b64e8b320d41014ec18c529ea30661944a732ea5/components/sync/model_impl/syncable_service_based_bridge.cc
[modify] https://crrev.com/b64e8b320d41014ec18c529ea30661944a732ea5/components/sync/model_impl/syncable_service_based_bridge.h
[modify] https://crrev.com/b64e8b320d41014ec18c529ea30661944a732ea5/components/sync/model_impl/syncable_service_based_bridge_unittest.cc

Project Member

Comment 35 by bugdroid1@chromium.org, Nov 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a33f61f6bb32fad90bc5d92d96dd841e567b62da

commit a33f61f6bb32fad90bc5d92d96dd841e567b62da
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Nov 08 14:34:54 2018

Record association time in UMA for pseudo-USS too

The pseudo-USS codepath now implements the same UMA metric as the legacy
codepath (SharedChangeProcessor), representing the time required to run
association per datatype (SyncableService::MergeDataAndStartSyncing()).

We are not only interested in the exact timings, but also the *number*
of them, which should be similar.

Bug:  870624 
Change-Id: If734966e37cda71b9f2edde3ea4ef8fc4779073b
Reviewed-on: https://chromium-review.googlesource.com/c/1326143
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606459}
[modify] https://crrev.com/a33f61f6bb32fad90bc5d92d96dd841e567b62da/components/sync/model_impl/syncable_service_based_bridge.cc
[modify] https://crrev.com/a33f61f6bb32fad90bc5d92d96dd841e567b62da/components/sync/model_impl/syncable_service_based_bridge.h

Project Member

Comment 36 by bugdroid1@chromium.org, Nov 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b4786aabb184c472864a057054b7c2821fec3908

commit b4786aabb184c472864a057054b7c2821fec3908
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Nov 09 22:12:09 2018

Add about flags for pseudo-USS datatypes

Each flag maps to one feature toggle representing one (usually) or two
(rarely) sync datatypes.

If enabled, the sync datatype(s) are wrapped within the USS
architecture, based on the so-called pseudo-USS approach
(SyncableServiceBasedBridge).

The functionality has been sanity-checked for crashes on dev&canary
already, although some resulting UMA is suspicious. The flags should
allow easier debugging and testing.

Bug:  870624 
Change-Id: Id5762c01f7232de8dda4e524083fae59f66217a2
Reviewed-on: https://chromium-review.googlesource.com/c/1329142
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606991}
[modify] https://crrev.com/b4786aabb184c472864a057054b7c2821fec3908/chrome/browser/about_flags.cc
[modify] https://crrev.com/b4786aabb184c472864a057054b7c2821fec3908/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/b4786aabb184c472864a057054b7c2821fec3908/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/b4786aabb184c472864a057054b7c2821fec3908/tools/metrics/histograms/enums.xml

Project Member

Comment 37 by bugdroid1@chromium.org, Nov 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/db43c565c69f9eac701e1243afb4e36da01fd137

commit db43c565c69f9eac701e1243afb4e36da01fd137
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Nov 22 16:52:05 2018

Add sync pseudo-USS flags for iOS too

Analogous to https://chromium-review.googlesource.com/c/1329142 but with
the subset of datatypes that are relevant on iOS.

Bug:  870624 
Change-Id: Id10165cd3baca9d0b17c451804b65c87cfa4e29e
Reviewed-on: https://chromium-review.googlesource.com/c/1347273
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610443}
[modify] https://crrev.com/db43c565c69f9eac701e1243afb4e36da01fd137/ios/chrome/browser/about_flags.mm
[modify] https://crrev.com/db43c565c69f9eac701e1243afb4e36da01fd137/ios/chrome/browser/ios_chrome_flag_descriptions.cc
[modify] https://crrev.com/db43c565c69f9eac701e1243afb4e36da01fd137/ios/chrome/browser/ios_chrome_flag_descriptions.h

Project Member

Comment 38 by bugdroid1@chromium.org, Nov 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0d2e78fae5a4a03d4a78e993f85ec0a70eb88c26

commit 0d2e78fae5a4a03d4a78e993f85ec0a70eb88c26
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Nov 26 13:38:32 2018

Move parsing of protos away from UI thread for pseudo-USS

We've seen reports (in non-pseudo-USS datatypes, e.g.
https://bugs.chromium.org/p/chromium/issues/detail?id=902203) that
suggest proto-parsing can take noticeable time if executed on the UI
thread.

Since this will be a common situation for most sync datatypes, let's add
a convenience function in ModelTypeStore itself and adopt it for
pseudo-USS types (SyncableServiceBasedBridge).

Bug:  870624 
Change-Id: Id9cee20c7f384ac51d4f5353b4aae831b5e74749
Reviewed-on: https://chromium-review.googlesource.com/c/1349313
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610838}
[modify] https://crrev.com/0d2e78fae5a4a03d4a78e993f85ec0a70eb88c26/components/sync/model/model_type_store.h
[modify] https://crrev.com/0d2e78fae5a4a03d4a78e993f85ec0a70eb88c26/components/sync/model/model_type_store_test_util.cc
[modify] https://crrev.com/0d2e78fae5a4a03d4a78e993f85ec0a70eb88c26/components/sync/model_impl/model_type_store_impl.cc
[modify] https://crrev.com/0d2e78fae5a4a03d4a78e993f85ec0a70eb88c26/components/sync/model_impl/model_type_store_impl.h
[modify] https://crrev.com/0d2e78fae5a4a03d4a78e993f85ec0a70eb88c26/components/sync/model_impl/model_type_store_impl_unittest.cc
[modify] https://crrev.com/0d2e78fae5a4a03d4a78e993f85ec0a70eb88c26/components/sync/model_impl/syncable_service_based_bridge.cc
[modify] https://crrev.com/0d2e78fae5a4a03d4a78e993f85ec0a70eb88c26/components/sync/model_impl/syncable_service_based_bridge.h

Status: Fixed (was: Started)

Sign in to add a comment