Skip to content

Commit

Permalink
Sync: Small refactoring of Directory::CheckTreeInvariants.
Browse files Browse the repository at this point in the history
The purpose of this change is to minimize chances of hitting
crbug/456029.

1) Call ModelTypeToRootTag only when the root folder could
potentially be created on the client i.e. model type is
defined in client EntitySpecifics and it is one of supported
model types.
2) Rename is_type_root_folder to is_client_creatable_type_root_folder
to make it clear that the checks are specific to client
created type root folders.
3) Narrow the scope of where is_client_creatable_type_root_folder
has to be calculated.

BUG=456029

Review URL: https://codereview.chromium.org/948113005

Cr-Commit-Position: refs/heads/master@{#317742}
  • Loading branch information
stanisc authored and Commit bot committed Feb 24, 2015
1 parent b79e365 commit 3b78f0e
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 9 deletions.
2 changes: 1 addition & 1 deletion sync/engine/directory_update_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ SyncerError DirectoryUpdateHandler::ProcessGetUpdatesResponse(

// Auto-create permanent folder for the type if the progress marker
// changes from empty to non-empty.
if (!IsTypeWithServerGeneratedRoot(type_) &&
if (IsTypeWithClientGeneratedRoot(type_) &&
dir_->HasEmptyDownloadProgress(type_) &&
IsValidProgressMarker(progress_marker)) {
CreateTypeRoot(&trans);
Expand Down
2 changes: 1 addition & 1 deletion sync/engine/syncer_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ UpdateAttemptResponse AttemptToUpdateEntry(
}
} else {
// new_parent is unset.
DCHECK(!IsTypeWithServerGeneratedRoot(type));
DCHECK(IsTypeWithClientGeneratedRoot(type));
}
} else if (entry->GetIsDir()) {
Directory::Metahandles handles;
Expand Down
4 changes: 4 additions & 0 deletions sync/internal_api/public/base/model_type.h
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,10 @@ SYNC_EXPORT bool IsActOnceDataType(ModelType model_type);
// created on the server during initial sync.
SYNC_EXPORT bool IsTypeWithServerGeneratedRoot(ModelType model_type);

// Returns true if root folder for |model_type| is created on the client when
// that type is initially synced.
SYNC_EXPORT bool IsTypeWithClientGeneratedRoot(ModelType model_type);

// Returns set of model types that should be backed up before first sync.
SYNC_EXPORT ModelTypeSet BackupTypes();

Expand Down
16 changes: 10 additions & 6 deletions sync/syncable/directory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1226,14 +1226,18 @@ bool Directory::CheckTreeInvariants(syncable::BaseTransaction* trans,
int64 base_version = e.GetBaseVersion();
int64 server_version = e.GetServerVersion();
bool using_unique_client_tag = !e.GetUniqueClientTag().empty();
bool is_type_root_folder =
parentid.IsRoot() &&
e.GetUniqueServerTag() == ModelTypeToRootTag(e.GetModelType());
if (CHANGES_VERSION == base_version || 0 == base_version) {
ModelType model_type = e.GetModelType();
bool is_client_creatable_type_root_folder =
parentid.IsRoot() &&
IsTypeWithClientGeneratedRoot(model_type) &&
e.GetUniqueServerTag() == ModelTypeToRootTag(model_type);
if (e.GetIsUnappliedUpdate()) {
// Must be a new item, or a de-duplicated unique client tag
// that was created both locally and remotely, or a type root folder
// that was created both locally and remotely.
if (!(using_unique_client_tag || is_type_root_folder)) {
if (!(using_unique_client_tag ||
is_client_creatable_type_root_folder)) {
if (!SyncAssert(e.GetIsDel(), FROM_HERE,
"The entry should have been deleted.", trans))
return false;
Expand All @@ -1252,8 +1256,8 @@ bool Directory::CheckTreeInvariants(syncable::BaseTransaction* trans,
trans))
return false;
}
if (is_type_root_folder) {
// This must be a locally created type root folder
if (is_client_creatable_type_root_folder) {
// This must be a locally created type root folder.
if (!SyncAssert(
!e.GetIsUnsynced(), FROM_HERE,
"Locally created type root folders should not be unsynced.",
Expand Down
2 changes: 1 addition & 1 deletion sync/syncable/model_neutral_mutable_entry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ ModelNeutralMutableEntry::ModelNeutralMutableEntry(BaseWriteTransaction* trans,
CreateNewTypeRoot,
ModelType type)
: Entry(trans), base_write_transaction_(trans) {
DCHECK(!IsTypeWithServerGeneratedRoot(type));
DCHECK(IsTypeWithClientGeneratedRoot(type));
Entry same_type_root(trans, GET_TYPE_ROOT, type);
kernel_ = NULL;
if (same_type_root.good()) {
Expand Down
5 changes: 5 additions & 0 deletions sync/syncable/model_type.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1187,4 +1187,9 @@ bool IsTypeWithServerGeneratedRoot(ModelType model_type) {
return model_type == BOOKMARKS || model_type == NIGORI;
}

bool IsTypeWithClientGeneratedRoot(ModelType model_type) {
return IsRealDataType(model_type) &&
!IsTypeWithServerGeneratedRoot(model_type);
}

} // namespace syncer

0 comments on commit 3b78f0e

Please sign in to comment.