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

Issue 823721 link

Starred by 4 users
Status: Fixed
Owner:
jkrcal@chromium.org
Closed: Jul 12
Cc:
mastiz@chromium.org
s...@chromium.org
zea@chromium.org
pav...@chromium.org
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

[USS] USS datatypes do not support MIGRATION_DONE.

Project Member Reported by li...@yandex-team.ru, Mar 20 2018 
UserAgent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 YaBrowser/18.2.0.234 (beta) Yowser/2.5 Safari/537.36

Steps to reproduce the problem:
While sync client process MIGRATION_DONE response from sync server it purges sync metadata for migrated datatypes.

For directory data type metadata are purged by ModelTypeConfigurer::ConfigureDataTypes.
But for USS data type only ModelTypeSyncBridge::DisableSync could purge metadata.

ModelTypeController calls ModelTypeSyncBridge::DisableSync only when datatype was removed from preferred types. This does not happen for MIGRATION_DONE processing.

What is the expected behavior?

What went wrong?
Metadata are not purged on client in MIGRATION_DONE processing, and server continues to send MIGRATION_DONE in next update cycle.

Did this work before? N/A 

Chrome version: 67.0.3377.0  Channel: dev
OS Version: 10.0
Flash Version:

Comment 1 by a-...@yandex-team.ru, Mar 20 2018

Cc: mastiz@chromium.org zea@chromium.org s...@chromium.org pav...@chromium.org
Adding sync owners to CC: mastiz@chromium.org, pavely@chromium.org, skym@chromium.org, zea@chromium.org

We believe that we found problem in USS handling of MIGRATION_DONE. We encountered it trying to issue MIGRATION_DONE from our sync server. We will create CL with fix in a couple of day.

Comment 2 by jkrcal@chromium.org, Mar 20 2018

Cc: -mastiz@chromium.org
Owner: mastiz@chromium.org
Status: Assigned (was: Unconfirmed)

Comment 3 by jkrcal@chromium.org, Mar 20 2018

Labels: Sync-Triaged

Comment 4 by mastiz@chromium.org, Apr 10 2018 

Issue 829611 has been merged into this issue.

Comment 5 by mastiz@chromium.org, Apr 10 2018

Owner: jkrcal@chromium.org

Comment 6 by jkrcal@chromium.org, Apr 11 2018 

a-v-y@yandex-team.ru, lixan@yandex-team.ru: Thanks for the report!

Are you working on a patch? If yes, I am happy to review it. If not, please let me know so that I can prioritize the fix on our side.

Comment 7 by li...@yandex-team.ru, Apr 11 2018 

We have found that problem is bigger.
CLIENT_DATA_OBSOLETE processing also doesn’t work for USS types.

And even more, if we have some error in metadata table, it couldn’t be purged when we disable datatype.

I have made CL for MIGRATION_DONE, but it doesn’t solve all this problems.

Comment 8 by jkrcal@chromium.org, Apr 18 2018

Cc: mastiz@chromium.org
Thanks Aleksei! I am sorry for the delay with reviewing the CL.

Comment 9 by treib@chromium.org, Jun 7 2018 

jkrcal/mastiz: Are there any updates on this? I believe we'd like to deprecate and eventually remove MIGRATION_DONE completely?

Comment 10 by mastiz@chromium.org, Jun 7 2018 

There's ongoing discussions in code reviews and we expect progress in the following days.
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 7 2018 

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

commit 8a4fe8a75d13c7c1b8cfc1dc399b7b5eed086ba8
Author: Aleksei Loshkarev <lixan@yandex-team.ru>
Date: Thu Jun 07 12:35:11 2018

Removes DisableSync and DISABLED misleading

Now in ModelTypeController we have two differrent meanings
for "disable".
1. State::DISABLED points sync error in model associating.
2. ModelTypeControllerDelegate::DisableSync disables sync
   and clears sync metadata for type.
This CL transforms State::DISABLED to State::FAILED and
removes misleading.

