sync: Add non-blocking type encryption support

sync: Add non-blocking type encryption support

Introduces the framework for dealing with sync encryption in
non-blocking types.  Unlike directory sync types, non-blocking type
encryption only encrypts data before it is sent to the server.
Encrypting the data on-disk is a separate problem.

Adds code to the ModelTypeSyncWorker so it can access the directory's
cryptographer (through a CryptographerProvider interface) and use it to
encrypt entities before it sends them to the server.  If the
cryptographer is unable to encrypt with the desired key, the worker will
not commit until the cryptographer returns to a good state.

Adds the concept of a "desired encryption key" to the data type state.
When the cryptographer key to be used to encrypt a type changes, this
will be reflected in the data type state.  The ModelTypeSyncProxy is
responsible for ensuring that all items which have not yet been
encrypted with this desired key are enqueued for commit.

Makes the ModelTypeSyncWorker, EntityTracker, and ModelTypeSyncProxy
collaborate on the management of undecryptable (inapplicable) updates.
The EntityTracker keeps track of their version numbers and content, and
prevents the committing of new items to the server until the
inapplicable update has been dealt with.  The ModelTypeSyncProxy is
responsible for saving inapplicable updates across restarts.

This CL alone is not enough to enable encryption support for
non-blocking types.  It requires additional code to hook up the
ModelTypeSyncWorkers to receive cryptographer events.  This will be
added in a future commit.  In the meantime, this CL includes plenty
of unit tests to verify the functionality that's being added.

BUG=351005
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287428

[rlarocque, 2014-07-29 21:03:22]

Here's the bulk of the encryption code.  It's a big CL, but more than half of it
is in tests, so I'm hoping the review won't be too onerous.  Let me know if
you'd like to try to split it up somehow.

Please review.

[stanisc, 2014-07-30 00:30:05]

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.h
File sync/engine/entity_tracker.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.h...
sync/engine/entity_tracker.h:166: // An update for this item which can't be
applied right now.  The presence of
"Inapplicable" sounds to me like it can't be applied at all. If this can't be
applied right now but could be applied later, perhaps, it should be called
differently. How about "pending"?

