[Sync] Add keystore migration conflict support.

[Sync] Add keystore migration conflict support.

We rely on the encryption handler's rollback detection for any cases
where conflicts might lose data and we can't merge. Else, we merge the
data as needed.

BUG=129665
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157186

[Nicolas Zea, 2012-09-10 21:38:05]

PTAL. This builds on the keystore migration patch (including fixing a conflict
case I had missed)

[Nicolas Zea, 2012-09-13 19:56:39]

+Richard, since Tim is going to be out for a bit (and this is conflict related
anyways).

[rlarocque, 2012-09-13 22:10:51]

Some comments.  I'm still struggling to wrap my head around the new encryption
code.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
File sync/engine/apply_control_data_updates.cc (right):

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:120: // keystore decryptor since this
only happens if local has an explicit
DCHECK that assumption?

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:128:
cryptographer->GetKeys(new_nigori->mutable_encryption_keybag());
It seems you do this almost everywhere.  Why not make it contingent solely on
cryptographer->is_ready()?  Are there cases where you want to avoid calling
GetKeys() on the new (from server) keybag?

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:142: if
(local_nigori.has_passphrase_type()) {
Let's make the division of cases more explicit.  It seems to me that there are
four main cases here:
- both are aware of passphrase_type
- neither are aware of passphrase_type
- local only is aware of passphrase type
- server only is aware of passphrase type

The last two are currently handled inside this else branch. Can they be moved
out to the higher level of else if branches?

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:154: nigori_node.Put(SPECIFICS,
new_specifics);
SetNigoriSpecifics(), which seems to be used for nigori writing most of the
time, will preserve unknown fields.  This will not.  

I'm guessing this doesn't matter because the unknown fields would have to be
delivered by the server, and the new_specifics here will contain the server's
unkown fields.  Still, just thinking about this makes my head hurt.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
File sync/engine/apply_control_data_updates_unittest.cc (right):

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:59: // Verify that applying a
nigori node updates sets initial sync ended properly
typo: "updates sets".

Add comma after properly.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:315: ModelTypeSet
encrypted_types(PASSWORDS);
Consider using SensitiveTypes() here.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:345: // Set up a local nigori
with the local encryptio keys and default encrypted
encryptio

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:425: // Set up a local nigori
with the local encryptio keys and default encrypted
Typo here, too.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:542: // If both nigoris are
migrated, but we also set a custom passphrase locally,
Are you testing the other side of the if branch anywhere?  Is there a test for
the case where both are migrated, and the expression

(server_nigori.passphrase_type() ==
sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE && local_nigori.passphrase_type()
!= sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE)

is false?

http://codereview.chromium.org/10905191/diff/4013/sync/internal_api/public/te...
File sync/internal_api/public/test/test_entry_factory.h (right):

http://codereview.chromium.org/10905191/diff/4013/sync/internal_api/public/te...
sync/internal_api/public/test/test_entry_factory.h:22: SERVER_SPECIFICS,
This identifier is dangerously close to syncable::SERVER_SPECIFICS.  I think you
should find another name for it.

http://codereview.chromium.org/10905191/diff/4013/sync/internal_api/test/test...
File sync/internal_api/test/test_entry_factory.cc (right):

http://codereview.chromium.org/10905191/diff/4013/sync/internal_api/test/test...
sync/internal_api/test/test_entry_factory.cc:189: if (!entry.good()) {
DCHECK() instead?

http://codereview.chromium.org/10905191/diff/4013/sync/internal_api/test/test...
sync/internal_api/test/test_entry_factory.cc:192: if (specifics_type ==
SERVER_SPECIFICS) {
Whereas the current definition allows you to reuse ~3 lines of code, I think it
would make sense to have two different functions for this:

GetLocalSpecificsForItem() and GetServerSpecificsForItem().

That way you wouldn't need the flag or this if statement.  

I'm assuming no one will ever want to pass in a non-literal 'specifics_type'
value to this function.

[Nicolas Zea, 2012-09-13 22:50:14]

All comments addressed, PTAL.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
File sync/engine/apply_control_data_updates.cc (right):

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:120: // keystore decryptor since this
only happens if local has an explicit
On 2012/09/13 22:10:51, rlarocque wrote:
> DCHECK that assumption?

Done.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:128:
cryptographer->GetKeys(new_nigori->mutable_encryption_keybag());
On 2012/09/13 22:10:51, rlarocque wrote:
> It seems you do this almost everywhere.  Why not make it contingent solely on
> cryptographer->is_ready()?  Are there cases where you want to avoid calling
> GetKeys() on the new (from server) keybag?

Yes, see the "Note: we only update the encryption keybag..." comment above. In
particular, we don't want to call this if we're not sure that we are properly
updating keystore_decryptor as well (or that it doesn't matter).

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:142: if
(local_nigori.has_passphrase_type()) {
On 2012/09/13 22:10:51, rlarocque wrote:
> Let's make the division of cases more explicit.  It seems to me that there are
> four main cases here:
> - both are aware of passphrase_type
> - neither are aware of passphrase_type
> - local only is aware of passphrase type
> - server only is aware of passphrase type
> 
> The last two are currently handled inside this else branch. Can they be moved
> out to the higher level of else if branches?

Done.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates.cc:154: nigori_node.Put(SPECIFICS,
new_specifics);
On 2012/09/13 22:10:51, rlarocque wrote:
> SetNigoriSpecifics(), which seems to be used for nigori writing most of the
> time, will preserve unknown fields.  This will not.  
> 
> I'm guessing this doesn't matter because the unknown fields would have to be
> delivered by the server, and the new_specifics here will contain the server's
> unkown fields.  Still, just thinking about this makes my head hurt.

We don't have access to SetNigoriSpecifics here actually (it's a syncapi method,
not a syncable method). That said, I've gone ahead and copied over the entire
local nigori when necessary, which will preserve local unknown fields in those
cases we're not letting server win.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
File sync/engine/apply_control_data_updates_unittest.cc (right):

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:59: // Verify that applying a
nigori node updates sets initial sync ended properly
On 2012/09/13 22:10:51, rlarocque wrote:
> typo: "updates sets".
> 
> Add comma after properly.

Done.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:315: ModelTypeSet
encrypted_types(PASSWORDS);
On 2012/09/13 22:10:51, rlarocque wrote:
> Consider using SensitiveTypes() here.

Done.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:345: // Set up a local nigori
with the local encryptio keys and default encrypted
On 2012/09/13 22:10:51, rlarocque wrote:
> encryptio

Done.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:425: // Set up a local nigori
with the local encryptio keys and default encrypted
On 2012/09/13 22:10:51, rlarocque wrote:
> Typo here, too.

Done.

http://codereview.chromium.org/10905191/diff/4013/sync/engine/apply_control_d...
sync/engine/apply_control_data_updates_unittest.cc:542: // If both nigoris are
migrated, but we also set a custom passphrase locally,
On 2012/09/13 22:10:51, rlarocque wrote:
> Are you testing the other side of the if branch anywhere?  Is there a test for
> the case where both are migrated, and the expression
> 
> (server_nigori.passphrase_type() ==
> sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE &&
local_nigori.passphrase_type()
> != sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE)
> 
> is false?

Yes, BothMigratedServerCustom hits that case.

http://codereview.chromium.org/10905191/diff/4013/sync/internal_api/public/te...
File sync/internal_api/public/test/test_entry_factory.h (right):

http://codereview.chromium.org/10905191/diff/4013/sync/internal_api/public/te...
sync/internal_api/public/test/test_entry_factory.h:22: SERVER_SPECIFICS,
On 2012/09/13 22:10:51, rlarocque wrote:
> This identifier is dangerously close to syncable::SERVER_SPECIFICS.  I think
you
> should find another name for it.

Moot now that it's two methods.

http://codereview.chromium.org/10905191/diff/4013/sync/internal_api/test/test...
File sync/internal_api/test/test_entry_factory.cc (right):

http://codereview.chromium.org/10905191/diff/4013/sync/internal_api/test/test...
sync/internal_api/test/test_entry_factory.cc:189: if (!entry.good()) {
On 2012/09/13 22:10:51, rlarocque wrote:
> DCHECK() instead?

Done.

http://codereview.chromium.org/10905191/diff/4013/sync/internal_api/test/test...
sync/internal_api/test/test_entry_factory.cc:192: if (specifics_type ==
SERVER_SPECIFICS) {
On 2012/09/13 22:10:51, rlarocque wrote:
> Whereas the current definition allows you to reuse ~3 lines of code, I think
it
> would make sense to have two different functions for this:
> 
> GetLocalSpecificsForItem() and GetServerSpecificsForItem().
> 
> That way you wouldn't need the flag or this if statement.  
> 
> I'm assuming no one will ever want to pass in a non-literal 'specifics_type'
> value to this function.

Done (same for SetSpecificsForItem).

[Nicolas Zea, 2012-09-17 17:25:06]

ping!

[rlarocque, 2012-09-17 17:34:30]

Sorry, I missed the PTAL.  LGTM.

[commit-bot: I haz the power, 2012-09-17 17:57:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/10905191/14001

[commit-bot: I haz the power, 2012-09-17 20:42:03]

Change committed as 157186