Bug:  823721 
Change-Id: I018559ebd98ad4af093be559b35a702bd11f656a
Reviewed-on: https://chromium-review.googlesource.com/1089335
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Aleksei Loshkarev <lixan@yandex-team.ru>
Cr-Commit-Position: refs/heads/master@{#565239}
[modify] https://crrev.com/8a4fe8a75d13c7c1b8cfc1dc399b7b5eed086ba8/components/search_engines/search_engine_data_type_controller_unittest.cc
[modify] https://crrev.com/8a4fe8a75d13c7c1b8cfc1dc399b7b5eed086ba8/components/sync/driver/async_directory_type_controller.cc
[modify] https://crrev.com/8a4fe8a75d13c7c1b8cfc1dc399b7b5eed086ba8/components/sync/driver/async_directory_type_controller_unittest.cc
[modify] https://crrev.com/8a4fe8a75d13c7c1b8cfc1dc399b7b5eed086ba8/components/sync/driver/data_type_controller.cc
[modify] https://crrev.com/8a4fe8a75d13c7c1b8cfc1dc399b7b5eed086ba8/components/sync/driver/data_type_controller.h
[modify] https://crrev.com/8a4fe8a75d13c7c1b8cfc1dc399b7b5eed086ba8/components/sync/driver/fake_data_type_controller.cc
[modify] https://crrev.com/8a4fe8a75d13c7c1b8cfc1dc399b7b5eed086ba8/components/sync/driver/frontend_data_type_controller.cc
[modify] https://crrev.com/8a4fe8a75d13c7c1b8cfc1dc399b7b5eed086ba8/components/sync/driver/frontend_data_type_controller_unittest.cc
[modify] https://crrev.com/8a4fe8a75d13c7c1b8cfc1dc399b7b5eed086ba8/components/sync_bookmarks/bookmark_data_type_controller_unittest.cc

Comment 12 by li...@yandex-team.ru, Jun 8 2018 

trieb: We use MIGRATION_DONE response when sync server detects wrong client metadata.
Project Member

Comment 13 by bugdroid1@chromium.org, Jun 8 2018 

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

commit e725098493b5f6c7d21c55706428eb84ef77892f
Author: Aleksei Loshkarev <lixan@yandex-team.ru>
Date: Fri Jun 08 09:45:36 2018

Propogates ShutdownReason to DataTypeManger::Stop

We need to know in DataTypeManager why we stopping.
It will helps us determine if we need to clear sync metadata on data type stop.

This is first CL in sequence.

Bug:  823721 
Change-Id: I7c20febbfc8ed820018f583f6de16bf3e269333a
Reviewed-on: https://chromium-review.googlesource.com/1091752
Commit-Queue: Aleksei Loshkarev <lixan@yandex-team.ru>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565598}
[modify] https://crrev.com/e725098493b5f6c7d21c55706428eb84ef77892f/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/e725098493b5f6c7d21c55706428eb84ef77892f/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/e725098493b5f6c7d21c55706428eb84ef77892f/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/e725098493b5f6c7d21c55706428eb84ef77892f/components/sync/driver/data_type_manager.h
[modify] https://crrev.com/e725098493b5f6c7d21c55706428eb84ef77892f/components/sync/driver/data_type_manager_impl.cc
[modify] https://crrev.com/e725098493b5f6c7d21c55706428eb84ef77892f/components/sync/driver/data_type_manager_impl.h
[modify] https://crrev.com/e725098493b5f6c7d21c55706428eb84ef77892f/components/sync/driver/data_type_manager_impl_unittest.cc
[modify] https://crrev.com/e725098493b5f6c7d21c55706428eb84ef77892f/components/sync/driver/data_type_manager_mock.h
Project Member

Comment 14 by bugdroid1@chromium.org, Jun 17 2018 

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

commit 1333bf9732bbf928df2d75d62bfef3791cb097de
Author: Aleksei Loshkarev <lixan@yandex-team.ru>
Date: Sun Jun 17 17:18:34 2018

Adds metadata_fate to DataTypeController::Stop

Now ModelTypeController:Stop uses preferences to determine if data type
will be disable completely.
This CL introduce adds metadata_fate parameter which will be used
as signal for clearing metadata.

