[Sync] Fixing two bugs in the worker revealed by trying to add an encryption integration test.

[Sync] Fixing two bugs in the worker revealed by trying to add an encryption integration test.

The first bug was we were trying to start a nested cycle cycle, which is not allowed. The logic in the worker that tries to nudge in response to encryption changing is super old, and doesn't make any sense, so this was fixed by simply not nudging. If there needs to be a commit in response to encryption changing, the processor will drive this.

The second issue was that in certain cases the worker would try to commit something with an id but not a version. This isn't supposed to be able to happen. Turns out two quick commits could get us into this state because both would be created by the processor before getting valid version data. However, the WorkerEntityTracker can detect this is happening and has enough information to make things right.

BUG=647875
Committed: https://crrev.com/946cbb1044cabe81b6a6758f9140089e60fc1950
Cr-Commit-Position: refs/heads/master@{#420471}

[skym, 2016-09-20 23:25:03]

Description was changed from

==========
[Sync] Adding integration test for USS encryption, and fixing two bugs it
revealed.

BUG=647875
==========

to

==========
[Sync] Adding integration test for USS encryption, and fixing two bugs it
revealed.

BUG=647875
==========

[skym, 2016-09-20 23:25:03]

skym@chromium.org changed reviewers:
+ maxbogue@chromium.org

[skym, 2016-09-20 23:25:04]

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

[skym, 2016-09-20 23:25:11]

PTAL

[commit-bot: I haz the power, 2016-09-20 23:25:52]

Dry run: 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, 2016-09-21 00:31:15]

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

[commit-bot: I haz the power, 2016-09-21 00:31:15]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-09-21 00:57:14]

https://codereview.chromium.org/2350803005/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right):

https://codereview.chromium.org/2350803005/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:314:
model1->WriteItem(kKey1, kValue1);
What are each of these four items testing?

https://codereview.chromium.org/2350803005/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:316:
ASSERT_TRUE(EnableEncryption(0));
So this is using... the old version of encryption, without a passphrase?

https://codereview.chromium.org/2350803005/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:321:
ASSERT_TRUE(DataChecker(model1, kKey1, kValue1).Wait());
I think you mean kKey2 here.

https://codereview.chromium.org/2350803005/diff/20001/components/sync/core/pr...
File components/sync/core/processor_entity_tracker_unittest.cc (right):

https://codereview.chromium.org/2350803005/diff/20001/components/sync/core/pr...
components/sync/core/processor_entity_tracker_unittest.cc:472:
EXPECT_TRUE(entity->IsUnsynced());
Thanks!

https://codereview.chromium.org/2350803005/diff/20001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker_unittest.cc (right):

https://codereview.chromium.org/2350803005/diff/20001/components/sync/engine_...
components/sync/engine_impl/model_type_worker_unittest.cc:882:
GenerateTagHash(kTag1), GenerateSpecifics(kTag1, kValue2));
Use kHash1.

https://codereview.chromium.org/2350803005/diff/20001/components/sync/engine_...
components/sync/engine_impl/model_type_worker_unittest.cc:883: // Override the
version the processor sets.
Instead of this comment, I would add line break below line 886 and add a comment
above line 881 along the lines of "Create a custom commit request using the old
base_version."

https://codereview.chromium.org/2350803005/diff/20001/components/sync/engine_...
components/sync/engine_impl/model_type_worker_unittest.cc:889: const
google::protobuf::RepeatedPtrField<sync_pb::SyncEntity>& entries =
I almost want to say use const auto& here because this type is a mess. But, it
might not be super clear that the result would be a repeated field so feel free
to leave it.

https://codereview.chromium.org/2350803005/diff/20001/components/sync/engine_...
File components/sync/engine_impl/worker_entity_tracker.cc (right):

https://codereview.chromium.org/2350803005/diff/20001/components/sync/engine_...
components/sync/engine_impl/worker_entity_tracker.cc:99: // Now that we know
we're not in a conflict state, increase if there have been
"There's no conflict; increase base_version_ if there was a commit response the
processor didn't know about."

Technically "between the creation of this commit data and now" is wrong. The
worker might have known of the ack and it just hadn't hit the model thread yet.

[skym, 2016-09-22 18:57:47]

Description was changed from

==========
[Sync] Adding integration test for USS encryption, and fixing two bugs it
revealed.

BUG=647875
==========

to

==========
[Sync] Fixing two bugs in the worker revealed by trying to add an encryption
integration test.

The first bug was we were trying to start a nested cycle cycle, which is not
allowed. The logic in the worker that tries to nudge in response to encryption
changing is super old, and doesn't make any sense, so this was fixed by simply
not nudging. If there needs to be a commit in response to encryption changing,
the processor will drive this.

The second issue was that in certain cases the worker would try to commit
something with an id but not a version. This isn't supposed to be able to
happen. Turns out two quick commits could get us into this state because both
would be created by the processor before getting valid version data. However,
the WorkerEntityTracker can detect this is happening and has enough information
to make things right.

BUG=647875
==========

