Commit 8bb9f4d3 authored by Florian Uunk's avatar Florian Uunk Committed by Commit Bot

Make metadata only observe syncing wallet changes

When Wallet sync is inactive, we can't rely on the cards being present.
The Wallet Metadata sync integration relies on Wallet data to prune
no-longer relevant metadata entries.

This CL changes the metadata syncable service to only do this pruning of
old metadata entries when we're sure that the wallet data bridge is
running.

Bug: 890709
Change-Id: Iaad482326ae0b147bf343d71e95c3bbe3e413d99
Reviewed-on: https://chromium-review.googlesource.com/c/1273297Reviewed-by: default avatarSebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: default avatarPeter Kasting <pkasting@chromium.org>
Reviewed-by: default avatarMikel Astiz <mastiz@chromium.org>
Commit-Queue: Florian Uunk <feuunk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601590}
parent adcae34b
......@@ -115,7 +115,7 @@ void AutofillWalletMetadataSyncBridge::CreateForWebDataServiceAndBackend(
}
// static
syncer::ModelTypeSyncBridge*
AutofillWalletMetadataSyncBridge*
AutofillWalletMetadataSyncBridge::FromWebDataService(
AutofillWebDataService* web_data_service) {
return static_cast<AutofillWalletMetadataSyncBridge*>(
......@@ -128,9 +128,10 @@ AutofillWalletMetadataSyncBridge::AutofillWalletMetadataSyncBridge(
AutofillWebDataBackend* web_data_backend)
: ModelTypeSyncBridge(std::move(change_processor)),
web_data_backend_(web_data_backend),
scoped_observer_(this) {
scoped_observer_(this),
track_wallet_data_(false),
weak_ptr_factory_(this) {
DCHECK(web_data_backend_);
scoped_observer_.Add(web_data_backend_);
LoadDataCacheAndMetadata();
......@@ -138,6 +139,11 @@ AutofillWalletMetadataSyncBridge::AutofillWalletMetadataSyncBridge(
AutofillWalletMetadataSyncBridge::~AutofillWalletMetadataSyncBridge() {}
void AutofillWalletMetadataSyncBridge::OnWalletDataTrackingStateChanged(
bool is_tracking) {
track_wallet_data_ = is_tracking;
}
std::unique_ptr<syncer::MetadataChangeList>
AutofillWalletMetadataSyncBridge::CreateMetadataChangeList() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
......
......@@ -45,7 +45,7 @@ class AutofillWalletMetadataSyncBridge
AutofillWebDataBackend* webdata_backend,
AutofillWebDataService* web_data_service);
static syncer::ModelTypeSyncBridge* FromWebDataService(
static AutofillWalletMetadataSyncBridge* FromWebDataService(
AutofillWebDataService* web_data_service);
AutofillWalletMetadataSyncBridge(
......@@ -53,6 +53,14 @@ class AutofillWalletMetadataSyncBridge
AutofillWebDataBackend* web_data_backend);
~AutofillWalletMetadataSyncBridge() override;
// Determines whether this bridge should be monitoring the Wallet data. This
// should be called whenever the data bridge sync state changes.
void OnWalletDataTrackingStateChanged(bool is_tracking);
base::WeakPtr<AutofillWalletMetadataSyncBridge> GetWeakPtr() {
return weak_ptr_factory_.GetWeakPtr();
}
// ModelTypeSyncBridge implementation.
std::unique_ptr<syncer::MetadataChangeList> CreateMetadataChangeList()
override;
......@@ -103,9 +111,16 @@ class AutofillWalletMetadataSyncBridge
// happen in the local database.
std::unordered_map<std::string, sync_pb::WalletMetadataSpecifics> cache_;
// Indicates whether we should rely on wallet data being actively synced. If
// true, the bridge will prune metadata entries without corresponding wallet
// data entry.
bool track_wallet_data_;
// The bridge should be used on the same sequence where it is constructed.
SEQUENCE_CHECKER(sequence_checker_);
base::WeakPtrFactory<AutofillWalletMetadataSyncBridge> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(AutofillWalletMetadataSyncBridge);
};
......
......@@ -328,6 +328,15 @@ std::string GetServerId(const DataType& data) {
AutofillWalletMetadataSyncableService::
~AutofillWalletMetadataSyncableService() {}
void AutofillWalletMetadataSyncableService::OnWalletDataTrackingStateChanged(
bool is_tracking) {
DCHECK_NE(track_wallet_data_, is_tracking);
track_wallet_data_ = is_tracking;
if (is_tracking && sync_processor_) {
MergeData(cache_);
}
}
syncer::SyncMergeResult
AutofillWalletMetadataSyncableService::MergeDataAndStartSyncing(
syncer::ModelType type,
......@@ -344,7 +353,13 @@ AutofillWalletMetadataSyncableService::MergeDataAndStartSyncing(
cache_ = initial_sync_data;
syncer::SyncMergeResult result = MergeData(initial_sync_data);
syncer::SyncMergeResult result(syncer::AUTOFILL_WALLET_METADATA);
if (track_wallet_data_) {
result = MergeData(initial_sync_data);
}
// Notify that sync has started. This callback does not currently take into
// account whether we're actually tracking wallet data.
if (web_data_backend_)
web_data_backend_->NotifyThatSyncHasStarted(type);
return result;
......@@ -390,6 +405,12 @@ syncer::SyncError AutofillWalletMetadataSyncableService::ProcessSyncChanges(
ApplyChangesToCache(changes_from_sync, &cache_);
// If we're not tracking wallet data, we can't rely on the local wallet
// data being up-to-date, so we should not do any merging with local data.
if (!track_wallet_data_) {
return syncer::SyncError();
}
std::unordered_map<std::string, std::unique_ptr<AutofillProfile>> profiles;
std::unordered_map<std::string, std::unique_ptr<CreditCard>> cards;
GetLocalData(&profiles, &cards);
......@@ -464,6 +485,9 @@ syncer::SyncError AutofillWalletMetadataSyncableService::ProcessSyncChanges(
void AutofillWalletMetadataSyncableService::AutofillProfileChanged(
const AutofillProfileChange& change) {
DCHECK(thread_checker_.CalledOnValidThread());
if (!track_wallet_data_) {
return;
}
if (sync_processor_ && change.data_model() &&
change.data_model()->record_type() != AutofillProfile::LOCAL_PROFILE) {
......@@ -484,6 +508,9 @@ void AutofillWalletMetadataSyncableService::AutofillProfileChanged(
void AutofillWalletMetadataSyncableService::CreditCardChanged(
const CreditCardChange& change) {
DCHECK(thread_checker_.CalledOnValidThread());
if (!track_wallet_data_) {
return;
}
if (sync_processor_ && change.data_model() &&
change.data_model()->record_type() != CreditCard::LOCAL_CARD) {
......@@ -502,7 +529,7 @@ void AutofillWalletMetadataSyncableService::CreditCardChanged(
}
void AutofillWalletMetadataSyncableService::AutofillMultipleChanged() {
if (sync_processor_)
if (sync_processor_ && track_wallet_data_)
MergeData(cache_);
}
......@@ -529,7 +556,10 @@ AutofillWalletMetadataSyncableService::FromWebDataService(
AutofillWalletMetadataSyncableService::AutofillWalletMetadataSyncableService(
AutofillWebDataBackend* web_data_backend,
const std::string& app_locale)
: web_data_backend_(web_data_backend), scoped_observer_(this) {
: web_data_backend_(web_data_backend),
scoped_observer_(this),
track_wallet_data_(false),
weak_ptr_factory_(this) {
scoped_observer_.Add(web_data_backend_);
}
......@@ -580,6 +610,10 @@ AutofillWalletMetadataSyncableService::SendChangesToSyncServer(
syncer::SyncMergeResult AutofillWalletMetadataSyncableService::MergeData(
const syncer::SyncDataList& sync_data) {
// If we're not tracking wallet data, we can't rely on the local wallet
// data being up-to-date, so we should not do any merging with local data.
DCHECK(track_wallet_data_);
std::unordered_map<std::string, std::unique_ptr<AutofillProfile>> profiles;
std::unordered_map<std::string, std::unique_ptr<CreditCard>> cards;
GetLocalData(&profiles, &cards);
......
......@@ -51,6 +51,14 @@ class AutofillWalletMetadataSyncableService
public:
~AutofillWalletMetadataSyncableService() override;
// Determines whether this bridge should be monitoring the Wallet data. This
// should be called whenever the data bridge sync state changes.
void OnWalletDataTrackingStateChanged(bool is_tracking);
base::WeakPtr<AutofillWalletMetadataSyncableService> GetWeakPtr() {
return weak_ptr_factory_.GetWeakPtr();
}
// syncer::SyncableService implementation.
syncer::SyncMergeResult MergeDataAndStartSyncing(
syncer::ModelType type,
......@@ -141,6 +149,13 @@ class AutofillWalletMetadataSyncableService
// Local metadata plus metadata for the data that hasn't synced down yet.
syncer::SyncDataList cache_;
// Indicates whether we should rely on wallet data being actively synced. If
// true, the service will prune metadata entries without corresponding wallet
// data entry.
bool track_wallet_data_;
base::WeakPtrFactory<AutofillWalletMetadataSyncableService> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(AutofillWalletMetadataSyncableService);
};
......
......@@ -152,6 +152,11 @@ class AutofillWalletMetadataSyncableServiceTest : public Test {
: local_(&backend_), remote_(&backend_) {}
~AutofillWalletMetadataSyncableServiceTest() override {}
void SetUp() {
local_.OnWalletDataTrackingStateChanged(true);
remote_.OnWalletDataTrackingStateChanged(true);
}
// Outlives local_ and remote_.
NiceMock<MockAutofillWebDataBackend> backend_;
......@@ -299,6 +304,45 @@ TEST_F(AutofillWalletMetadataSyncableServiceTest, DeleteFromServerOnMerge) {
MergeMetadata(&local_, &remote_);
}
// Verify that remote data without local counterpart is kept when we're not
// tracking wallet data.
TEST_F(AutofillWalletMetadataSyncableServiceTest,
DeleteFromServerOnMerge_NotWhenNotTracking) {
local_.OnWalletDataTrackingStateChanged(false);
remote_.UpdateAddressStats(BuildAddress(kAddr1, 1, 2, true));
remote_.UpdateCardStats(BuildCard(kCard1, 3, 4, kAddr1));
EXPECT_CALL(local_, UpdateAddressStats(_)).Times(0);
EXPECT_CALL(local_, UpdateCardStats(_)).Times(0);
EXPECT_CALL(local_, SendChangesToSyncServer(_)).Times(0);
MergeMetadata(&local_, &remote_);
}
// Verify that remote data without local counterpart is deleted when we start
// tracking wallet data after the initial merge happened.
TEST_F(AutofillWalletMetadataSyncableServiceTest,
DeleteFromServerOnMerge_MergeBeforeTracking) {
local_.OnWalletDataTrackingStateChanged(false);
remote_.UpdateAddressStats(BuildAddress(kAddr1, 1, 2, true));
remote_.UpdateCardStats(BuildCard(kCard1, 3, 4, kAddr1));
EXPECT_CALL(local_, UpdateAddressStats(_)).Times(0);
EXPECT_CALL(local_, UpdateCardStats(_)).Times(0);
EXPECT_CALL(
local_,
SendChangesToSyncServer(UnorderedElementsAre(
SyncChangeMatches(syncer::SyncChange::ACTION_DELETE, kAddr1SyncTag),
SyncChangeMatches(syncer::SyncChange::ACTION_DELETE,
kCard1SyncTag))));
MergeMetadata(&local_, &remote_);
local_.OnWalletDataTrackingStateChanged(true);
}
MATCHER_P7(SyncAddressChangeAndDataMatch,
change_type,
sync_tag,
......@@ -593,6 +637,50 @@ TEST_F(AutofillWalletMetadataSyncableServiceTest,
local_.AutofillMultipleChanged();
}
// Verify that erased local metadata is not erased from the sync server when
// the service is not tracking Wallet data.
TEST_F(AutofillWalletMetadataSyncableServiceTest,
DeleteFromServerOnMultiChange_NotWhenNotTracking) {
local_.OnWalletDataTrackingStateChanged(false);
local_.UpdateAddressStats(BuildAddress(kAddr1, 1, 2, true));
local_.UpdateCardStats(BuildCard(kCard1, 3, 4, kAddr1));
remote_.UpdateAddressStats(BuildAddress(kAddr1, 1, 2, true));
remote_.UpdateCardStats(BuildCard(kCard1, 3, 4, kAddr1));
MergeMetadata(&local_, &remote_);
// This method dooes not trigger notifications or sync:
local_.ClearServerData();
EXPECT_CALL(local_, SendChangesToSyncServer(_)).Times(0);
local_.AutofillMultipleChanged();
}
// Verify that erased local metadata is also erased from the sync server when
// we start tracking Wallet data after multiple metadata values change at once.
TEST_F(AutofillWalletMetadataSyncableServiceTest,
DeleteFromServerOnMultiChange_ChangeBeforeTracking) {
local_.OnWalletDataTrackingStateChanged(false);
local_.UpdateAddressStats(BuildAddress(kAddr1, 1, 2, true));
local_.UpdateCardStats(BuildCard(kCard1, 3, 4, kAddr1));
remote_.UpdateAddressStats(BuildAddress(kAddr1, 1, 2, true));
remote_.UpdateCardStats(BuildCard(kCard1, 3, 4, kAddr1));
MergeMetadata(&local_, &remote_);
// This method dooes not trigger notifications or sync:
local_.ClearServerData();
EXPECT_CALL(
local_,
SendChangesToSyncServer(UnorderedElementsAre(
SyncChangeMatches(syncer::SyncChange::ACTION_DELETE, kAddr1SyncTag),
SyncChangeMatches(syncer::SyncChange::ACTION_DELETE,
kCard1SyncTag))));
local_.AutofillMultipleChanged();
local_.OnWalletDataTrackingStateChanged(true);
}
// Verify that empty sync change from the sync server does not trigger writing
// to disk or sending any data to the sync server.
TEST_F(AutofillWalletMetadataSyncableServiceTest, EmptySyncChange) {
......
......@@ -63,12 +63,14 @@ std::string GetClientTagForWalletDataSpecificsId(
// static
void AutofillWalletSyncBridge::CreateForWebDataServiceAndBackend(
const std::string& app_locale,
const base::RepeatingCallback<void(bool)>& active_callback,
bool has_persistent_storage,
AutofillWebDataBackend* web_data_backend,
AutofillWebDataService* web_data_service) {
web_data_service->GetDBUserData()->SetUserData(
&kAutofillWalletSyncBridgeUserDataKey,
std::make_unique<AutofillWalletSyncBridge>(
active_callback,
std::make_unique<syncer::ClientTagBasedModelTypeProcessor>(
syncer::AUTOFILL_WALLET_DATA,
/*dump_stack=*/base::RepeatingClosure()),
......@@ -84,11 +86,13 @@ syncer::ModelTypeSyncBridge* AutofillWalletSyncBridge::FromWebDataService(
}
AutofillWalletSyncBridge::AutofillWalletSyncBridge(
const base::RepeatingCallback<void(bool)>& active_callback,
std::unique_ptr<syncer::ModelTypeChangeProcessor> change_processor,
bool has_persistent_storage,
AutofillWebDataBackend* web_data_backend)
: ModelTypeSyncBridge(std::move(change_processor)),
has_persistent_storage_(has_persistent_storage),
active_callback_(active_callback),
initial_sync_done_(false),
web_data_backend_(web_data_backend) {
DCHECK(web_data_backend_);
......@@ -113,7 +117,10 @@ base::Optional<syncer::ModelError> AutofillWalletSyncBridge::MergeSyncData(
SetSyncData(entity_data);
// After the first sync, we are sure that initial sync is done.
initial_sync_done_ = true;
if (!initial_sync_done_) {
initial_sync_done_ = true;
active_callback_.Run(true);
}
return base::nullopt;
}
......@@ -213,7 +220,12 @@ AutofillWalletSyncBridge::ApplyStopSyncChanges(
// If a metadata change list gets passed in, that means sync is actually
// disabled, so we want to delete the payments data.
if (delete_metadata_change_list) {
if (initial_sync_done_) {
active_callback_.Run(false);
}
SetSyncData(syncer::EntityChangeList());
initial_sync_done_ = false;
}
return StopSyncResponse::kModelStillReadyToSync;
}
......@@ -440,6 +452,8 @@ AutofillTable* AutofillWalletSyncBridge::GetAutofillTable() {
}
void AutofillWalletSyncBridge::LoadMetadata() {
DCHECK(!initial_sync_done_);
if (!web_data_backend_ || !web_data_backend_->GetDatabase() ||
!GetAutofillTable()) {
change_processor()->ReportError(
......@@ -454,9 +468,12 @@ void AutofillWalletSyncBridge::LoadMetadata() {
{FROM_HERE, "Failed reading autofill metadata from WebDatabase."});
return;
}
initial_sync_done_ = batch->GetModelTypeState().initial_sync_done();
change_processor()->ModelReadyToSync(std::move(batch));
if (change_processor()->IsTrackingMetadata()) {
initial_sync_done_ = true;
active_callback_.Run(true);
}
}
} // namespace autofill
......@@ -35,8 +35,11 @@ class AutofillWalletSyncBridge : public base::SupportsUserData::Data,
// Factory method that hides dealing with change_processor and also stores the
// created bridge within |web_data_service|. This method should only be
// called on |web_data_service|'s DB thread.
// |active_callback| will be called with a boolean describing whether Wallet
// data is actively sync whenever the state changes.
static void CreateForWebDataServiceAndBackend(
const std::string& app_locale,
const base::RepeatingCallback<void(bool)>& active_callback,
bool has_persistent_storage_,
AutofillWebDataBackend* webdata_backend,
AutofillWebDataService* web_data_service);
......@@ -45,6 +48,7 @@ class AutofillWalletSyncBridge : public base::SupportsUserData::Data,
AutofillWebDataService* web_data_service);
explicit AutofillWalletSyncBridge(
const base::RepeatingCallback<void(bool)>& active_callback,
std::unique_ptr<syncer::ModelTypeChangeProcessor> change_processor,
bool has_persistent_storage_,
AutofillWebDataBackend* web_data_backend);
......@@ -124,6 +128,10 @@ class AutofillWalletSyncBridge : public base::SupportsUserData::Data,
// content-area-account-based lightweight sync).
const bool has_persistent_storage_;
// Callback to let the metadata bridge know that whether the card data
// is actively syncing.
const base::RepeatingCallback<void(bool)> active_callback_;
// Stores whether initial sync has been done.
bool initial_sync_done_;
......
......@@ -15,6 +15,7 @@
#include "base/message_loop/message_loop.h"
#include "base/test/bind_test_util.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/mock_callback.h"
#include "base/test/scoped_task_environment.h"
#include "base/time/time.h"
#include "components/autofill/core/browser/autofill_profile.h"
......@@ -220,9 +221,9 @@ class AutofillWalletSyncBridgeTest : public testing::Test {
model_type_state.set_initial_sync_done(initial_sync_done);
EXPECT_TRUE(table()->UpdateModelTypeState(syncer::AUTOFILL_WALLET_DATA,
model_type_state));
bridge_.reset(new AutofillWalletSyncBridge(
mock_processor_.CreateForwardingProcessor(), UseFullSync(), &backend_));
active_callback_.Get(), mock_processor_.CreateForwardingProcessor(),
UseFullSync(), &backend_));
}
void StartSyncing(
......@@ -327,6 +328,10 @@ class AutofillWalletSyncBridgeTest : public testing::Test {
MockAutofillWebDataBackend* backend() { return &backend_; }
base::MockCallback<base::RepeatingCallback<void(bool)>>* active_callback() {
return &active_callback_;
};
virtual bool UseFullSync() { return true; }
private:
......@@ -340,6 +345,8 @@ class AutofillWalletSyncBridgeTest : public testing::Test {
std::unique_ptr<syncer::ClientTagBasedModelTypeProcessor> real_processor_;
std::unique_ptr<AutofillWalletSyncBridge> bridge_;
base::HistogramTester histogram_tester_;
NiceMock<base::MockCallback<base::RepeatingCallback<void(bool)>>>
active_callback_;
DISALLOW_COPY_AND_ASSIGN(AutofillWalletSyncBridgeTest);
};
......@@ -763,6 +770,31 @@ TEST_F(AutofillWalletSyncBridgeTest, ApplyStopSyncChanges_KeepData) {
ExpectNoHistogramsForCardsDiff();
}
TEST_F(AutofillWalletSyncBridgeTest, NotifiesWhenActivelySyncing) {
testing::InSequence seq;
ResetProcessor();
EXPECT_CALL(*active_callback(), Run(true));
ResetBridge(/*initial_sync_done=*/true);
// Start and stop sync to check that we notify the callback.
StartSyncing({});
EXPECT_CALL(*active_callback(), Run(false));
// Stopping sync with change list to indicate that the type is disabled.
bridge()->ApplyStopSyncChanges(
std::make_unique<syncer::InMemoryMetadataChangeList>());
EXPECT_CALL(*active_callback(), Run(true));
// Start and stop sync again to make sure we notify the callback again.
StartSyncing({});
// Passing in a non-null metadata change list indicates to the bridge that
// sync is stopping but the data type is not disabled, so we should not get
// a callback.
bridge()->ApplyStopSyncChanges(/*delete_metadata_change_list=*/nullptr);
}
class AutofillWalletEphemeralSyncBridgeTest
: public AutofillWalletSyncBridgeTest {
public:
......
......@@ -81,25 +81,42 @@ void InitSyncableAccountServicesOnDBSequence(
autofill::AutofillWebDataBackend* autofill_backend) {
DCHECK(db_task_runner->RunsTasksInCurrentSequence());
if (base::FeatureList::IsEnabled(switches::kSyncUSSAutofillWalletData)) {
autofill::AutofillWalletSyncBridge::CreateForWebDataServiceAndBackend(
app_locale, is_full_sync, autofill_backend, autofill_web_data.get());
} else {
autofill::AutofillWalletSyncableService::CreateForWebDataServiceAndBackend(
autofill_web_data.get(), autofill_backend, app_locale);
autofill::AutofillWalletSyncableService::FromWebDataService(
autofill_web_data.get())
->InjectStartSyncFlare(sync_flare);
}
base::RepeatingCallback<void(bool)> wallet_active_callback;
if (base::FeatureList::IsEnabled(switches::kSyncUSSAutofillWalletMetadata)) {
autofill::AutofillWalletMetadataSyncBridge::
CreateForWebDataServiceAndBackend(app_locale, autofill_backend,
autofill_web_data.get());
wallet_active_callback = base::BindRepeating(
&autofill::AutofillWalletMetadataSyncBridge::
OnWalletDataTrackingStateChanged,
autofill::AutofillWalletMetadataSyncBridge::FromWebDataService(
autofill_web_data.get())
->GetWeakPtr());
} else {
autofill::AutofillWalletMetadataSyncableService::
CreateForWebDataServiceAndBackend(autofill_web_data.get(),
autofill_backend, app_locale);
wallet_active_callback = base::BindRepeating(
&autofill::AutofillWalletMetadataSyncableService::
OnWalletDataTrackingStateChanged,
autofill::AutofillWalletMetadataSyncableService::FromWebDataService(
autofill_web_data.get())
->GetWeakPtr());
}
if (base::FeatureList::IsEnabled(switches::kSyncUSSAutofillWalletData)) {
autofill::AutofillWalletSyncBridge::CreateForWebDataServiceAndBackend(
app_locale, wallet_active_callback, is_full_sync, autofill_backend,
autofill_web_data.get());
} else {
autofill::AutofillWalletSyncableService::CreateForWebDataServiceAndBackend(
autofill_web_data.get(), autofill_backend, app_locale);
autofill::AutofillWalletSyncableService::FromWebDataService(
autofill_web_data.get())
->InjectStartSyncFlare(sync_flare);
// For non-USS wallet, the metadata is always checking the existence of
// wallet data to add/remove metadata entries.
wallet_active_callback.Run(true);
}
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment