Make sure Wallet USS bridge clears the data on sign out |
|||
Issue descriptionBased on treib@'s comment on crbug.com/885211, we should make sure that the Wallet data and metadata get correctly removed on sign-out in the new USS implementation.
,
Sep 28
AFAICT, AutofillWalletSyncBridge::ApplyStopSyncChanges *does* remove the cards, by calling AutofillTable::SetServerCreditCards. But somehow that doesn't make it to PersonalDataManager.
,
Sep 28
Hah - turns out the cards *do* get removed. But pre-USS, the removal happened synchronously, by AutofillWalletDataTypeController calling PersonalDataManager::ClearAllServerData. With USS, it happens asynchronously, by the bridge deleting stuff from the DB, which will eventually make it to the PDM. If the async-ness is okay (I guess it probably is), then all that needs to be done is make the tests wait for the change to arrive, probably with WaitForOnPersonalDataChanged.
,
Sep 28
,
Sep 28
Thanks treib@, your explanation makes sense. As long as after the data is being cleared, the PDM does get refreshed I think it's fine. WaitForOnPersonalDataChanged should let us know that.
,
Sep 28
This just keeps getting better :) Effectively, SyncService::RequestStop(KEEP_DATA) (i.e. what Android's Sync feature toggle does) is broken with Wallet USS. Here's what happens: RequestStop(KEEP_DATA) results (via many steps) in a call to AutofillWalletSyncBridge::ApplyStopSyncChanges with a null change_list. So this will *not* clear the cards. Currently they do still get cleared because the AutofillWalletDataTypeController for AUTOFILL_WALLET_METADATA happens to do it (which as mentioned above should be changed). But we're now in an inconsistent state: We've thrown away the actual card data, but the sync metadata is still there. That means when Sync later gets turned back on, we won't re-download them. I think even a Chrome restart won't fix that. Pre-USS, everything works out because the Sync Directory still has a copy of the actual cards.
,
Sep 28
,
Oct 1
,
Oct 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/611a195c2fe44a714986b569d55ab7993b7c8058 commit 611a195c2fe44a714986b569d55ab7993b7c8058 Author: Marc Treib <treib@chromium.org> Date: Tue Oct 02 08:51:59 2018 SyncCustomizationFragmentTest: don't use PDM.addServerCreditCardForTest PersonalDataManager.addServerCreditCardForTest adds a server credit card directly to PDM's cache, without going through Sync. That creates an inconsistent state, which makes the tests fragile to unrelated changes (e.g. https://crrev.com/c/1251041). Instead, this CL uses FakeServer::SetWalletData, which needed to be plumbed through to Java. Bug: 889941 Change-Id: Ie70a2f8ca311d36267c9aa16dca9363b60b4ee2c Reviewed-on: https://chromium-review.googlesource.com/1255062 Reviewed-by: Mikel Astiz <mastiz@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#595764} [modify] https://crrev.com/611a195c2fe44a714986b569d55ab7993b7c8058/chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/FakeServerHelper.java [modify] https://crrev.com/611a195c2fe44a714986b569d55ab7993b7c8058/chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/SyncCustomizationFragmentTest.java [modify] https://crrev.com/611a195c2fe44a714986b569d55ab7993b7c8058/components/sync/test/fake_server/android/fake_server_helper_android.cc [modify] https://crrev.com/611a195c2fe44a714986b569d55ab7993b7c8058/components/sync/test/fake_server/android/fake_server_helper_android.h
,
Oct 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3efff7c9d8c1e53a894a9bf516615a5c7f9b256b commit 3efff7c9d8c1e53a894a9bf516615a5c7f9b256b Author: Marc Treib <treib@chromium.org> Date: Tue Oct 02 10:49:58 2018 Consistently clear server credit cards with USS AutofillWalletDataTypeController is used (pre-USS) for both AUTOFILL_WALLET_DATA and AUTOFILL_WALLET_METADATA. When Sync is stopped, it calls PersonalDataManager::ClearAllServerData. Since there are two instances of the controller, that happens twice (which is unnecessary but doesn't hurt). However, when AUTOFILL_WALLET_DATA is on USS, then the remaining ClearAllServerData call (for the metadata controller) still happens, which brings Sync into a bad state since the data is now gone, but the progress marker is still there (see bug 885211). This CL changes AutofillWalletDataTypeController to only call ClearAllServerData for AUTOFILL_WALLET_DATA (i.e. it won't be called anymore if Wallet is on USS). Instead, AUTOFILL_WALLET_DATA gets special-cased in ModelAssociationManager to clear even in the STOP_SYNC case. This achieves the same "clear data on stopping sync, but *not* on browser shutdown", but without requiring a special side channel to the SyncService. One consequence is that with AUTOFILL_WALLET_DATA on USS, the clearing now happens asynchronously (because it goes through the DB), which requires some test changes. Bug: 889941 Change-Id: I54c36050c81fa862acfc0052f355ff1645a8b292 Reviewed-on: https://chromium-review.googlesource.com/1251041 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Reviewed-by: Jan Krcal <jkrcal@chromium.org> Reviewed-by: Florian Uunk <feuunk@chromium.org> Cr-Commit-Position: refs/heads/master@{#595787} [modify] https://crrev.com/3efff7c9d8c1e53a894a9bf516615a5c7f9b256b/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc [modify] https://crrev.com/3efff7c9d8c1e53a894a9bf516615a5c7f9b256b/chrome/browser/sync/test/integration/two_client_polling_sync_test.cc [modify] https://crrev.com/3efff7c9d8c1e53a894a9bf516615a5c7f9b256b/components/autofill/core/browser/autofill_wallet_data_type_controller.cc [modify] https://crrev.com/3efff7c9d8c1e53a894a9bf516615a5c7f9b256b/components/sync/BUILD.gn [modify] https://crrev.com/3efff7c9d8c1e53a894a9bf516615a5c7f9b256b/components/sync/driver/data_type_manager_impl.cc [modify] https://crrev.com/3efff7c9d8c1e53a894a9bf516615a5c7f9b256b/components/sync/driver/model_association_manager.cc [modify] https://crrev.com/3efff7c9d8c1e53a894a9bf516615a5c7f9b256b/components/sync/driver/model_association_manager.h [modify] https://crrev.com/3efff7c9d8c1e53a894a9bf516615a5c7f9b256b/components/sync/driver/model_association_manager_unittest.cc [add] https://crrev.com/3efff7c9d8c1e53a894a9bf516615a5c7f9b256b/components/sync/engine/shutdown_reason.cc [modify] https://crrev.com/3efff7c9d8c1e53a894a9bf516615a5c7f9b256b/components/sync/engine/shutdown_reason.h
,
Oct 2
This should fix the bug. Left to do: Move the special-casing for AUTOFILL_WALLET_DATA from the generic ModelAssociationManager into the Wallet-specific DataTypeController.
,
Oct 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eaacef69656c818e6e9c03186c9d7d55bd187e97 commit eaacef69656c818e6e9c03186c9d7d55bd187e97 Author: Findit <findit-for-me@appspot.gserviceaccount.com> Date: Tue Oct 02 14:46:15 2018 Revert "Consistently clear server credit cards with USS" This reverts commit 3efff7c9d8c1e53a894a9bf516615a5c7f9b256b. Reason for revert: Findit (https://goo.gl/kROfz5) identified CL at revision 595787 as the culprit for flakes in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/flake/flake-culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vM2VmZmY3YzlkOGMxZTUzYTg5NGE5YmY1MTY2MTVhNWM3ZjliMjU2Ygw Sample Failed Build: https://ci.chromium.org/buildbot/tryserver.chromium.win/win10_chromium_x64_rel_ng/106570 Sample Failed Step: sync_integration_tests (with patch) on Windows-10-15063 Sample Flaky Test: USS/SingleClientWalletSyncTest.ClearOnStopSync/1 Original change's description: > Consistently clear server credit cards with USS > > AutofillWalletDataTypeController is used (pre-USS) for both AUTOFILL_WALLET_DATA > and AUTOFILL_WALLET_METADATA. When Sync is stopped, it calls > PersonalDataManager::ClearAllServerData. Since there are two instances of the > controller, that happens twice (which is unnecessary but doesn't hurt). However, > when AUTOFILL_WALLET_DATA is on USS, then the remaining ClearAllServerData call > (for the metadata controller) still happens, which brings Sync into a bad state > since the data is now gone, but the progress marker is still there (see bug 885211). > > This CL changes AutofillWalletDataTypeController to only call ClearAllServerData > for AUTOFILL_WALLET_DATA (i.e. it won't be called anymore if Wallet is on USS). > Instead, AUTOFILL_WALLET_DATA gets special-cased in ModelAssociationManager to > clear even in the STOP_SYNC case. This achieves the same "clear data on stopping > sync, but *not* on browser shutdown", but without requiring a special side channel > to the SyncService. > > One consequence is that with AUTOFILL_WALLET_DATA on USS, the clearing now happens > asynchronously (because it goes through the DB), which requires some test changes. > > Bug: 889941 > Change-Id: I54c36050c81fa862acfc0052f355ff1645a8b292 > Reviewed-on: https://chromium-review.googlesource.com/1251041 > Commit-Queue: Marc Treib <treib@chromium.org> > Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> > Reviewed-by: Jan Krcal <jkrcal@chromium.org> > Reviewed-by: Florian Uunk <feuunk@chromium.org> > Cr-Commit-Position: refs/heads/master@{#595787} Change-Id: I429e5ef10c8d5b858f36ce69cc2b41b30e318fd4 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 889941 , 891254 Reviewed-on: https://chromium-review.googlesource.com/1255573 Cr-Commit-Position: refs/heads/master@{#595831} [modify] https://crrev.com/eaacef69656c818e6e9c03186c9d7d55bd187e97/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc [modify] https://crrev.com/eaacef69656c818e6e9c03186c9d7d55bd187e97/chrome/browser/sync/test/integration/two_client_polling_sync_test.cc [modify] https://crrev.com/eaacef69656c818e6e9c03186c9d7d55bd187e97/components/autofill/core/browser/autofill_wallet_data_type_controller.cc [modify] https://crrev.com/eaacef69656c818e6e9c03186c9d7d55bd187e97/components/sync/BUILD.gn [modify] https://crrev.com/eaacef69656c818e6e9c03186c9d7d55bd187e97/components/sync/driver/data_type_manager_impl.cc [modify] https://crrev.com/eaacef69656c818e6e9c03186c9d7d55bd187e97/components/sync/driver/model_association_manager.cc [modify] https://crrev.com/eaacef69656c818e6e9c03186c9d7d55bd187e97/components/sync/driver/model_association_manager.h [modify] https://crrev.com/eaacef69656c818e6e9c03186c9d7d55bd187e97/components/sync/driver/model_association_manager_unittest.cc [delete] https://crrev.com/9dff9f8d79ef8dcf10a1f16549fdc75036808ff0/components/sync/engine/shutdown_reason.cc [modify] https://crrev.com/eaacef69656c818e6e9c03186c9d7d55bd187e97/components/sync/engine/shutdown_reason.h
,
Oct 4
I can repro the flakiness (in debug builds only). The problem is some race condition related to WaitForOnPersonalDataChanged(): We only wait for *any* change, but in some cases we get a notification that something changed before the change we're waiting for. I guess the fix is to just wait again until we see the expected number of cards. It's not pretty, but I don't really see a better way.
,
Oct 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/70d853a8946d19bb210d8ccbbf8400d7627eb63d commit 70d853a8946d19bb210d8ccbbf8400d7627eb63d Author: Marc Treib <treib@chromium.org> Date: Thu Oct 04 15:01:47 2018 Reland "Consistently clear server credit cards with USS" This is a reland of 3efff7c9d8c1e53a894a9bf516615a5c7f9b256b Changes from the previous patch: - Fixed the waiting condition in tests to be reliable. TBR=sebsg for autofill_wallet_data_type_controller.cc which is unchanged since the first attempt. Original change's description: > Consistently clear server credit cards with USS > > AutofillWalletDataTypeController is used (pre-USS) for both AUTOFILL_WALLET_DATA > and AUTOFILL_WALLET_METADATA. When Sync is stopped, it calls > PersonalDataManager::ClearAllServerData. Since there are two instances of the > controller, that happens twice (which is unnecessary but doesn't hurt). However, > when AUTOFILL_WALLET_DATA is on USS, then the remaining ClearAllServerData call > (for the metadata controller) still happens, which brings Sync into a bad state > since the data is now gone, but the progress marker is still there (see bug 885211). > > This CL changes AutofillWalletDataTypeController to only call ClearAllServerData > for AUTOFILL_WALLET_DATA (i.e. it won't be called anymore if Wallet is on USS). > Instead, AUTOFILL_WALLET_DATA gets special-cased in ModelAssociationManager to > clear even in the STOP_SYNC case. This achieves the same "clear data on stopping > sync, but *not* on browser shutdown", but without requiring a special side channel > to the SyncService. > > One consequence is that with AUTOFILL_WALLET_DATA on USS, the clearing now happens > asynchronously (because it goes through the DB), which requires some test changes. > > Bug: 889941 > Change-Id: I54c36050c81fa862acfc0052f355ff1645a8b292 > Reviewed-on: https://chromium-review.googlesource.com/1251041 > Commit-Queue: Marc Treib <treib@chromium.org> > Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> > Reviewed-by: Jan Krcal <jkrcal@chromium.org> > Reviewed-by: Florian Uunk <feuunk@chromium.org> > Cr-Commit-Position: refs/heads/master@{#595787} Bug: 889941 Change-Id: I1c577212e06e39cc8fca9a39658d2bd96b6b3bed Reviewed-on: https://chromium-review.googlesource.com/c/1261083 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org> Cr-Commit-Position: refs/heads/master@{#596668} [modify] https://crrev.com/70d853a8946d19bb210d8ccbbf8400d7627eb63d/chrome/browser/sync/test/integration/single_client_wallet_sync_test.cc [modify] https://crrev.com/70d853a8946d19bb210d8ccbbf8400d7627eb63d/chrome/browser/sync/test/integration/two_client_polling_sync_test.cc [modify] https://crrev.com/70d853a8946d19bb210d8ccbbf8400d7627eb63d/components/autofill/core/browser/autofill_wallet_data_type_controller.cc [modify] https://crrev.com/70d853a8946d19bb210d8ccbbf8400d7627eb63d/components/sync/BUILD.gn [modify] https://crrev.com/70d853a8946d19bb210d8ccbbf8400d7627eb63d/components/sync/driver/data_type_manager_impl.cc [modify] https://crrev.com/70d853a8946d19bb210d8ccbbf8400d7627eb63d/components/sync/driver/model_association_manager.cc [modify] https://crrev.com/70d853a8946d19bb210d8ccbbf8400d7627eb63d/components/sync/driver/model_association_manager.h [modify] https://crrev.com/70d853a8946d19bb210d8ccbbf8400d7627eb63d/components/sync/driver/model_association_manager_unittest.cc [add] https://crrev.com/70d853a8946d19bb210d8ccbbf8400d7627eb63d/components/sync/engine/shutdown_reason.cc [modify] https://crrev.com/70d853a8946d19bb210d8ccbbf8400d7627eb63d/components/sync/engine/shutdown_reason.h
,
Oct 4
Flakiness dashboard link to watch: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=sync_integration_tests&tests=ClearOnStopSync Also the cleanup mentioned in #11 is still open.
,
Oct 8
Thanks Marc! Marking this as fixed because the root cause is now gone, the clean-up can be done as a follow-up.
,
Oct 8
,
Oct 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f97002c47cd312661fccb2e296643dced29f9a9e commit f97002c47cd312661fccb2e296643dced29f9a9e Author: Marc Treib <treib@chromium.org> Date: Mon Oct 08 12:34:50 2018 DataTypeController::Stop: Take ShutdownReason rather than SyncStopMetadataFate This is done for two reasons: 1) It'll allow us to move a Wallet-specific special case from the general datatype-agnostic code into the Wallet-specific controller (once that exists). 2) It's actually less misleading: Directory data types actually ignore this value, i.e. passing CLEAR_METADATA didn't actually clear data for them. Bug: 889941 Change-Id: I4e660e41b17142480eb8b03569077686cedaab6c Reviewed-on: https://chromium-review.googlesource.com/c/1264695 Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Reviewed-by: Jan Krcal <jkrcal@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#597539} [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/history/core/browser/sync/history_delete_directives_model_type_controller.cc [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/history/core/browser/sync/history_delete_directives_model_type_controller.h [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/search_engines/search_engine_data_type_controller_unittest.cc [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/async_directory_type_controller.cc [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/async_directory_type_controller.h [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/async_directory_type_controller_mock.h [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/async_directory_type_controller_unittest.cc [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/data_type_controller.h [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/directory_data_type_controller.cc [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/directory_data_type_controller.h [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/fake_data_type_controller.cc [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/fake_data_type_controller.h [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/frontend_data_type_controller.cc [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/frontend_data_type_controller.h [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/frontend_data_type_controller_mock.h [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/frontend_data_type_controller_unittest.cc [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/model_association_manager.cc [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/model_type_controller.cc [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/model_type_controller.h [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/model_type_controller_unittest.cc [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/proxy_data_type_controller.cc [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync/driver/proxy_data_type_controller.h [modify] https://crrev.com/f97002c47cd312661fccb2e296643dced29f9a9e/components/sync_bookmarks/bookmark_data_type_controller_unittest.cc |
|||
►
Sign in to add a comment |
|||
Comment 1 by treib@chromium.org
, Sep 28