[skym, 2016-09-22 18:58:19]

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

[commit-bot: I haz the power, 2016-09-22 18:58:50]

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

[skym, 2016-09-22 18:58:52]

https://codereview.chromium.org/2350803005/diff/20001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right):

https://codereview.chromium.org/2350803005/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:314:
model1->WriteItem(kKey1, kValue1);
On 2016/09/21 00:57:14, maxbogue wrote:
> What are each of these four items testing?

Reverted file.

https://codereview.chromium.org/2350803005/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:316:
ASSERT_TRUE(EnableEncryption(0));
On 2016/09/21 00:57:14, maxbogue wrote:
> So this is using... the old version of encryption, without a passphrase?

Whoops, good catch. Breaking into multiple CLs.

https://codereview.chromium.org/2350803005/diff/20001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:321:
ASSERT_TRUE(DataChecker(model1, kKey1, kValue1).Wait());
On 2016/09/21 00:57:14, maxbogue wrote:
> I think you mean kKey2 here.

You're right! Fixed.

https://codereview.chromium.org/2350803005/diff/20001/components/sync/core/pr...
File components/sync/core/processor_entity_tracker_unittest.cc (right):

https://codereview.chromium.org/2350803005/diff/20001/components/sync/core/pr...
components/sync/core/processor_entity_tracker_unittest.cc:472:
EXPECT_TRUE(entity->IsUnsynced());
On 2016/09/21 00:57:14, maxbogue wrote:
> Thanks!

Acknowledged.

https://codereview.chromium.org/2350803005/diff/20001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker_unittest.cc (right):

https://codereview.chromium.org/2350803005/diff/20001/components/sync/engine_...
components/sync/engine_impl/model_type_worker_unittest.cc:882:
GenerateTagHash(kTag1), GenerateSpecifics(kTag1, kValue2));
On 2016/09/21 00:57:14, maxbogue wrote:
> Use kHash1.

Done.

https://codereview.chromium.org/2350803005/diff/20001/components/sync/engine_...
components/sync/engine_impl/model_type_worker_unittest.cc:883: // Override the
version the processor sets.
On 2016/09/21 00:57:14, maxbogue wrote:
> Instead of this comment, I would add line break below line 886 and add a
comment
> above line 881 along the lines of "Create a custom commit request using the
old
> base_version."

Done.

https://codereview.chromium.org/2350803005/diff/20001/components/sync/engine_...
components/sync/engine_impl/model_type_worker_unittest.cc:889: const
google::protobuf::RepeatedPtrField<sync_pb::SyncEntity>& entries =
On 2016/09/21 00:57:14, maxbogue wrote:
> I almost want to say use const auto& here because this type is a mess. But, it
> might not be super clear that the result would be a repeated field so feel
free
> to leave it.

Yeah, with auto the .Get(0) doesn't really make any sense. I think the
assumption with auto would be that it's just a generic iterator, which is not
how i'm using it.

https://codereview.chromium.org/2350803005/diff/20001/components/sync/engine_...
File components/sync/engine_impl/worker_entity_tracker.cc (right):

https://codereview.chromium.org/2350803005/diff/20001/components/sync/engine_...
components/sync/engine_impl/worker_entity_tracker.cc:99: // Now that we know
we're not in a conflict state, increase if there have been
On 2016/09/21 00:57:14, maxbogue wrote:
> "There's no conflict; increase base_version_ if there was a commit response
the
> processor didn't know about."
> 
> Technically "between the creation of this commit data and now" is wrong. The
> worker might have known of the ack and it just hadn't hit the model thread
yet.

Done.

[commit-bot: I haz the power, 2016-09-22 19:46:34]

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

[commit-bot: I haz the power, 2016-09-22 19:46:35]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-09-22 19:55:58]

lgtm

[skym, 2016-09-22 20:40:06]

The CQ bit was checked by skym@chromium.org

[commit-bot: I haz the power, 2016-09-22 20:40:52]

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, 2016-09-22 20:46:57]

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

[commit-bot: I haz the power, 2016-09-22 20:46:58]

Failed to apply patch for components/sync/engine_impl/model_type_worker.cc:
While running git apply --index -3 -p1;
  error: patch failed: components/sync/engine_impl/model_type_worker.cc:78
  Falling back to three-way merge...
  Applied patch to 'components/sync/engine_impl/model_type_worker.cc' with
conflicts.
  U components/sync/engine_impl/model_type_worker.cc

Patch:       components/sync/engine_impl/model_type_worker.cc
Index: components/sync/engine_impl/model_type_worker.cc
diff --git a/components/sync/engine_impl/model_type_worker.cc
b/components/sync/engine_impl/model_type_worker.cc
index
72160a5cd35e6f8fab546388702a1c1c1fdccbb9..abf6b3f4122ce5fe966602cf6dcf9183f41f2ae4
100644
--- a/components/sync/engine_impl/model_type_worker.cc
+++ b/components/sync/engine_impl/model_type_worker.cc
@@ -78,10 +78,6 @@ void ModelTypeWorker::UpdateCryptographer(
 
   // Update our state and that of the proxy.
   OnCryptographerUpdated();
-
-  // Nudge the scheduler if we're now allowed to commit.
-  if (CanCommitItems())
-    nudge_handler_->NudgeForCommit(type_);
 }
 
 // UpdateHandler implementation.
