[Sync] Update EntityData on all codepaths from model type sync bridge

[Sync] Update EntityData on all codepaths from model type sync bridge

When passing nEntityData to change processor, sync bridge usually only populates
few required fields: specifics and non_unique_name. It also provides storage key
and, indirectly, client tag. It is a responsibility of change processor to
populate remaining fields (id, creation time, modification time) before passing
EntityData to worker. values for these fields come from metadata.

There are a few scenarios how change processor receives EntityData to be
committed:
1. Local change: bridge creates new entry or updates existing one
2. Conflict resolution where bridge overrides resulting entity with locally
   generated specifics.
3. Committing pending changes after restart: change processor detects entities
   that need to be committed and requests EntityData from bridge.
4. Local reencryption
5. Reencryption after receiving remote update with outdated key.

The issue is that in a few scenarios above id is not copied from metadata to
EntityData. As a result when committing pending changes change processor sets
valid base_version in CommitRequestData but keeps id empty causing DCHECK in
ModelTypeWorker.

The solution is to treat metadata as the source of truth and to always populate
EntityData fields from metadata. Code path looks approximately like this:
- Update metadata if needed (cases 1 and 2). Implemented in
  ProcessorEntityTracker::MakeLocalChange)
- Copy fields from metadata to EntityData (cases 1-4). Implemented in
  ProcessorEntityTracker::SetCommitData

Case 5 is special. EntityData doesn't come from bridge, but instead from sync
server and therefore doesn't need to be adjusted.

This change is not final. There is one more case where metadata can be updated
after EntityData is wrapped into EntityDataPtr and made immutable:
- Local client creates new entity and enqueues it for commit. (entity is
  immutable)
- Update from server comes with entity with the same client tag hash and valid
  id.
- Conflict resolution logic kicks in and, if it is resolved with "Local wins"
  resolution, then original EntityData (with still empty id) should be
  committed.

I'll make a separate change for this scenario as it is more involved. The
chances of hitting it are much smaller compared to currently broken "committing
pending changes" scenario, therefore I think it is important to land this
partial fix and merge it to M57.

I've fixed how assigning id to new entity is handled in
MockModelTypeWorker::SuccessfulCommitResponse.

I've added checks that validate id/version combination into
ProcessorEntityTracker and MockModelTypeWorker. They verify correctness of this
change on scenarios from SharedModelTypeProcessorTest.

BUG=672100
R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2690913005
Cr-Commit-Position: refs/heads/master@{#450227}
Committed: https://chromium.googlesource.com/chromium/src/+/4d9357af0df028dab696fe7c4e7cce2815da01ef

[pavely, 2017-02-13 22:57:00]

The CQ bit was checked by pavely@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 22:57:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-02-14 00:33:41]

Amazing CL description Pavel. Feel free to ignore any/all changes, especially if
you plan to do them in the non-M57-merging change

lgtm

https://codereview.chromium.org/2690913005/diff/1/components/sync/model_impl/...
File components/sync/model_impl/processor_entity_tracker.cc (right):

https://codereview.chromium.org/2690913005/diff/1/components/sync/model_impl/...
components/sync/model_impl/processor_entity_tracker.cc:68: if
(data->creation_time.is_null())
What do you think of DCHECK we have both times and then set them blindly?

https://codereview.chromium.org/2690913005/diff/1/components/sync/model_impl/...
components/sync/model_impl/processor_entity_tracker.cc:74: commit_data_.reset();
why do you need this reset?

https://codereview.chromium.org/2690913005/diff/1/components/sync/model_impl/...
components/sync/model_impl/processor_entity_tracker.cc:166: if
(!data->creation_time.is_null())
The rest of your change code seems to be defensive/repairing, but not this one?

[commit-bot: I haz the power, 2017-02-14 00:43:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-14 00:43:38]

Dry run: This issue passed the CQ dry run.

[pavely, 2017-02-14 02:09:28]

Patchset #3 (id:40001) has been deleted

[pavely, 2017-02-14 02:09:34]

The CQ bit was checked by pavely@chromium.org

[pavely, 2017-02-14 02:09:34]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2690913005/#ps20001
(title: "Address comments")

[pavely, 2017-02-14 02:09:42]

https://codereview.chromium.org/2690913005/diff/1/components/sync/model_impl/...
File components/sync/model_impl/processor_entity_tracker.cc (right):

https://codereview.chromium.org/2690913005/diff/1/components/sync/model_impl/...
components/sync/model_impl/processor_entity_tracker.cc:68: if
(data->creation_time.is_null())
On 2017/02/14 00:33:40, skym wrote:
> What do you think of DCHECK we have both times and then set them blindly?

I thought that if data doesn't specify mtime then we should use the one from
metadata, but if data explicitly specifies mtime then we should commit with that
value. Now that I think about it, if bridge provided mtime for uncommitted
entity then mtime in EntityData would diverge from mtime in metadata.

I'm blindly reseting data's timestamps to metadata values.

https://codereview.chromium.org/2690913005/diff/1/components/sync/model_impl/...
components/sync/model_impl/processor_entity_tracker.cc:74: commit_data_.reset();
On 2017/02/14 00:33:40, skym wrote:
> why do you need this reset?

CacheCommitData has a DCHECK that ensures there were no previous cached value
before setting new one. This DCHECK is useful since CacheCommitData can be
called from other place.

https://codereview.chromium.org/2690913005/diff/1/components/sync/model_impl/...
components/sync/model_impl/processor_entity_tracker.cc:166: if
(!data->creation_time.is_null())
On 2017/02/14 00:33:40, skym wrote:
> The rest of your change code seems to be defensive/repairing, but not this
one?

