0

[Bookmark] Clear account BookmarkModel on sign-out

BookmarkModelTypeProcessor::OnSyncStopping is called with
CLEAR_METADATA upon sign-out. After this CL,
BookmarkModelTypeProcessor can be configured to call
RemoveAllUserBookmarks at that moment.

Bug: 1444662
Change-Id: Ida7bba5e80940bd9ad81e80dc1062f283b8fab12
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4527935
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Boris Sazonov <bsazonov@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1143978}
This commit is contained in:
Boris Sazonov
2023-05-15 10:21:10 +00:00
committed by Chromium LUCI CQ
parent 69614b7e37
commit e33f23f1de
8 changed files with 116 additions and 43 deletions

@ -38,5 +38,6 @@ KeyedService* BookmarkSyncServiceFactory::BuildServiceInstanceFor(
content::BrowserContext* context) const {
Profile* profile = Profile::FromBrowserContext(context);
return new sync_bookmarks::BookmarkSyncService(
BookmarkUndoServiceFactory::GetForProfileIfExists(profile));
BookmarkUndoServiceFactory::GetForProfileIfExists(profile),
/*wipe_model_on_stopping_sync_with_clear_data=*/false);
}

@ -51,7 +51,7 @@ constexpr size_t kDefaultMaxBookmarksTillSyncEnabled = 100000;
class ScopedRemoteUpdateBookmarks {
public:
// |bookmark_model|, |bookmark_undo_service| and |observer| must not be null
// `bookmark_model`, `bookmark_undo_service` and `observer` must not be null
// and must outlive this object.
ScopedRemoteUpdateBookmarks(bookmarks::BookmarkModel* bookmark_model,
BookmarkUndoService* bookmark_undo_service,
@ -123,8 +123,11 @@ size_t CountSyncableBookmarksFromModel(bookmarks::BookmarkModel* model) {
} // namespace
BookmarkModelTypeProcessor::BookmarkModelTypeProcessor(
BookmarkUndoService* bookmark_undo_service)
BookmarkUndoService* bookmark_undo_service,
bool wipe_model_on_stopping_sync_with_clear_data)
: bookmark_undo_service_(bookmark_undo_service),
wipe_model_on_stopping_sync_with_clear_data_(
wipe_model_on_stopping_sync_with_clear_data),
max_bookmarks_till_sync_enabled_(kDefaultMaxBookmarksTillSyncEnabled) {}
BookmarkModelTypeProcessor::~BookmarkModelTypeProcessor() {
@ -141,7 +144,7 @@ void BookmarkModelTypeProcessor::ConnectSync(
worker_ = std::move(worker);
// |bookmark_tracker_| is instantiated only after initial sync is done.
// `bookmark_tracker_` is instantiated only after initial sync is done.
if (bookmark_tracker_) {
NudgeForCommitIfNeeded();
}
@ -164,7 +167,7 @@ void BookmarkModelTypeProcessor::GetLocalChanges(
GetLocalChangesCallback callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// Processor should never connect if
// |last_initial_merge_remote_updates_exceeded_limit_| is set.
// `last_initial_merge_remote_updates_exceeded_limit_` is set.
DCHECK(!last_initial_merge_remote_updates_exceeded_limit_);
BookmarkLocalChangesBuilder builder(bookmark_tracker_.get(), bookmark_model_);
std::move(callback).Run(builder.BuildCommitRequests(max_entries));
@ -176,7 +179,7 @@ void BookmarkModelTypeProcessor::OnCommitCompleted(
const syncer::FailedCommitResponseDataList& error_response_list) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// |error_response_list| is ignored, because all errors are treated as
// `error_response_list` is ignored, because all errors are treated as
// transient and the processor with eventually retry.
for (const syncer::CommitResponseData& response : committed_response_list) {
const SyncedBookmarkTrackerEntity* entity =
@ -205,10 +208,10 @@ void BookmarkModelTypeProcessor::OnUpdateReceived(
DCHECK(syncer::IsInitialSyncDone(model_type_state.initial_sync_state()));
DCHECK(start_callback_.is_null());
// Processor should never connect if
// |last_initial_merge_remote_updates_exceeded_limit_| is set.
// `last_initial_merge_remote_updates_exceeded_limit_` is set.
DCHECK(!last_initial_merge_remote_updates_exceeded_limit_);
// TODO(crbug.com/1356900): validate incoming updates, e.g. |gc_directive|
// TODO(crbug.com/1356900): validate incoming updates, e.g. `gc_directive`
// must be empty for Bookmarks.
syncer::LogUpdatesReceivedByProcessorHistogram(
@ -285,7 +288,7 @@ bool BookmarkModelTypeProcessor::IsConnectedForTest() const {
std::string BookmarkModelTypeProcessor::EncodeSyncMetadata() const {
std::string metadata_str;
if (bookmark_tracker_) {
// |last_initial_merge_remote_updates_exceeded_limit_| is only set in error
// `last_initial_merge_remote_updates_exceeded_limit_` is only set in error
// cases where the tracker would not be initialized.
DCHECK(!last_initial_merge_remote_updates_exceeded_limit_);
@ -343,7 +346,7 @@ void BookmarkModelTypeProcessor::ModelReadyToSync(
syncer::kSyncEnforceBookmarksCountLimit)) {
// Report error if remote updates fetched last time during initial merge
// exceeded limit. Note that here we are only setting
// |last_initial_merge_remote_updates_exceeded_limit_|, the actual error
// `last_initial_merge_remote_updates_exceeded_limit_`, the actual error
// would be reported in ConnectIfReady().
last_initial_merge_remote_updates_exceeded_limit_ = true;
} else {
@ -354,8 +357,8 @@ void BookmarkModelTypeProcessor::ModelReadyToSync(
if (bookmark_tracker_) {
StartTrackingMetadata();
} else if (!metadata_str.empty()) {
// Even if the field |last_initial_merge_remote_updates_exceeded_limit| is
// set and the feature toggle |kSyncEnforceBookmarksCountLimit| not
// Even if the field `last_initial_merge_remote_updates_exceeded_limit` is
// set and the feature toggle `kSyncEnforceBookmarksCountLimit` not
// enabled, making the metadata_str non-empty, scheduling a save shouldn't
// cause any problem.
DLOG(WARNING)
@ -398,7 +401,7 @@ void BookmarkModelTypeProcessor::OnSyncStarting(
StartCallback start_callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(start_callback);
// |favicon_service_| should have been set by now.
// `favicon_service_` should have been set by now.
DCHECK(favicon_service_);
DVLOG(1) << "Sync is starting for Bookmarks";
@ -428,7 +431,7 @@ void BookmarkModelTypeProcessor::ConnectIfReady() {
// Report error if remote updates fetched last time during initial merge
// exceeded limit.
if (last_initial_merge_remote_updates_exceeded_limit_) {
// |last_initial_merge_remote_updates_exceeded_limit_| is only set in error
// `last_initial_merge_remote_updates_exceeded_limit_` is only set in error
// case and thus tracker should be empty.
DCHECK(!bookmark_tracker_);
start_callback_.Reset();
@ -510,6 +513,14 @@ void BookmarkModelTypeProcessor::OnSyncStopping(
StopTrackingMetadataAndResetTracker();
}
last_initial_merge_remote_updates_exceeded_limit_ = false;
if (wipe_model_on_stopping_sync_with_clear_data_) {
// `CLEAR_METADATA` indicates sync is permanently disabled. Since
// `wipe_model_on_stopping_sync_with_clear_data_` is `true`, the
// lifetime of local data (bookmarks) is coupled with sync metadata's,
// which means disabling sync requires that bookmarks in local storage
// are deleted.
bookmark_model_->RemoveAllUserBookmarks();
}
schedule_save_closure_.Run();
break;
}
@ -524,7 +535,7 @@ void BookmarkModelTypeProcessor::NudgeForCommitIfNeeded() {
DCHECK(bookmark_tracker_);
// Issue error and stop sync if the number of local bookmarks exceed limit.
// If |error_handler_| is not set, the check is ignored because this gets
// If `error_handler_` is not set, the check is ignored because this gets
// re-evaluated in ConnectIfReady().
if (error_handler_ &&
bookmark_tracker_->TrackedBookmarksCount() >
@ -564,7 +575,7 @@ void BookmarkModelTypeProcessor::OnInitialUpdateReceived(
TRACE_EVENT0("sync", "BookmarkModelTypeProcessor::OnInitialUpdateReceived");
// |updates| can contain an additional root folder. The server may or may not
// `updates` can contain an additional root folder. The server may or may not
// deliver a root node - it is not guaranteed, but this works as an
// approximated safeguard.
const size_t max_initial_updates_count = max_bookmarks_till_sync_enabled_ + 1;

@ -35,9 +35,12 @@ class BookmarkModelObserverImpl;
class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
public syncer::ModelTypeControllerDelegate {
public:
// |bookmark_undo_service| must not be nullptr and must outlive this object.
explicit BookmarkModelTypeProcessor(
BookmarkUndoService* bookmark_undo_service);
// `bookmark_undo_service` must not be nullptr and must outlive this object.
// If `wipe_model_on_stopping_sync_with_clear_data` is `true`, then calling
// `OnSyncStopping` with `CLEAR_METADATA` will trigger the removal of all user
// bookmarks from the corresponding `BookmarkModel`.
BookmarkModelTypeProcessor(BookmarkUndoService* bookmark_undo_service,
bool wipe_model_on_stopping_sync_with_clear_data);
BookmarkModelTypeProcessor(const BookmarkModelTypeProcessor&) = delete;
BookmarkModelTypeProcessor& operator=(const BookmarkModelTypeProcessor&) =
@ -78,10 +81,10 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
std::string EncodeSyncMetadata() const;
// It mainly decodes a BookmarkModelMetadata proto serialized in
// |metadata_str|, and uses it to fill in the tracker and the model type state
// objects. |model| must not be null and must outlive this object. It is used
// `metadata_str`, and uses it to fill in the tracker and the model type state
// objects. `model` must not be null and must outlive this object. It is used
// to the retrieve the local node ids, and is stored in the processor to be
// used for further model operations. |schedule_save_closure| is a repeating
// used for further model operations. `schedule_save_closure` is a repeating
// closure used to schedule a save of the bookmark model together with the
// metadata.
void ModelReadyToSync(const std::string& metadata_str,
@ -90,7 +93,7 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
// Sets the favicon service used when processing remote updates. It must be
// called before the processor is ready to receive remote updates, and hence
// before OnSyncStarting() is called. |favicon_service| must not be null.
// before OnSyncStarting() is called. `favicon_service` must not be null.
void SetFaviconService(favicon::FaviconService* favicon_service);
// Returns the estimate of dynamically allocated memory in bytes.
@ -136,9 +139,9 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
void StopTrackingMetadataAndResetTracker();
// Creates a DictionaryValue for local and remote debugging information about
// |node| and appends it to |all_nodes|. It does the same for child nodes
// recursively. |index| is the index of |node| within its parent. |index|
// could computed from |node|, however it's much cheaper to pass from outside
// `node` and appends it to `all_nodes`. It does the same for child nodes
// recursively. `index` is the index of `node` within its parent. `index`
// could computed from `node`, however it's much cheaper to pass from outside
// since we iterate over child nodes already in the calling sites.
void AppendNodeAndChildrenForDebugging(const bookmarks::BookmarkNode* node,
int index,
@ -163,6 +166,11 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor,
// Used to suspend bookmark undo when processing remote changes.
const raw_ptr<BookmarkUndoService, DanglingUntriaged> bookmark_undo_service_;
// Controls whether bookmarks should be wiped when sync is stopped. Contains
// `true` for Account `BookmarkModel` and `false` for LocalOrSyncable
// `BookmarkModel`.
const bool wipe_model_on_stopping_sync_with_clear_data_;
// The callback used to schedule the persistence of bookmark model as well as
// the metadata to a file during which latest metadata should also be pulled
// via EncodeSyncMetadata. Processor should invoke it upon changes in the

@ -50,6 +50,7 @@ using testing::IsEmpty;
using testing::IsNull;
using testing::NiceMock;
using testing::NotNull;
using testing::Pointer;
using testing::UnorderedElementsAre;
const char kBookmarkBarTag[] = "bookmark_bar";
@ -244,7 +245,8 @@ class BookmarkModelTypeProcessorTest : public testing::Test {
public:
BookmarkModelTypeProcessorTest()
: processor_(std::make_unique<BookmarkModelTypeProcessor>(
&bookmark_undo_service_)),
&bookmark_undo_service_,
/*wipe_model_on_stopping_sync_with_clear_data=*/false)),
bookmark_model_(bookmarks::TestBookmarkClient::CreateModel()) {
processor_->SetFaviconService(&favicon_service_);
}
@ -306,9 +308,10 @@ class BookmarkModelTypeProcessorTest : public testing::Test {
}
// Simulate browser restart.
void ResetModelTypeProcessor() {
processor_ =
std::make_unique<BookmarkModelTypeProcessor>(&bookmark_undo_service_);
void ResetModelTypeProcessor(
bool wipe_model_on_stopping_sync_with_clear_data = false) {
processor_ = std::make_unique<BookmarkModelTypeProcessor>(
&bookmark_undo_service_, wipe_model_on_stopping_sync_with_clear_data);
processor_->SetFaviconService(&favicon_service_);
}
@ -517,7 +520,9 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldDecodeSyncMetadata) {
CreateNodeMetadata(bookmarknode, kNodeId);
// Create a new processor and init it with the metadata str.
BookmarkModelTypeProcessor new_processor(bookmark_undo_service());
BookmarkModelTypeProcessor new_processor(
bookmark_undo_service(),
/*wipe_model_on_stopping_sync_with_clear_data=*/false);
std::string metadata_str;
model_metadata.SerializeToString(&metadata_str);
@ -546,7 +551,9 @@ TEST_F(BookmarkModelTypeProcessorTest, ShouldDecodeEncodedSyncMetadata) {
SimulateModelReadyToSyncWithInitialSyncDone();
// Create a new processor and init it with the same metadata str.
BookmarkModelTypeProcessor new_processor(bookmark_undo_service());
BookmarkModelTypeProcessor new_processor(
bookmark_undo_service(),
/*wipe_model_on_stopping_sync_with_clear_data=*/false);
new_processor.ModelReadyToSync(processor()->EncodeSyncMetadata(),
base::DoNothing(), bookmark_model());
@ -579,7 +586,9 @@ TEST_F(BookmarkModelTypeProcessorTest,
bookmark_metadata->mutable_metadata()->set_server_id(kBookmarkBarId);
// Create a new processor and init it with the metadata str.
BookmarkModelTypeProcessor new_processor(bookmark_undo_service());
BookmarkModelTypeProcessor new_processor(
bookmark_undo_service(),
/*wipe_model_on_stopping_sync_with_clear_data=*/false);
// A save should be scheduled.
NiceMock<base::MockCallback<base::RepeatingClosure>>
@ -606,7 +615,9 @@ TEST_F(BookmarkModelTypeProcessorTest,
/*server_id=*/kBookmarkBarId);
// Create a new processor and init it with the metadata str.
BookmarkModelTypeProcessor new_processor(bookmark_undo_service());
BookmarkModelTypeProcessor new_processor(
bookmark_undo_service(),
/*wipe_model_on_stopping_sync_with_clear_data=*/false);
// A save should be scheduled.
NiceMock<base::MockCallback<base::RepeatingClosure>>
@ -1573,6 +1584,41 @@ TEST_F(BookmarkModelTypeProcessorTest,
"Sync.ClearMetadataWhileStopped.ImmediateClear", 0);
}
TEST_F(BookmarkModelTypeProcessorTest,
ShouldWipeBookmarksIfStoppedWithClearMetadata) {
ResetModelTypeProcessor(/*wipe_model_on_stopping_sync_with_clear_data=*/true);
const GURL kUrl("http://www.example.com");
bookmark_model()->AddURL(bookmark_model()->bookmark_bar_node(), /*index=*/0,
u"foo", kUrl);
const bookmarks::BookmarkNode* folder = bookmark_model()->AddFolder(
bookmark_model()->mobile_node(), /*index=*/0, u"folder");
bookmark_model()->AddURL(folder, /*index=*/0, u"bar", kUrl);
SimulateModelReadyToSyncWithInitialSyncDone();
SimulateOnSyncStarting();
ASSERT_FALSE(bookmark_model()->HasNoUserCreatedBookmarksOrFolders());
processor()->OnSyncStopping(syncer::CLEAR_METADATA);
EXPECT_TRUE(bookmark_model()->HasNoUserCreatedBookmarksOrFolders());
}
TEST_F(BookmarkModelTypeProcessorTest,
ShouldNotWipeBookmarksIfStoppedWithKeepMetadata) {
ResetModelTypeProcessor(/*wipe_model_on_stopping_sync_with_clear_data=*/true);
const GURL kUrl("http://www.example.com");
const bookmarks::BookmarkNode* node = bookmark_model()->AddURL(
bookmark_model()->mobile_node(), /*index=*/0, u"foo", kUrl);
SimulateModelReadyToSyncWithInitialSyncDone();
SimulateOnSyncStarting();
processor()->OnSyncStopping(syncer::KEEP_METADATA);
EXPECT_THAT(bookmark_model()->mobile_node()->children(),
ElementsAre(Pointer(Eq(node))));
}
TEST_F(BookmarkModelTypeProcessorTest,
ShouldNotClearMetadataWhileStoppedWithoutMetadataInitially) {
SimulateModelReadyToSyncWithoutLocalMetadata();

@ -10,8 +10,11 @@
namespace sync_bookmarks {
BookmarkSyncService::BookmarkSyncService(
BookmarkUndoService* bookmark_undo_service)
: bookmark_model_type_processor_(bookmark_undo_service) {}
BookmarkUndoService* bookmark_undo_service,
bool wipe_model_on_stopping_sync_with_clear_data)
: bookmark_model_type_processor_(
bookmark_undo_service,
wipe_model_on_stopping_sync_with_clear_data) {}
BookmarkSyncService::~BookmarkSyncService() = default;

@ -34,8 +34,12 @@ class BookmarkModelTypeProcessor;
// This service owns the BookmarkModelTypeProcessor.
class BookmarkSyncService : public KeyedService {
public:
// |bookmark_undo_service| must not be null and must outlive this object.
explicit BookmarkSyncService(BookmarkUndoService* bookmark_undo_service);
// If `wipe_model_on_stopping_sync_with_clear_data` is `true`, then the
// `bookmark_undo_service` must not be null and must outlive this object.
// lifetime of bookmarks in the associated storage is coupled with sync
// metadata's, so disabling sync will delete bookmarks in the storage.
BookmarkSyncService(BookmarkUndoService* bookmark_undo_service,
bool wipe_model_on_stopping_sync_with_clear_data);
BookmarkSyncService(const BookmarkSyncService&) = delete;
BookmarkSyncService& operator=(const BookmarkSyncService&) = delete;
@ -51,7 +55,7 @@ class BookmarkSyncService : public KeyedService {
bookmarks::BookmarkModel* model);
// Returns the ModelTypeControllerDelegate for syncer::BOOKMARKS.
// |favicon_service| is the favicon service used when processing updates in
// `favicon_service` is the favicon service used when processing updates in
// the underlying processor. It could have been a separate a setter in
// BookmarkSyncService instead of passing it as a parameter to
// GetBookmarkSyncControllerDelegate(). However, this would incur the risk of

@ -51,8 +51,8 @@ AccountBookmarkSyncServiceFactory::BuildServiceInstanceFor(
ChromeBrowserState::FromBrowserState(context);
std::unique_ptr<sync_bookmarks::BookmarkSyncService> bookmark_sync_service(
new sync_bookmarks::BookmarkSyncService(
BookmarkUndoServiceFactory::GetForBrowserStateIfExists(
browser_state)));
BookmarkUndoServiceFactory::GetForBrowserStateIfExists(browser_state),
/*wipe_model_on_stopping_sync_with_clear_data=*/true));
return bookmark_sync_service;
}

@ -46,8 +46,8 @@ LocalOrSyncableBookmarkSyncServiceFactory::BuildServiceInstanceFor(
ChromeBrowserState::FromBrowserState(context);
std::unique_ptr<sync_bookmarks::BookmarkSyncService> bookmark_sync_service(
new sync_bookmarks::BookmarkSyncService(
BookmarkUndoServiceFactory::GetForBrowserStateIfExists(
browser_state)));
BookmarkUndoServiceFactory::GetForBrowserStateIfExists(browser_state),
/*wipe_model_on_stopping_sync_with_clear_data=*/false));
return bookmark_sync_service;
}