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

Issue 890361 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Make sure SyncStandaloneTransport doesn't clear all data

Project Member Reported by treib@chromium.org, Sep 28

Issue description

ModelAssociationManager::Initialize (which gets called during datatype (re)configuration) stops all datatypes which are not "desired" anymore. If a type is also not "preferred", then it also clears its data.

When switching from full Sync into standalone transport mode, most types become no longer desired/preferred. We need to make sure that in this case, we don't blow away all the Sync data. This is particularly important on Android, where some users regularly disable the Sync feature toggle: When they later turn Sync back on, we don't wanna have to re-download everything.

I'm not sure what actually happens at the moment. Need to write some tests first.
 
Labels: butter-hotlist
Verified manually: If sync-the-transport starts up, it *does* clear all other data :(
Labels: -Pri-2 Pri-1
I think there are at least two places that need changes:
1) ModelAssociationManager::Initialize will stop all data types (with CLEAR_DATA) which are not "preferred" anymore.
2) SyncManagerImpl::PurgeDisabledTypes gets called for all disabled types via DataTypeManagerImpl::StartNextDownload -> SyncBackendHostImpl::ConfigureDataTypes.
Status: Started (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 9

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

commit ab8b2509884ff6cb9a1192e66f27f66bc7b50f26
Author: Marc Treib <treib@chromium.org>
Date: Tue Oct 09 06:34:28 2018

Sync: don't purge sync data in ephemeral mode

When the user temporarily disables Sync (e.g. using the Sync feature
toggle on Android), we keep the Sync data around so that we don't have
to re-download everything once Sync is started again.

However, if the Sync machinery then restarted in standalone-transport
mode, we ended up clearing everything after all, because we purged all
data for all not-enabled data types (which in standalone-transport mode
means most of them).
This CL fixes that by *not* purging data when starting Sync in
standalone-transport mode.

Bug:  890361 
Change-Id: Ice49b73a7bc13cf09f1b960896fe9bccb2a06204
Reviewed-on: https://chromium-review.googlesource.com/c/1256247
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597818}
[modify] https://crrev.com/ab8b2509884ff6cb9a1192e66f27f66bc7b50f26/components/sync/driver/data_type_manager_impl.cc
[modify] https://crrev.com/ab8b2509884ff6cb9a1192e66f27f66bc7b50f26/components/sync/driver/data_type_manager_impl_unittest.cc
[modify] https://crrev.com/ab8b2509884ff6cb9a1192e66f27f66bc7b50f26/components/sync/driver/model_association_manager.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 9

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

commit e5d25c33093cbe9a1040a311a08685bee98bbb62
Author: Marc Treib <treib@chromium.org>
Date: Tue Oct 09 09:12:54 2018

Sync: Add tests for switching storage option in ModelAssociationManager

In particular: When switching storage option, all data types should get
restarted. If switching to in-memory storage, then metadata should be
kept around even for no-longer-preferred data types.

Bug:  890361 , 885211
Change-Id: Id0c7784ddcda08dc20d0b3c472bd6f6d1c118a7a
Reviewed-on: https://chromium-review.googlesource.com/c/1268058
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597850}
[modify] https://crrev.com/e5d25c33093cbe9a1040a311a08685bee98bbb62/components/sync/driver/model_association_manager_unittest.cc

Status: Fixed (was: Started)
This should do it!
There are unit tests, but integration tests are still missing. I've filed  bug 893496  for that.

Sign in to add a comment