Also in another file you mentioned that inapplicable updates are usually caused
by a temporary decryption failure. How about mentioning that here as well?

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_proxy_impl.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_proxy_impl.h:140: STLValueDeleter<UpdateMap>
inapplicable_updates_deleter_;
Since we seem to be using this map/deleted pair a lot, does it make sense to
introduce a class that combines both? We don't have to do that as a part of this
change but you should consider filing a bug.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_proxy_impl_unittest.cc (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_proxy_impl_unittest.cc:81: // Inapplicable updates
are usually caused by a temprorary decryption failure.
temporary

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_worker_impl.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_worker_impl.h:113: // encryption settings in a good
state.
merge with the previous line

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_worker_impl.h:130: static bool
DecryptSpecifics(Cryptographer* cryptographer,
Consider calling this TryDecryptSpecifics. Also consider explaining the return
value.

https://codereview.chromium.org/423193002/diff/1/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/util/cryptographer.h#ne...
sync/util/cryptographer.h:180: std::string GetDefaultKeyName() const;
Consider disambiguating this function from the one below. Both functions get a
Nigori key and both return a string. And their names are very similar.

[Nicolas Zea, 2014-07-30 00:34:16]

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.cc
File sync/engine/entity_tracker.cc (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.c...
sync/engine/entity_tracker.cc:196: if (version > highest_gu_response_version_) {
It seems like it might be better to do:
if (version <= highest...)
  return

That way old versions have no effect (such as clearing pending commit)

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.c...
sync/engine/entity_tracker.cc:212: if (data.response_version >=
highest_gu_response_version_) {
nit: I find it cleaner to just return early here too if we're not going to do
anything

Also, what is the expected behavior if the version is the same? Receive it or
ignore it?

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_entity.h
File sync/engine/model_type_entity.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_entit...
sync/engine/model_type_entity.h:92: void RequireEncryptionKey(const std::string&
name);
Seems odd that a method called "RequireEncryptionKey" would schedule a commit.
When is this expected to be called?

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_proxy_impl.cc (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_proxy_impl.cc:195: const UpdateResponseDataList&
inapplicable_updates) {
I find it confusing that this type is a list, while inapplicable_updates_ is a
map. Perhaps call this one list?

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_proxy_impl.cc:308: inapplicable_updates_.clear();
STLDeleteValues calls clear for you.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_proxy_impl_unittest.cc (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_proxy_impl_unittest.cc:246: const std::string&
client_tag_hash = GenerateTagHash(tag);
Why make this a ref (here and below)? GenerateTagHash and GetInapplicableUpdates
are returning refs as far as I saw.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_worker_impl.cc (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_worker_impl.cc:382: if (IsEncryptionRequired() &&
cryptographer) {
Seems like if IsEncryptionRequired() is true, we should maybe
CHECK(cryptographer)

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_worker_impl.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_worker_impl.h:113: // encryption settings in a good
state.
nit: merge onto previous line

https://codereview.chromium.org/423193002/diff/1/sync/sync_core.gypi
File sync/sync_core.gypi (right):

https://codereview.chromium.org/423193002/diff/1/sync/sync_core.gypi#newcode93
sync/sync_core.gypi:93: 'engine/sync_engine_event_listener.h',
added by mistake?

[rlarocque, 2014-07-30 21:56:01]

Here are responses to the first round of reviews.  Patch is updated, too.  PTAL.

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.cc
File sync/engine/entity_tracker.cc (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.c...
sync/engine/entity_tracker.cc:196: if (version > highest_gu_response_version_) {
On 2014/07/30 00:34:15, Nicolas Zea wrote:
> It seems like it might be better to do:
> if (version <= highest...)
>   return
> 
> That way old versions have no effect (such as clearing pending commit)

Done.

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.c...
sync/engine/entity_tracker.cc:212: if (data.response_version >=
highest_gu_response_version_) {
On 2014/07/30 00:34:15, Nicolas Zea wrote:
> nit: I find it cleaner to just return early here too if we're not going to do
> anything

Done.

> Also, what is the expected behavior if the version is the same? Receive it or
> ignore it? 

Technically it shouldn't matter, since the entities will always be identical if
they have the same ID and version.  Whether we decide to treat it as a new
update and apply it, or treat it as an old update and discard it, there should
be no practical effect in the end.

I'm using a bit of subtlety to make the code simpler, though.  In the
ModelTypeSyncWorker, I sometimes use the pattern:

EntityTracker* et = EntityTracker::InitFromUpdate(inapplicable);
et->ReceiveInapplicableUpdate(inapplicable);

That wouldn't work very well if we dropped reflections.

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.h
File sync/engine/entity_tracker.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.h...
sync/engine/entity_tracker.h:166: // An update for this item which can't be
applied right now.  The presence of
On 2014/07/30 00:30:04, stanisc wrote:
> "Inapplicable" sounds to me like it can't be applied at all. If this can't be
> applied right now but could be applied later, perhaps, it should be called
> differently. How about "pending"?

Nicolas mentioned that he might need to the concept of "inapplicable updates"
for a different feature.  I'll let him comment on this.

If forward-compatibility is not a concern, then I would advocate calling them
simply "undecryptable updates".

> Also in another file you mentioned that inapplicable updates are usually
caused
> by a temporary decryption failure. How about mentioning that here as well?

Done.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_entity.h
File sync/engine/model_type_entity.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_entit...
sync/engine/model_type_entity.h:92: void RequireEncryptionKey(const std::string&
name);
On 2014/07/30 00:34:16, Nicolas Zea wrote:
> Seems odd that a method called "RequireEncryptionKey" would schedule a commit.
> When is this expected to be called?

When the ModelTypeSyncProxy receives a new encryption key, it will call this
method on all of its items.  If the item's encryption key does not match, it
"sets the unsynced bit" for that item.

This method does not directly request a commit, but it does ensure the item will
be picked up in the next commit batch, which should happen soon after.

Not sure what a better name would be.  Something like
"MarkForCommitIfEncryptionKeyIsOld" would work, but it's a bit long.  I'm
looking for something more concise.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_proxy_impl.cc (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_proxy_impl.cc:195: const UpdateResponseDataList&
inapplicable_updates) {
On 2014/07/30 00:34:16, Nicolas Zea wrote:
> I find it confusing that this type is a list, while inapplicable_updates_ is a
> map. Perhaps call this one list?

How about new_inapplicable_updates?

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_proxy_impl.cc:308: inapplicable_updates_.clear();
On 2014/07/30 00:34:16, Nicolas Zea wrote:
> STLDeleteValues calls clear for you.

Fixed.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_proxy_impl.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_proxy_impl.h:140: STLValueDeleter<UpdateMap>
inapplicable_updates_deleter_;
On 2014/07/30 00:30:04, stanisc wrote:
> Since we seem to be using this map/deleted pair a lot, does it make sense to
> introduce a class that combines both? We don't have to do that as a part of
this
> change but you should consider filing a bug.

Done: crbug.com/398944

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_proxy_impl_unittest.cc (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_proxy_impl_unittest.cc:81: // Inapplicable updates
are usually caused by a temprorary decryption failure.
On 2014/07/30 00:30:04, stanisc wrote:
> temporary

Done.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_proxy_impl_unittest.cc:246: const std::string&
client_tag_hash = GenerateTagHash(tag);
On 2014/07/30 00:34:16, Nicolas Zea wrote:
> Why make this a ref (here and below)? GenerateTagHash and
GetInapplicableUpdates
> are returning refs as far as I saw.

Premature optimization, I suppose.  Removed the '&' to make this conform with
the conventions in the rest of the file.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_worker_impl.cc (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_worker_impl.cc:382: if (IsEncryptionRequired() &&
cryptographer) {
On 2014/07/30 00:34:16, Nicolas Zea wrote:
> Seems like if IsEncryptionRequired() is true, we should maybe
> CHECK(cryptographer)

Good point.  I don't know why it would be NULL.

Deleted the '&& cryptographer' part so we'll crash on de-referencing a NULL
pointer if it doesn't exist.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_worker_impl.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_worker_impl.h:113: // encryption settings in a good
state.
On 2014/07/30 00:30:05, stanisc wrote:
> merge with the previous line

Done.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_worker_impl.h:130: static bool
DecryptSpecifics(Cryptographer* cryptographer,
On 2014/07/30 00:30:04, stanisc wrote:
> Consider calling this TryDecryptSpecifics. Also consider explaining the return
> value.

I'm reluctant to call it TryDecryptSpecifics() because it should never fail.  If
CanDecrypt() returns true, then there's no good reason why this call would fail.
 The only possible explanation I can think of would be a logic error in this
client, or in some other client connected to this user's account.  Since the
error could reside on another client, I'd prefer to not DCHECK here, but this is
truly an exceptional case.

Updated the comment.

https://codereview.chromium.org/423193002/diff/1/sync/sync_core.gypi
File sync/sync_core.gypi (right):

https://codereview.chromium.org/423193002/diff/1/sync/sync_core.gypi#newcode93
sync/sync_core.gypi:93: 'engine/sync_engine_event_listener.h',
On 2014/07/30 00:34:16, Nicolas Zea wrote:
> added by mistake?

Probably.  Might have been a bad merge.  Removed.

https://codereview.chromium.org/423193002/diff/1/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/util/cryptographer.h#ne...
sync/util/cryptographer.h:180: std::string GetDefaultKeyName() const;
On 2014/07/30 00:30:05, stanisc wrote:
> Consider disambiguating this function from the one below. Both functions get a
> Nigori key and both return a string. And their names are very similar.

Would renaming this function to GetDefaultNigoriKeyName help?

[stanisc, 2014-07-30 22:20:55]

https://codereview.chromium.org/423193002/diff/1/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/util/cryptographer.h#ne...
sync/util/cryptographer.h:180: std::string GetDefaultKeyName() const;
On 2014/07/30 21:56:01, rlarocque wrote:
> On 2014/07/30 00:30:05, stanisc wrote:
> > Consider disambiguating this function from the one below. Both functions get
a
> > Nigori key and both return a string. And their names are very similar.
> 
> Would renaming this function to GetDefaultNigoriKeyName help?

I guess so. Call this one GetDefaultNigoriKeyName and the other one -
GetDefaultNigoriKeyData.

[rlarocque, 2014-07-30 22:28:48]

https://codereview.chromium.org/423193002/diff/1/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/util/cryptographer.h#ne...
sync/util/cryptographer.h:180: std::string GetDefaultKeyName() const;
On 2014/07/30 22:20:55, stanisc wrote:
> On 2014/07/30 21:56:01, rlarocque wrote:
> > On 2014/07/30 00:30:05, stanisc wrote:
> > > Consider disambiguating this function from the one below. Both functions
get
> a
> > > Nigori key and both return a string. And their names are very similar.
> > 
> > Would renaming this function to GetDefaultNigoriKeyName help?
> 
> I guess so. Call this one GetDefaultNigoriKeyName and the other one -
> GetDefaultNigoriKeyData.

Done.

[Nicolas Zea, 2014-07-31 18:06:27]

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.h
File sync/engine/entity_tracker.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.h...
sync/engine/entity_tracker.h:166: // An update for this item which can't be
applied right now.  The presence of
On 2014/07/30 21:56:01, rlarocque wrote:
> On 2014/07/30 00:30:04, stanisc wrote:
> > "Inapplicable" sounds to me like it can't be applied at all. If this can't
be
> > applied right now but could be applied later, perhaps, it should be called
> > differently. How about "pending"?
> 
> Nicolas mentioned that he might need to the concept of "inapplicable updates"
> for a different feature.  I'll let him comment on this.
> 
> If forward-compatibility is not a concern, then I would advocate calling them
> simply "undecryptable updates".
> 
> > Also in another file you mentioned that inapplicable updates are usually
> caused
> > by a temporary decryption failure. How about mentioning that here as well?
> 
> Done.

Eventually we want to support datatype specific conflict resolution, wherein an
update can remain unapplied while the datatype itself decides how the conflict
should be resolved. In that case, I think it makes sense to keep those updates
around as updates that could not be applied. 

Something like pending_updates is fine though. Undecryptable updates will remain
pending until the key is provided.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_entity.h
File sync/engine/model_type_entity.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_entit...
sync/engine/model_type_entity.h:92: void RequireEncryptionKey(const std::string&
name);
On 2014/07/30 21:56:01, rlarocque wrote:
> On 2014/07/30 00:34:16, Nicolas Zea wrote:
> > Seems odd that a method called "RequireEncryptionKey" would schedule a
commit.
> > When is this expected to be called?
> 
> When the ModelTypeSyncProxy receives a new encryption key, it will call this
> method on all of its items.  If the item's encryption key does not match, it
> "sets the unsynced bit" for that item.
> 
> This method does not directly request a commit, but it does ensure the item
will
> be picked up in the next commit batch, which should happen soon after.
> 
> Not sure what a better name would be.  Something like
> "MarkForCommitIfEncryptionKeyIsOld" would work, but it's a bit long.  I'm
> looking for something more concise. 

How about just UpdateServerEncryptionKey?

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_proxy_impl.cc (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_proxy_impl.cc:195: const UpdateResponseDataList&
inapplicable_updates) {
On 2014/07/30 21:56:01, rlarocque wrote:
> On 2014/07/30 00:34:16, Nicolas Zea wrote:
> > I find it confusing that this type is a list, while inapplicable_updates_ is
a
> > map. Perhaps call this one list?
> 
> How about new_inapplicable_updates?

Or just naming the member variable inapplicable_update_map_

[rlarocque, 2014-07-31 18:14:38]

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_entity.h
File sync/engine/model_type_entity.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_entit...
sync/engine/model_type_entity.h:92: void RequireEncryptionKey(const std::string&
name);
On 2014/07/31 18:06:26, Nicolas Zea wrote:
> On 2014/07/30 21:56:01, rlarocque wrote:
> > On 2014/07/30 00:34:16, Nicolas Zea wrote:
> > > Seems odd that a method called "RequireEncryptionKey" would schedule a
> commit.
> > > When is this expected to be called?
> > 
> > When the ModelTypeSyncProxy receives a new encryption key, it will call this
> > method on all of its items.  If the item's encryption key does not match, it
> > "sets the unsynced bit" for that item.
> > 
> > This method does not directly request a commit, but it does ensure the item
> will
> > be picked up in the next commit batch, which should happen soon after.
> > 
> > Not sure what a better name would be.  Something like
> > "MarkForCommitIfEncryptionKeyIsOld" would work, but it's a bit long.  I'm
> > looking for something more concise. 
> 
> How about just UpdateServerEncryptionKey?

UpdateDesiredServerEncryptionKey?  The actual update happens later, on a
different thread.

[Nicolas Zea, 2014-07-31 18:26:53]

On 2014/07/31 18:14:38, rlarocque wrote:
>
https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_entity.h
> File sync/engine/model_type_entity.h (right):
> 
>
https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_entit...
> sync/engine/model_type_entity.h:92: void RequireEncryptionKey(const
std::string&
> name);
> On 2014/07/31 18:06:26, Nicolas Zea wrote:
> > On 2014/07/30 21:56:01, rlarocque wrote:
> > > On 2014/07/30 00:34:16, Nicolas Zea wrote:
> > > > Seems odd that a method called "RequireEncryptionKey" would schedule a
> > commit.
> > > > When is this expected to be called?
> > > 
> > > When the ModelTypeSyncProxy receives a new encryption key, it will call
this
> > > method on all of its items.  If the item's encryption key does not match,
it
> > > "sets the unsynced bit" for that item.
> > > 
> > > This method does not directly request a commit, but it does ensure the
item
> > will
> > > be picked up in the next commit batch, which should happen soon after.
> > > 
> > > Not sure what a better name would be.  Something like
> > > "MarkForCommitIfEncryptionKeyIsOld" would work, but it's a bit long.  I'm
> > > looking for something more concise. 
> > 
> > How about just UpdateServerEncryptionKey?
> 
> UpdateDesiredServerEncryptionKey?  The actual update happens later, on a
> different thread.

Can probably just shorten that to UpdateDesiredEncryptionKey

[rlarocque, 2014-07-31 18:54:04]

Patch updated with renames.  PTAL.

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.h
File sync/engine/entity_tracker.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/entity_tracker.h...
sync/engine/entity_tracker.h:166: // An update for this item which can't be
applied right now.  The presence of
On 2014/07/31 18:06:26, Nicolas Zea wrote:
> On 2014/07/30 21:56:01, rlarocque wrote:
> > On 2014/07/30 00:30:04, stanisc wrote:
> > > "Inapplicable" sounds to me like it can't be applied at all. If this can't
> be
> > > applied right now but could be applied later, perhaps, it should be called
> > > differently. How about "pending"?
> > 
> > Nicolas mentioned that he might need to the concept of "inapplicable
updates"
> > for a different feature.  I'll let him comment on this.
> > 
> > If forward-compatibility is not a concern, then I would advocate calling
them
> > simply "undecryptable updates".
> > 
> > > Also in another file you mentioned that inapplicable updates are usually
> > caused
> > > by a temporary decryption failure. How about mentioning that here as well?
> > 
> > Done.
> 
> Eventually we want to support datatype specific conflict resolution, wherein
an
> update can remain unapplied while the datatype itself decides how the conflict
> should be resolved. In that case, I think it makes sense to keep those updates
> around as updates that could not be applied. 
> 
> Something like pending_updates is fine though. Undecryptable updates will
remain
> pending until the key is provided.

Done.  Renamed all instances of inapplicable to pending throughout the CL.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_entity.h
File sync/engine/model_type_entity.h (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_entit...
sync/engine/model_type_entity.h:92: void RequireEncryptionKey(const std::string&
name);
On 2014/07/31 18:14:37, rlarocque wrote:
> On 2014/07/31 18:06:26, Nicolas Zea wrote:
> > On 2014/07/30 21:56:01, rlarocque wrote:
> > > On 2014/07/30 00:34:16, Nicolas Zea wrote:
> > > > Seems odd that a method called "RequireEncryptionKey" would schedule a
> > commit.
> > > > When is this expected to be called?
> > > 
> > > When the ModelTypeSyncProxy receives a new encryption key, it will call
this
> > > method on all of its items.  If the item's encryption key does not match,
it
> > > "sets the unsynced bit" for that item.
> > > 
> > > This method does not directly request a commit, but it does ensure the
item
> > will
> > > be picked up in the next commit batch, which should happen soon after.
> > > 
> > > Not sure what a better name would be.  Something like
> > > "MarkForCommitIfEncryptionKeyIsOld" would work, but it's a bit long.  I'm
> > > looking for something more concise. 
> > 
> > How about just UpdateServerEncryptionKey?
> 
> UpdateDesiredServerEncryptionKey?  The actual update happens later, on a
> different thread.

Done.

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
File sync/engine/model_type_sync_proxy_impl.cc (right):

https://codereview.chromium.org/423193002/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_proxy_impl.cc:195: const UpdateResponseDataList&
inapplicable_updates) {
On 2014/07/31 18:06:26, Nicolas Zea wrote:
> On 2014/07/30 21:56:01, rlarocque wrote:
> > On 2014/07/30 00:34:16, Nicolas Zea wrote:
> > > I find it confusing that this type is a list, while inapplicable_updates_
is
> a
> > > map. Perhaps call this one list?
> > 
> > How about new_inapplicable_updates?
> 
> Or just naming the member variable inapplicable_update_map_

Done.

[stanisc, 2014-08-01 19:10:15]

lgtm

[Nicolas Zea, 2014-08-04 18:05:23]

lgtm

[rlarocque, 2014-08-04 18:19:36]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-08-04 18:24:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rlarocque@chromium.org/423193002/80001

[commit-bot: I haz the power, 2014-08-05 01:06:55]

Change committed as 287428

[Lei Zhang, 2014-08-05 02:15:38]

A revert of this CL has been created in
https://codereview.chromium.org/442623002/ by thestig@chromium.org.

The reason for reverting is: Both LSAN and Valgrind complains about leaks in the
tests..