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

Issue 862942 link

Starred by 1 user
Status: Fixed
Owner:
mastiz@chromium.org
Closed: Jul 20
Cc:
mastiz@chromium.org
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Remove home-grown singleton maps to ModelTypeStore

Project Member Reported by mastiz@chromium.org, Jul 12 
ModelTypeStoreImpl and ModelTypeStoreBackend internally maintain singleton maps to achieve what KeyedServices are designed for: have single instantes of certain objects per profile.

They do so with a home-grown and rather error-prone way, which is to use the profile path as a key (since two profiles cannot have the same path). Among other things, this also means the objects can outlive the profile.

Let's instead adopt a proper KeyedService for this, which makes lifetime a lot more obvious, semantics more standard (like how incognito should behave).
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 19 

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

commit 53131349c4aba96c9c9fe79319aaf44705a9f9c6
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Jul 19 08:59:58 2018

Get rid of home-grown singleton maps for ModelTypeStore

Prior to this patch, ModelTypeStoreImpl and ModelTypeStoreBackend
internally maintain singleton maps to achieve what KeyedServices are
designed for: have single instances of certain objects per profile. The
implementation was rather error-prone (e.g. to leak data across
profiles) and the object lifetime questionable.

Instead, this patch replaces them with a newly introduced keyed service,
ModelTypeStoreService, which is designed to abstract away sync's needs
to persist data. The associated semantics wrt lifetime, incognito
etc. are more standard now.

For directory, we don't migrate the code other than making the keyed
service the authoritative source for defining where data should be
persisted (FS path). We do this to minimize the diff, and because we
expect all datatypes to be eventually migrated to USS in the long term.

Bug:  862942 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ib444220ead181b6765c863703439eec18fcb0886
Reviewed-on: https://chromium-review.googlesource.com/1134774
Reviewed-by: Colin Blundell <blundell@chromium.org>
Reviewed-by: Eric Noyau <noyau@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Markus Heintz <markusheintz@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576424}
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/BUILD.gn
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/chromeos/printing/synced_printers_manager_factory.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/chromeos/printing/synced_printers_manager_unittest.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/consent_auditor/consent_auditor_factory.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/sync/chrome_sync_client.h
[add] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/sync/model_type_store_service_factory.cc
[add] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/sync/model_type_store_service_factory.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/sync/profile_sync_service_factory.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/sync/profile_sync_test_util.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/sync/test/integration/single_client_directory_sync_test.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/chrome/browser/sync/user_event_service_factory.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/browser_sync/profile_sync_test_util.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/browser_sync/profile_sync_test_util.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/BUILD.gn
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/driver/fake_sync_client.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/driver/fake_sync_client.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/driver/sync_client.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/driver/sync_client_mock.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model/DEPS
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model/blocking_model_type_store.h
[delete] https://crrev.com/cd0d9861ee13bec2f79b6be872f8e7aa3ab759f8/components/sync/model/model_type_store.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model/model_type_store.h
[add] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model/model_type_store_service.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model/model_type_store_test_util.cc
[add] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model/test_model_type_store_service.cc
[add] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model/test_model_type_store_service.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model_impl/blocking_model_type_store_impl.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model_impl/blocking_model_type_store_impl.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model_impl/model_type_store_backend.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model_impl/model_type_store_backend.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model_impl/model_type_store_backend_unittest.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model_impl/model_type_store_impl.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model_impl/model_type_store_impl.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model_impl/model_type_store_impl_unittest.cc
[add] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model_impl/model_type_store_service_impl.cc
[add] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/components/sync/model_impl/model_type_store_service_impl.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/ios/chrome/browser/browser_state/browser_state_keyed_service_factories.mm
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/ios/chrome/browser/reading_list/reading_list_model_factory.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/ios/chrome/browser/sync/BUILD.gn
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/ios/chrome/browser/sync/consent_auditor_factory.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/ios/chrome/browser/sync/ios_chrome_profile_sync_test_util.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/ios/chrome/browser/sync/ios_chrome_sync_client.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/ios/chrome/browser/sync/ios_chrome_sync_client.mm
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/ios/chrome/browser/sync/ios_user_event_service_factory.cc
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/ios/chrome/browser/sync/ios_user_event_service_factory.h
[add] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/ios/chrome/browser/sync/model_type_store_service_factory.cc
[add] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/ios/chrome/browser/sync/model_type_store_service_factory.h
[modify] https://crrev.com/53131349c4aba96c9c9fe79319aaf44705a9f9c6/ios/chrome/browser/sync/profile_sync_service_factory.cc

Comment 2 by mastiz@chromium.org, Jul 20

Status: Fixed (was: Started)
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 1 

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

commit fb6d11516df6ffe68cbf167fb305de7817ce86d4
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Aug 01 19:25:51 2018

Avoid reference counting for ModelTypeStore et al

We don't actually make use of the reference counting, so let's simplify
lifetime and adopt a single ownership model.

In the new implementation, we also enforce sequence affinity during
object destruction, because it's likely to catch future bugs where
future datatypes adopt BlockingModelTypeStore directly.

The changes affect a few tests which assumed that two in-memory stores
coexisting in time would share the same underlying data, which is
quite brittle and counter intuitive.

Bug:  862942 

# Bypass PRESUBMIT script bug, see  crbug.com/867823 
No-Presubmit: true

Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I98b754dd63d6bca65fe79769ae3a105e10328dbc
Reviewed-on: https://chromium-review.googlesource.com/1142778
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579896}
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/consent_auditor/consent_sync_bridge_impl.cc
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/consent_auditor/consent_sync_bridge_impl.h
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/consent_auditor/consent_sync_bridge_impl_unittest.cc
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model/model_type_store_test_util.cc
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model/model_type_store_test_util.h
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model/test_model_type_store_service.cc
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model/test_model_type_store_service.h
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model_impl/blocking_model_type_store_impl.cc
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model_impl/blocking_model_type_store_impl.h
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model_impl/model_type_store_backend.cc
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model_impl/model_type_store_backend.h
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model_impl/model_type_store_backend_unittest.cc
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model_impl/model_type_store_impl.cc
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model_impl/model_type_store_impl.h
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model_impl/model_type_store_service_impl.cc
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/model_impl/model_type_store_service_impl.h
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/user_events/user_event_sync_bridge.cc
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/user_events/user_event_sync_bridge.h
[modify] https://crrev.com/fb6d11516df6ffe68cbf167fb305de7817ce86d4/components/sync/user_events/user_event_sync_bridge_unittest.cc
►

Sign in to add a comment