Not happening anymore today.
May be I was not rebase?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/46bb6daf3f67cc9f04ddc14b165cb3c40a6ed7b0

commit 46bb6daf3f67cc9f04ddc14b165cb3c40a6ed7b0
Author: pavely <pavely@chromium.org>
Date: Fri Dec 09 01:34:16 2016

[Sync] WorkerEntityTracker: Ensure that update and commit response have valid id.

This change enforces the check in ModelTypeWorker::AdjustCommitProto ensuring
that id is set to valid string.

R=maxbogue@chromium.org
BUG= 672100 

step

Review-Url: https://codereview.chromium.org/2562673003
Cr-Commit-Position: refs/heads/master@{#437411}

[modify] https://crrev.com/46bb6daf3f67cc9f04ddc14b165cb3c40a6ed7b0/components/sync/engine_impl/model_type_worker.cc
[modify] https://crrev.com/46bb6daf3f67cc9f04ddc14b165cb3c40a6ed7b0/components/sync/engine_impl/worker_entity_tracker.cc

Had it again today.

I hit DCHECK(!sync_entity->id_string().empty()); (line 369) with a device_info_.
Maybe this is more general than reading list.

I now regularly see this (can't run chrome on my device for more than a few minutes before dcheck/crash.)

So far the theory is that in ProcessorEntityTracker::InitializeCommitRequestData id comes from commit_data_ while base_version comes from metadata_. Id in metadata might not match it in commit_data_.

 Issue 690067  has been merged into this issue.

If the DCHECK catch some inconsistency, this must be fixed in M-57.

My previous theory was wrong. I was able to reproduce the issue with different sequence of steps:
- Setup sync, wait for local entry to get committed (metadata with server version and correct id gets written to the store)
- Switch to offline and make local modifications (metadata gets updated indicating commit is requested)
- Restart browser, switch to online (ProcessorEntityTracker knows entity needs to be committed, but doesn't have cached data in memory, it requests data from the bridge)
- Bridge reads data (id is empty in EntityData), passes it to ProcessorEntityTracker, which keeps id intact, but adds server version. When this entity reaches worker it has empty id and valid version, which triggers DCHECK.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d9357af0df028dab696fe7c4e7cce2815da01ef

commit 4d9357af0df028dab696fe7c4e7cce2815da01ef
Author: pavely <pavely@chromium.org>
Date: Tue Feb 14 03:47:46 2017

[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}

[modify] https://crrev.com/4d9357af0df028dab696fe7c4e7cce2815da01ef/components/sync/model_impl/processor_entity_tracker.cc
[modify] https://crrev.com/4d9357af0df028dab696fe7c4e7cce2815da01ef/components/sync/model_impl/processor_entity_tracker.h
[modify] https://crrev.com/4d9357af0df028dab696fe7c4e7cce2815da01ef/components/sync/model_impl/shared_model_type_processor.cc
[modify] https://crrev.com/4d9357af0df028dab696fe7c4e7cce2815da01ef/components/sync/model_impl/shared_model_type_processor_unittest.cc
[modify] https://crrev.com/4d9357af0df028dab696fe7c4e7cce2815da01ef/components/sync/test/engine/mock_model_type_worker.cc

I'm requesting a merge to M57.

The issue might cause duplication of ReadingList entries in certain scenarios. ReadingList is launching on iOS in M57.

This bug requires manual review: Less than 2 weeks to go before AppStore submit on M57
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Is there any steps to follow to verify this issue?

The issue was consistently reproducing for justincohen@. I've asked him yesterday and he said that after he updated to trunk he didn't hit this issue.

Here is one way to test this issue (it needs to be debug build of Chrome though):
- enable ReadingList
- sign in into chrome
- take device offline, make some modifications of ReadingList entries
- restart browser
Without my change DCHECK should get triggered within a minute.

I just did manual verification of this change. It works as expected. 

Here are the steps.
With debug build of chrome:
- Open page, add to reading list
- Turn off wifi, open page from reading list (this modifies reading list entry making it eligible for commit to server)
- Find entry in about:sync->Node browser. Confirm that sequence_number is greater than acked_sequence_number
- Kill chrome app, enable wifi, restart chrome
- Confirm that acked_sequence_number got updated to value of sequence number.

Last step confirms that everything worked. Without my change chrome would DCHECK there.

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dab3057f63560616c7be5d426d5b61b77a4a2880

commit dab3057f63560616c7be5d426d5b61b77a4a2880
Author: Pavel Yatsuk <pavely@chromium.org>
Date: Tue Feb 21 18:44:49 2017

[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}
(cherry picked from commit 4d9357af0df028dab696fe7c4e7cce2815da01ef)

Review-Url: https://codereview.chromium.org/2706283002 .
Cr-Commit-Position: refs/branch-heads/2987@{#613}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/dab3057f63560616c7be5d426d5b61b77a4a2880/components/sync/model_impl/processor_entity_tracker.cc
[modify] https://crrev.com/dab3057f63560616c7be5d426d5b61b77a4a2880/components/sync/model_impl/processor_entity_tracker.h
[modify] https://crrev.com/dab3057f63560616c7be5d426d5b61b77a4a2880/components/sync/model_impl/shared_model_type_processor.cc
[modify] https://crrev.com/dab3057f63560616c7be5d426d5b61b77a4a2880/components/sync/model_impl/shared_model_type_processor_unittest.cc
[modify] https://crrev.com/dab3057f63560616c7be5d426d5b61b77a4a2880/components/sync/test/engine/mock_model_type_worker.cc