diff options
author | zea <zea@chromium.org> | 2015-04-01 18:11:04 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-04-02 01:12:08 +0000 |
commit | 5a36760990918e47c63d9c19471aab229f566b3d (patch) | |
tree | 5dfe480798a4980edaae5dcd0c779bb7edcfbdc9 /sync/syncable | |
parent | 9faf912c9b5e7135630e2ccb7fbfe4e451d66a77 (diff) | |
download | chromium_src-5a36760990918e47c63d9c19471aab229f566b3d.zip chromium_src-5a36760990918e47c63d9c19471aab229f566b3d.tar.gz chromium_src-5a36760990918e47c63d9c19471aab229f566b3d.tar.bz2 |
[Sync] Fix undeletion during commit logic
We were relying on the SYNCING bit to track whether a commit has begun or not.
This will not work in all cases, as any modification actually unsets the
SYNCING bit, thereby making it unreliable as a source for whether a sync cycle
has started for an entity. To fix that, introduce a new DIRTY_SYNC bit that
is only modified when a entry becomes dirty during a sync cycle. SYNCING can
then better match it's name, and will stay true until the sync cycle ends.
BUG=466098
TEST=repeatedly bookmark a page then remove the bookmark
Review URL: https://codereview.chromium.org/1048933003
Cr-Commit-Position: refs/heads/master@{#323388}
Diffstat (limited to 'sync/syncable')
-rw-r--r-- | sync/syncable/entry.cc | 10 | ||||
-rw-r--r-- | sync/syncable/entry.h | 6 | ||||
-rw-r--r-- | sync/syncable/entry_kernel.h | 8 | ||||
-rw-r--r-- | sync/syncable/model_neutral_mutable_entry.cc | 5 | ||||
-rw-r--r-- | sync/syncable/model_neutral_mutable_entry.h | 1 | ||||
-rw-r--r-- | sync/syncable/mutable_entry.cc | 7 | ||||
-rw-r--r-- | sync/syncable/syncable_enum_conversions.cc | 3 |
7 files changed, 30 insertions, 10 deletions
diff --git a/sync/syncable/entry.cc b/sync/syncable/entry.cc index 5bee5ee..9c6f862 100644 --- a/sync/syncable/entry.cc +++ b/sync/syncable/entry.cc @@ -62,6 +62,16 @@ base::DictionaryValue* Entry::ToValue(Cryptographer* cryptographer) const { return entry_info; } +bool Entry::GetSyncing() const { + DCHECK(kernel_); + return kernel_->ref(SYNCING); +} + +bool Entry::GetDirtySync() const { + DCHECK(kernel_); + return kernel_->ref(DIRTY_SYNC); +} + ModelType Entry::GetServerModelType() const { ModelType specifics_type = kernel_->GetServerModelType(); if (specifics_type != UNSPECIFIED) diff --git a/sync/syncable/entry.h b/sync/syncable/entry.h index 184377a..0c30ccd 100644 --- a/sync/syncable/entry.h +++ b/sync/syncable/entry.h @@ -215,10 +215,8 @@ class SYNC_EXPORT Entry { return kernel_->ref(SERVER_ATTACHMENT_METADATA); } - bool GetSyncing() const { - DCHECK(kernel_); - return kernel_->ref(SYNCING); - } + bool GetSyncing() const; + bool GetDirtySync() const ; ModelType GetServerModelType() const; ModelType GetModelType() const; diff --git a/sync/syncable/entry_kernel.h b/sync/syncable/entry_kernel.h index 0c2f471..3e0d2c7 100644 --- a/sync/syncable/entry_kernel.h +++ b/sync/syncable/entry_kernel.h @@ -178,9 +178,13 @@ enum { }; enum BitTemp { - // Not to be confused with IS_UNSYNCED, this bit is used to detect local - // changes to items that happen during the server Commit operation. + // Whether a server commit operation was started and has not yet completed + // for this entity. SYNCING = BIT_TEMPS_BEGIN, + // Whether a local change was made to an entity that had SYNCING set to true, + // and was therefore in the middle of a commit operation. + // Note: must only be set if SYNCING is true. + DIRTY_SYNC, BIT_TEMPS_END, }; diff --git a/sync/syncable/model_neutral_mutable_entry.cc b/sync/syncable/model_neutral_mutable_entry.cc index 81c25c3..5064f4507 100644 --- a/sync/syncable/model_neutral_mutable_entry.cc +++ b/sync/syncable/model_neutral_mutable_entry.cc @@ -405,6 +405,11 @@ void ModelNeutralMutableEntry::PutSyncing(bool value) { kernel_->put(SYNCING, value); } +void ModelNeutralMutableEntry::PutDirtySync(bool value) { + DCHECK(!value || GetSyncing()); + kernel_->put(DIRTY_SYNC, value); +} + void ModelNeutralMutableEntry::PutParentIdPropertyOnly(const Id& parent_id) { base_write_transaction_->TrackChangesTo(kernel_); dir()->ReindexParentId(base_write_transaction(), kernel_, parent_id); diff --git a/sync/syncable/model_neutral_mutable_entry.h b/sync/syncable/model_neutral_mutable_entry.h index 2a9a698..5ff71ad 100644 --- a/sync/syncable/model_neutral_mutable_entry.h +++ b/sync/syncable/model_neutral_mutable_entry.h @@ -78,6 +78,7 @@ class SYNC_EXPORT_PRIVATE ModelNeutralMutableEntry : public Entry { void PutServerUniquePosition(const UniquePosition& value); void PutServerAttachmentMetadata(const sync_pb::AttachmentMetadata& value); void PutSyncing(bool value); + void PutDirtySync(bool value); // Do a simple property-only update of the PARENT_ID field. Use with caution. // diff --git a/sync/syncable/mutable_entry.cc b/sync/syncable/mutable_entry.cc index d627cf4..f6f3f16 100644 --- a/sync/syncable/mutable_entry.cc +++ b/sync/syncable/mutable_entry.cc @@ -187,8 +187,8 @@ void MutableEntry::PutIsDel(bool value) { // - Let us delete this entry permanently when we next restart sync - see // DirectoryBackingStore::SafeToPurgeOnLoading. // This will avoid crbug.com/125381. - // Note: do not unset IsUnsynced if the syncer is in the middle of - // attempting to commit this entity. + // Note: do not unset IsUnsynced if the syncer has started but not yet + // finished committing this entity. if (!GetId().ServerKnows() && !GetSyncing()) { PutIsUnsynced(false); } @@ -293,7 +293,8 @@ bool MarkForSyncing(MutableEntry* e) { DCHECK(!e->IsRoot()) << "We shouldn't mark a permanent object for syncing."; if (!(e->PutIsUnsynced(true))) return false; - e->PutSyncing(false); + if (e->GetSyncing()) + e->PutDirtySync(true); return true; } diff --git a/sync/syncable/syncable_enum_conversions.cc b/sync/syncable/syncable_enum_conversions.cc index b98500f..fe8cd5d4 100644 --- a/sync/syncable/syncable_enum_conversions.cc +++ b/sync/syncable/syncable_enum_conversions.cc @@ -176,10 +176,11 @@ const char* GetAttachmentMetadataFieldString( } const char* GetBitTempString(BitTemp bit_temp) { - ASSERT_ENUM_BOUNDS(SYNCING, SYNCING, + ASSERT_ENUM_BOUNDS(SYNCING, DIRTY_SYNC, BIT_TEMPS_BEGIN, BIT_TEMPS_END - 1); switch (bit_temp) { ENUM_CASE(SYNCING); + ENUM_CASE(DIRTY_SYNC); case BIT_TEMPS_END: break; } NOTREACHED(); |