This is second CL in sequence.

Bug:  823721 
Change-Id: Idfe4a70864e2f05aba9fc1ede3b0ccc2a108287a
Reviewed-on: https://chromium-review.googlesource.com/1093318
Commit-Queue: Aleksei Loshkarev <lixan@yandex-team.ru>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567913}
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/search_engines/search_engine_data_type_controller_unittest.cc
[add] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/base/sync_stop_metadata_fate.h
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/async_directory_type_controller.cc
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/async_directory_type_controller.h
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/async_directory_type_controller_mock.h
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/async_directory_type_controller_unittest.cc
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/data_type_controller.h
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/data_type_manager_impl.cc
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/data_type_manager_impl_unittest.cc
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/fake_data_type_controller.cc
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/fake_data_type_controller.h
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/frontend_data_type_controller.cc
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/frontend_data_type_controller.h
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/frontend_data_type_controller_mock.h
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/frontend_data_type_controller_unittest.cc
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/model_association_manager.cc
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/model_association_manager.h
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/model_association_manager_unittest.cc
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/model_type_controller.h
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/model_type_controller_unittest.cc
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/proxy_data_type_controller.cc
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync/driver/proxy_data_type_controller.h
[modify] https://crrev.com/1333bf9732bbf928df2d75d62bfef3791cb097de/components/sync_bookmarks/bookmark_data_type_controller_unittest.cc
Project Member

Comment 15 by bugdroid1@chromium.org, Jun 20 2018 

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

commit 3b74efab422dce95e1b0ee0c11e96c0cfbe335bd
Author: Aleksei Loshkarev <lixan@yandex-team.ru>
Date: Wed Jun 20 09:35:43 2018

Fixes MIGRATION_DONE processing for USS datatypes

Removes from ModelTypeController preferences dependent logic
for purging sync metadata.

Now ModelAssociationManager advises data type controllers
when is it necessery to purge sync metadata.

Bug:  823721 
Change-Id: Ib4e73fde912accc6b84188d0f21c3df724000f5e
Reviewed-on: https://chromium-review.googlesource.com/1008303
Commit-Queue: Aleksei Loshkarev <lixan@yandex-team.ru>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568775}
[modify] https://crrev.com/3b74efab422dce95e1b0ee0c11e96c0cfbe335bd/components/sync/BUILD.gn
[add] https://crrev.com/3b74efab422dce95e1b0ee0c11e96c0cfbe335bd/components/sync/base/sync_stop_metadata_fate.cc
[modify] https://crrev.com/3b74efab422dce95e1b0ee0c11e96c0cfbe335bd/components/sync/base/sync_stop_metadata_fate.h
[modify] https://crrev.com/3b74efab422dce95e1b0ee0c11e96c0cfbe335bd/components/sync/driver/data_type_manager_impl.cc
[modify] https://crrev.com/3b74efab422dce95e1b0ee0c11e96c0cfbe335bd/components/sync/driver/data_type_manager_impl_unittest.cc
[modify] https://crrev.com/3b74efab422dce95e1b0ee0c11e96c0cfbe335bd/components/sync/driver/model_association_manager.cc
[modify] https://crrev.com/3b74efab422dce95e1b0ee0c11e96c0cfbe335bd/components/sync/driver/model_association_manager.h
[modify] https://crrev.com/3b74efab422dce95e1b0ee0c11e96c0cfbe335bd/components/sync/driver/model_association_manager_unittest.cc
[modify] https://crrev.com/3b74efab422dce95e1b0ee0c11e96c0cfbe335bd/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/3b74efab422dce95e1b0ee0c11e96c0cfbe335bd/components/sync/driver/model_type_controller.h
[modify] https://crrev.com/3b74efab422dce95e1b0ee0c11e96c0cfbe335bd/components/sync/driver/model_type_controller_unittest.cc

Comment 16 by mastiz@chromium.org, Jul 3 

lixan@yandex-team.ru: Should we mark this as fixed? Is there any further work to do?

Comment 17 by li...@yandex-team.ru, Jul 12

Status: Fixed (was: Assigned)
►

Sign in to add a comment