This code says: If data's timestamps were explicitly set by bridge, there must
be a reason for it and we should copy them to metadata. Otherwise keep creation
time and set modification time to Now().

I wasn't sure what the right behavior should be when data timestamps are set, so
I coded the most sensible one I could think of.

[commit-bot: I haz the power, 2017-02-14 02:10:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-14 03:47:51]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487038174449980,
"parent_rev": "c15cf23b7d906052c0db5cb1e0a5513b35a636ba", "commit_rev":
"4d9357af0df028dab696fe7c4e7cce2815da01ef"}

[commit-bot: I haz the power, 2017-02-14 03:48:53]

Description was changed from

==========
[Sync] Update EntityData on all codepaths from model type sync bridge

When passing nEntityData to change processor, sync bridge usually only populates
few required fields: specifics and non_unique_name. It also provides storage key
and, indirectly, client tag. It is a responsibility of change processor to
populate remaining fields (id, creation time, modification time) before passing
EntityData to worker. values for these fields come from metadata.

There are a few scenarios how change processor receives EntityData to be
committed:
1. Local change: bridge creates new entry or updates existing one
2. Conflict resolution where bridge overrides resulting entity with locally
   generated specifics.
3. Committing pending changes after restart: change processor detects entities
   that need to be committed and requests EntityData from bridge.
4. Local reencryption
5. Reencryption after receiving remote update with outdated key.

The issue is that in a few scenarios above id is not copied from metadata to
EntityData. As a result when committing pending changes change processor sets
valid base_version in CommitRequestData but keeps id empty causing DCHECK in
ModelTypeWorker.

The solution is to treat metadata as the source of truth and to always populate
EntityData fields from metadata. Code path looks approximately like this:
- Update metadata if needed (cases 1 and 2). Implemented in
  ProcessorEntityTracker::MakeLocalChange)
- Copy fields from metadata to EntityData (cases 1-4). Implemented in
  ProcessorEntityTracker::SetCommitData

Case 5 is special. EntityData doesn't come from bridge, but instead from sync
server and therefore doesn't need to be adjusted.

This change is not final. There is one more case where metadata can be updated
after EntityData is wrapped into EntityDataPtr and made immutable:
- Local client creates new entity and enqueues it for commit. (entity is
  immutable)
- Update from server comes with entity with the same client tag hash and valid
  id.
- Conflict resolution logic kicks in and, if it is resolved with "Local wins"
  resolution, then original EntityData (with still empty id) should be
  committed.

I'll make a separate change for this scenario as it is more involved. The
chances of hitting it are much smaller compared to currently broken "committing
pending changes" scenario, therefore I think it is important to land this
partial fix and merge it to M57.

I've fixed how assigning id to new entity is handled in
MockModelTypeWorker::SuccessfulCommitResponse.

I've added checks that validate id/version combination into
ProcessorEntityTracker and MockModelTypeWorker. They verify correctness of this
change on scenarios from SharedModelTypeProcessorTest.

BUG=672100
R=skym@chromium.org
==========

to

==========
[Sync] Update EntityData on all codepaths from model type sync bridge

When passing nEntityData to change processor, sync bridge usually only populates
few required fields: specifics and non_unique_name. It also provides storage key
and, indirectly, client tag. It is a responsibility of change processor to
populate remaining fields (id, creation time, modification time) before passing
EntityData to worker. values for these fields come from metadata.

There are a few scenarios how change processor receives EntityData to be
committed:
1. Local change: bridge creates new entry or updates existing one
2. Conflict resolution where bridge overrides resulting entity with locally
   generated specifics.
3. Committing pending changes after restart: change processor detects entities
   that need to be committed and requests EntityData from bridge.
4. Local reencryption
5. Reencryption after receiving remote update with outdated key.

The issue is that in a few scenarios above id is not copied from metadata to
EntityData. As a result when committing pending changes change processor sets
valid base_version in CommitRequestData but keeps id empty causing DCHECK in
ModelTypeWorker.

The solution is to treat metadata as the source of truth and to always populate
EntityData fields from metadata. Code path looks approximately like this:
- Update metadata if needed (cases 1 and 2). Implemented in
  ProcessorEntityTracker::MakeLocalChange)
- Copy fields from metadata to EntityData (cases 1-4). Implemented in
  ProcessorEntityTracker::SetCommitData

Case 5 is special. EntityData doesn't come from bridge, but instead from sync
server and therefore doesn't need to be adjusted.

This change is not final. There is one more case where metadata can be updated
after EntityData is wrapped into EntityDataPtr and made immutable:
- Local client creates new entity and enqueues it for commit. (entity is
  immutable)
- Update from server comes with entity with the same client tag hash and valid
  id.
- Conflict resolution logic kicks in and, if it is resolved with "Local wins"
  resolution, then original EntityData (with still empty id) should be
  committed.

I'll make a separate change for this scenario as it is more involved. The
chances of hitting it are much smaller compared to currently broken "committing
pending changes" scenario, therefore I think it is important to land this
partial fix and merge it to M57.

I've fixed how assigning id to new entity is handled in
MockModelTypeWorker::SuccessfulCommitResponse.

I've added checks that validate id/version combination into
ProcessorEntityTracker and MockModelTypeWorker. They verify correctness of this
change on scenarios from SharedModelTypeProcessorTest.

BUG=672100
R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2690913005
Cr-Commit-Position: refs/heads/master@{#450227}
Committed:
https://chromium.googlesource.com/chromium/src/+/4d9357af0df028dab696fe7c4e7c...
==========

[commit-bot: I haz the power, 2017-02-14 03:48:54]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/4d9357af0df028dab696fe7c4e7c...