[Sync] Ensure cryptographer ready before encrypting.

[Sync] Ensure cryptographer ready before encrypting.

Also, make cryptographer more robust to errors and add error logging for cases
where the cryptographer is not ready when we expect it to be.

BUG=81152
TEST=new cryptographer unit tests and synapi unit tests.


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

[Nicolas Zea, 2011-06-10 20:43:54]

This patch should both prevent us from attempting to encrypt when the
cryptographer is not ready (we should trigger a PASSPHRASE_REQUIRED in
bootstrapencryption, which will eventually do a setpassphrase and retrigger the
encryptdatatypes), as well as make the cryptographer robust enough that we don't
crash but print error logs.

[tim (not reviewing), 2011-06-13 17:43:22]

http://codereview.chromium.org/7108067/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi_unittest.cc (right):

http://codereview.chromium.org/7108067/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_unittest.cc:1368:
password_node.SetPasswordSpecifics(data);
Is this legal?

http://codereview.chromium.org/7108067/diff/1/chrome/browser/sync/util/nigori.cc
File chrome/browser/sync/util/nigori.cc (right):

http://codereview.chromium.org/7108067/diff/1/chrome/browser/sync/util/nigori...
chrome/browser/sync/util/nigori.cc:172: return false;
sure we don't want if ... { NOTREACHED(); return false } ?

[Nicolas Zea, 2011-06-13 18:38:21]

http://codereview.chromium.org/7108067/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncapi_unittest.cc (right):

http://codereview.chromium.org/7108067/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncapi_unittest.cc:1368:
password_node.SetPasswordSpecifics(data);
On 2011/06/13 17:43:22, timsteele wrote:
> Is this legal?

Not really, but I primarily want to test that SetPassphrase doesn't choke when
it happens (we shouldn't deliberately get into this situation, but it might be
possible if chrome crashes in the middle of writing a node). I've modified it to
just not set the password specifics, and brought back the notreached in there.

http://codereview.chromium.org/7108067/diff/1/chrome/browser/sync/util/nigori.cc
File chrome/browser/sync/util/nigori.cc (right):

http://codereview.chromium.org/7108067/diff/1/chrome/browser/sync/util/nigori...
chrome/browser/sync/util/nigori.cc:172: return false;
On 2011/06/13 17:43:22, timsteele wrote:
> sure we don't want if ... { NOTREACHED(); return false } ?

I have the NOTREACHED at a higher level (EncryptIfNecessary and
SetPasswordSpecifics). This way, we can still have tests that ensure everything
is handled gracefully.

[Nicolas Zea, 2011-06-13 23:45:22]

PTAL. I've gone ahead and implemented the design we discussed. Namely, bootstrap
encryption now does only that, and the actual updating from the nigori node only
occurs after we have set syncapi_intiialized_ to true in the SBH.

[tim (not reviewing), 2011-06-14 01:32:58]

http://codereview.chromium.org/7108067/diff/6001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7108067/diff/6001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:3000: if
(data_->UpdateCryptographerFromNigori())
is it possible for is_ready() to change from true to false after this call?

http://codereview.chromium.org/7108067/diff/6001/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/7108067/diff/6001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/sync_backend_host.cc:983:
core_->syncapi()->ReloadNigori();
Im trying to think of a better name for this...  Maybe RefreshCryptographer or
RefreshEncryption?  

I'll be adding some call to download nigori here :\  or were you intending that
to happen under the covers in ReloadNigori?

[Nicolas Zea, 2011-06-14 16:52:37]

PTAL

http://codereview.chromium.org/7108067/diff/6001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/7108067/diff/6001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:3000: if
(data_->UpdateCryptographerFromNigori())
On 2011/06/14 01:32:58, timsteele wrote:
> is it possible for is_ready() to change from true to false after this call?

You're right, once we release the transaction it's possible a new passphrase
will arrive. But, in EncryptDataTypes, as long as the cryptographer has been
initialized, we can still encrypt with the old passphrase. I've changed
EncryptDataTypes to just ensure the cryptographer is initialized.

http://codereview.chromium.org/7108067/diff/6001/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/7108067/diff/6001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/sync_backend_host.cc:983:
core_->syncapi()->ReloadNigori();
On 2011/06/14 01:32:58, timsteele wrote:
> Im trying to think of a better name for this...  Maybe RefreshCryptographer or
> RefreshEncryption?  
> 
> I'll be adding some call to download nigori here :\  or were you intending
that
> to happen under the covers in ReloadNigori? 

Done.

[tim (not reviewing), 2011-06-14 17:00:33]

LGTM

[commit-bot: I haz the power, 2011-06-14 18:55:37]

Change committed as 89038