sync: Finish non-blocking type encryption support

sync: Finish non-blocking type encryption support

Undoes some previous work towards encryption support.  That approach
suffered from some subtle deadlock issues that could not be easily
worked around.  The new approach involves less sharing and less locks.

Gives the ModelTypeSyncWorker its own copy of the Cryptographer.  By
passing around copies, it no longer needs to worry about acquiring locks
in order to access the Directory's cryptographer.  This required
a rewrite of some changes to the way the ModelTypeSyncWorker detects the
current encryption state.  Most notably, its Cryptographer is NULL if
encryption is not enabled for its model type.

Makes the ModelTypeSyncRegistry responsible for observing changes
emitted by the SyncEncryptionHandler and forwarding them to the
ModelTypeSyncWorkers.  It should receive callbacks from the
SyncEncryptionHandler during startup, so it does not need to cache or
query any new data.

Removes the CryptographerProviders.  Since the ModelTypeSyncWorker no
longer need to access the directory's cryptographer, it's no longer
necessary.

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

[rlarocque, 2014-08-08 22:23:07]

Sorry for the last minute changes.

The design docs for this feature had originally intended to go this route, but I
figured it would be easier to implement (and less prone to getting out of sync)
if I took a shortcut and relied on the directory's cryptographer instead.  It
was easier to implement, and the unit tests looked fine, but it deadlocked as
soon as I tried to run it in a real browser.

Here's a new attempt that doesn't require shared (and locked) access to the
directory's cryptographer.  Please review and let me know what you think of this
new approach.

[stanisc, 2014-08-11 21:05:42]

Mostly looks good, but I wonder why each model type has to have its own copy of
Cryptographer and not something like scoped_refptr?

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

https://codereview.chromium.org/452283003/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_worker_impl.cc:393:
cryptographer_->GetDefaultNigoriKeyName()) {
Should this be using new_key_name instead of
cryptographer_->GetDefaultNigoriKeyName() here and below?

https://codereview.chromium.org/452283003/diff/1/sync/sessions/model_type_reg...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/452283003/diff/1/sync/sessions/model_type_reg...
sync/sessions/model_type_registry.cc:352: make_scoped_ptr<Cryptographer>(new
Cryptographer(*cryptographer_)));
I think the "<Cryptographer>" part is redundant.

[rlarocque, 2014-08-11 21:50:14]

On 2014/08/11 21:05:42, stanisc wrote:
> Mostly looks good, but I wonder why each model type has to have its own copy
of
> Cryptographer and not something like scoped_refptr?

That's a good question.  It's a trade off.

There's no question that the current approach wastes more memory than it needs
to.  If we find this to be a problem, we'll have to revisit this decision.

The biggest issue is probably that the Cryptographer does not currently inherit
from scoped_refptr.  We'd have to wrap or subclass it if we wanted to refcount
it.

For now, I'd prefer to take the easy way out, and just pay the cost of a copy. 
The cryptographer doesn't get updated very often, so it's not a cost that we'll
have to pay very often.

[rlarocque, 2014-08-11 21:50:22]

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

https://codereview.chromium.org/452283003/diff/1/sync/engine/model_type_sync_...
sync/engine/model_type_sync_worker_impl.cc:393:
cryptographer_->GetDefaultNigoriKeyName()) {
On 2014/08/11 21:05:42, stanisc wrote:
> Should this be using new_key_name instead of
> cryptographer_->GetDefaultNigoriKeyName() here and below?

Good point.  It shouldn't make a difference, but the code will be easier to read
that way.

https://codereview.chromium.org/452283003/diff/1/sync/sessions/model_type_reg...
File sync/sessions/model_type_registry.cc (right):

https://codereview.chromium.org/452283003/diff/1/sync/sessions/model_type_reg...
sync/sessions/model_type_registry.cc:352: make_scoped_ptr<Cryptographer>(new
Cryptographer(*cryptographer_)));
On 2014/08/11 21:05:42, stanisc wrote:
> I think the "<Cryptographer>" part is redundant.

You're right.  Fixed.

[stanisc, 2014-08-12 00:30:00]

On 2014/08/11 21:50:14, rlarocque wrote:
> On 2014/08/11 21:05:42, stanisc wrote:
> > Mostly looks good, but I wonder why each model type has to have its own copy
> of
> > Cryptographer and not something like scoped_refptr?
> 
> That's a good question.  It's a trade off.
> 
> There's no question that the current approach wastes more memory than it needs
> to.  If we find this to be a problem, we'll have to revisit this decision.
> 
> The biggest issue is probably that the Cryptographer does not currently
inherit
> from scoped_refptr.  We'd have to wrap or subclass it if we wanted to refcount
> it.
> 
> For now, I'd prefer to take the easy way out, and just pay the cost of a copy.

> The cryptographer doesn't get updated very often, so it's not a cost that
we'll
> have to pay very often.

Wouldn't it be enough to just derive Cryptographer from
RefCounted<Cryptographer> or perhaps RefCountedThreadBase<Cryptographer>?
I guess changing this would trigger more changes everywhere Cryptographer is
used.

[Nicolas Zea, 2014-08-13 20:49:08]