@@ -202,7 +198,6 @@ void ModelTypeWorker::ApplyPendingUpdates() {
 
 void ModelTypeWorker::EnqueueForCommit(const CommitRequestDataList& list) {
   DCHECK(CalledOnValidThread());
-
   DCHECK(IsTypeInitialized())
       << "Asked to commit items before type was initialized.  "
       << "ModelType is: " << ModelTypeToString(type_);
@@ -247,9 +242,8 @@ std::unique_ptr<CommitContribution>
ModelTypeWorker::GetContribution(
   if (commit_entities.size() == 0)
     return std::unique_ptr<CommitContribution>();
 
-  return std::unique_ptr<CommitContribution>(
-      new NonBlockingTypeCommitContribution(data_type_state_.type_context(),
-                                            commit_entities, this));
+  return base::MakeUnique<NonBlockingTypeCommitContribution>(
+      data_type_state_.type_context(), commit_entities, this);
 }
 
 void ModelTypeWorker::OnCommitResponse(CommitResponseDataList* response_list) {

[skym, 2016-09-22 20:50:32]

The CQ bit was checked by skym@chromium.org

[skym, 2016-09-22 20:50:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxbogue@chromium.org
Link to the patchset: https://codereview.chromium.org/2350803005/#ps60001
(title: "Merging conflict.")

[commit-bot: I haz the power, 2016-09-22 20:51:17]

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, 2016-09-22 21:46:36]

Description was changed from

==========
[Sync] Fixing two bugs in the worker revealed by trying to add an encryption
integration test.

The first bug was we were trying to start a nested cycle cycle, which is not
allowed. The logic in the worker that tries to nudge in response to encryption
changing is super old, and doesn't make any sense, so this was fixed by simply
not nudging. If there needs to be a commit in response to encryption changing,
the processor will drive this.

The second issue was that in certain cases the worker would try to commit
something with an id but not a version. This isn't supposed to be able to
happen. Turns out two quick commits could get us into this state because both
would be created by the processor before getting valid version data. However,
the WorkerEntityTracker can detect this is happening and has enough information
to make things right.

BUG=647875
==========

to

==========
[Sync] Fixing two bugs in the worker revealed by trying to add an encryption
integration test.

The first bug was we were trying to start a nested cycle cycle, which is not
allowed. The logic in the worker that tries to nudge in response to encryption
changing is super old, and doesn't make any sense, so this was fixed by simply
not nudging. If there needs to be a commit in response to encryption changing,
the processor will drive this.

The second issue was that in certain cases the worker would try to commit
something with an id but not a version. This isn't supposed to be able to
happen. Turns out two quick commits could get us into this state because both
would be created by the processor before getting valid version data. However,
the WorkerEntityTracker can detect this is happening and has enough information
to make things right.

BUG=647875
==========

[commit-bot: I haz the power, 2016-09-22 21:46:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-22 21:50:06]

Description was changed from

==========
[Sync] Fixing two bugs in the worker revealed by trying to add an encryption
integration test.

The first bug was we were trying to start a nested cycle cycle, which is not
allowed. The logic in the worker that tries to nudge in response to encryption
changing is super old, and doesn't make any sense, so this was fixed by simply
not nudging. If there needs to be a commit in response to encryption changing,
the processor will drive this.

The second issue was that in certain cases the worker would try to commit
something with an id but not a version. This isn't supposed to be able to
happen. Turns out two quick commits could get us into this state because both
would be created by the processor before getting valid version data. However,
the WorkerEntityTracker can detect this is happening and has enough information
to make things right.

BUG=647875
==========

to

==========
[Sync] Fixing two bugs in the worker revealed by trying to add an encryption
integration test.

The first bug was we were trying to start a nested cycle cycle, which is not
allowed. The logic in the worker that tries to nudge in response to encryption
changing is super old, and doesn't make any sense, so this was fixed by simply
not nudging. If there needs to be a commit in response to encryption changing,
the processor will drive this.

The second issue was that in certain cases the worker would try to commit
something with an id but not a version. This isn't supposed to be able to
happen. Turns out two quick commits could get us into this state because both
would be created by the processor before getting valid version data. However,
the WorkerEntityTracker can detect this is happening and has enough information
to make things right.

BUG=647875

Committed: https://crrev.com/946cbb1044cabe81b6a6758f9140089e60fc1950
Cr-Commit-Position: refs/heads/master@{#420471}
==========

[commit-bot: I haz the power, 2016-09-22 21:50:07]

Patchset 4 (id:??) landed as
https://crrev.com/946cbb1044cabe81b6a6758f9140089e60fc1950
Cr-Commit-Position: refs/heads/master@{#420471}