Some comments. Also maybe add a unit test for the copy constructor for
cryptographer?

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
File sync/engine/model_type_sync_proxy_impl.cc (right):

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_proxy_impl.cc:243: DVLOG(1) <<
ModelTypeToString(type_) << ": Requesting re-encrypt commit "
nit: given this is per entity, prefer DVLOG(2)

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
File sync/engine/model_type_sync_worker_impl.cc (right):

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_worker_impl.cc:71: cryptographer_ =
cryptographer.Pass();
nit: DCHECK(cryptographer)?

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_worker_impl.cc:348: (!cryptographer_ ||
!cryptographer_->is_ready()))
nit: curly braces for multi-line body

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_worker_impl.cc:367: DCHECK(CanCommitItems());
nit: pull this out of the if statement? Should always be true for this method
right?

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
File sync/engine/model_type_sync_worker_impl_unittest.cc (right):

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_worker_impl_unittest.cc:278:
cryptographer_copy.reset(new Cryptographer(*cryptographer_));
nit: remove curly brace if that's the style of the file

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_worker_impl_unittest.cc:958: // The worker should
the model thread about encryption as soon as possible,
nit: should the -> should tell the?

https://codereview.chromium.org/452283003/diff/20001/sync/internal_api/sync_m...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/452283003/diff/20001/sync/internal_api/sync_m...
sync/internal_api/sync_manager_impl.cc:612: if (model_type_registry_) {
nit: remove curly brace

https://codereview.chromium.org/452283003/diff/20001/sync/sessions/model_type...
File sync/sessions/model_type_registry.h (right):

https://codereview.chromium.org/452283003/diff/20001/sync/sessions/model_type...
sync/sessions/model_type_registry.h:73: // Implementation of
NonBlockingEncryptionCacheObserver
Is this just SyncEncryptionHandlerObserver?

[rlarocque, 2014-08-15 18:33:18]

Sorry for the slow response.  Patch updated; PTAL.

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
File sync/engine/model_type_sync_proxy_impl.cc (right):

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_proxy_impl.cc:243: DVLOG(1) <<
ModelTypeToString(type_) << ": Requesting re-encrypt commit "
On 2014/08/13 20:49:07, Nicolas Zea wrote:
> nit: given this is per entity, prefer DVLOG(2)

Done.  I fixed this log statement, and a few others.

While I was at it, I also removed some VLOG(1) statements that had been
accidentally left in model_type_registry.cc.

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
File sync/engine/model_type_sync_worker_impl.cc (right):

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_worker_impl.cc:71: cryptographer_ =
cryptographer.Pass();
On 2014/08/13 20:49:07, Nicolas Zea wrote:
> nit: DCHECK(cryptographer)?

Done.

This implies that we never disable encryption once it's been turned on.  Which
is true.

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_worker_impl.cc:348: (!cryptographer_ ||
!cryptographer_->is_ready()))
On 2014/08/13 20:49:07, Nicolas Zea wrote:
> nit: curly braces for multi-line body

Done.

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_worker_impl.cc:367: DCHECK(CanCommitItems());
On 2014/08/13 20:49:07, Nicolas Zea wrote:
> nit: pull this out of the if statement? Should always be true for this method
> right?

Done.

This DCHECK is here not so much for correctness, but to make it clear why it's
safe to dereference the cryptographer and call cryptographer_->Encrypt() in this
context.  If the reader didn't know that this function can only be called when
CanCommitItems() is true, this code might look like a bug.

I've updated the comment and moved things around a bit to better match the
assumptions used in the new approach to encryption.

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
File sync/engine/model_type_sync_worker_impl_unittest.cc (right):

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_worker_impl_unittest.cc:278:
cryptographer_copy.reset(new Cryptographer(*cryptographer_));
On 2014/08/13 20:49:07, Nicolas Zea wrote:
> nit: remove curly brace if that's the style of the file

Added curlies everywhere.  If we're going to be consistent, let's do it the
right way.  :)

https://codereview.chromium.org/452283003/diff/20001/sync/engine/model_type_s...
sync/engine/model_type_sync_worker_impl_unittest.cc:958: // The worker should
the model thread about encryption as soon as possible,
On 2014/08/13 20:49:07, Nicolas Zea wrote:
> nit: should the -> should tell the?

Done.

https://codereview.chromium.org/452283003/diff/20001/sync/sessions/model_type...
File sync/sessions/model_type_registry.h (right):

https://codereview.chromium.org/452283003/diff/20001/sync/sessions/model_type...
sync/sessions/model_type_registry.h:73: // Implementation of
NonBlockingEncryptionCacheObserver
On 2014/08/13 20:49:08, Nicolas Zea wrote:
> Is this just SyncEncryptionHandlerObserver?

Correct.  That name was left over from a path abandoned during development.

[rlarocque, 2014-08-15 19:08:15]

Patch updated again with a test for the cryptographer's copy constructor.

https://codereview.chromium.org/452283003/diff/20001/sync/internal_api/sync_m...
File sync/internal_api/sync_manager_impl.cc (right):

https://codereview.chromium.org/452283003/diff/20001/sync/internal_api/sync_m...
sync/internal_api/sync_manager_impl.cc:612: if (model_type_registry_) {
On 2014/08/13 20:49:08, Nicolas Zea wrote:
> nit: remove curly brace

Done.

[Nicolas Zea, 2014-08-15 20:43:05]

lgtm

[rlarocque, 2014-08-15 20:47:09]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-08-15 20:50:37]

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

[commit-bot: I haz the power, 2014-08-16 00:34:19]

Committed patchset #6 (100001) as 290067