Remove dependency on server generated type root folders

Remove dependency on server generated type root folders

This change prepares the client to deal with implicit permanent folders
in Sync server updates. That includes the following:
- Expect type root folders to not come from the server on the initial
  sync. The client auto-creates type root folders for all types except
  Bookmarks and Nigori when progress marker changes from empty to
  non-empty. These folders are created as local nodes (with client IDs)
  that aren't expected to sync back to the server.
- Expect empty parent IDs in updates for both new and existing items.
- Because the client code updates first, the client must expect server
  to override locally created type root folders. To enable that the
  directory update code that matches update entities to local entities
  was updated to look at server unique tags in addition to client
  unique tags. Later when the server stops generating folders we
  should be able to remove that code.
- Added some extra special cases in Directory::CheckTreeInvariants to
  deal with client side created type root folders.
- Added / modified a few tests to cover cases with implicit parent IDs
  in updates.

BUG=438313
Committed: https://crrev.com/8d4046a92a3314bf5e1b6f1dd6a3c4aed5449e64
Cr-Commit-Position: refs/heads/master@{#313977}

[stanisc, 2015-01-23 22:02:10]

stanisc@chromium.org changed reviewers:
+ zea@chromium.org

[stanisc, 2015-01-23 22:02:22]

Patchset #1 (id:1) has been deleted

[stanisc, 2015-01-23 22:02:32]

Patchset #1 (id:20001) has been deleted

[stanisc, 2015-01-23 22:02:41]

Patchset #1 (id:40001) has been deleted

[stanisc, 2015-01-23 22:02:48]

Patchset #1 (id:60001) has been deleted

[stanisc, 2015-01-23 22:06:18]

Nicolas, PTAL.
This is the third patch for this work item, after which
we should be able to switch the server to implicit 
permanent folders.
The remaining work (not included in this patch) is to 
further reduce dependency on root folders, for example
stop checking them to decide whether a type has been
initialized and use progress markers instead.

[maniscalco, 2015-01-24 00:55:59]

maniscalco@chromium.org changed reviewers:
+ maniscalco@chromium.org

[maniscalco, 2015-01-24 00:56:00]

(drive-by question...)

https://codereview.chromium.org/867793003/diff/80001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/867793003/diff/80001/sync/syncable/directory....
sync/syncable/directory.cc:929: return
kernel_->persisted_info.HasEmptyDownloadProgress(type);
Normally, when Directory methods access kernel_ they either take a ScopedLock
(like GetEntriesCount) or they assert the thread already has a lock (like
GetTransactionVersion).  Should this new method follow one of those approaches?

[stanisc, 2015-01-26 19:02:09]

Addressed issue found by Nick - added ScopedKernelLock to
Directory::HasEmptyDownloadProgress.

https://codereview.chromium.org/867793003/diff/80001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/867793003/diff/80001/sync/syncable/directory....
sync/syncable/directory.cc:929: return
kernel_->persisted_info.HasEmptyDownloadProgress(type);
On 2015/01/24 00:55:59, maniscalco wrote:
> Normally, when Directory methods access kernel_ they either take a ScopedLock
> (like GetEntriesCount) or they assert the thread already has a lock (like
> GetTransactionVersion).  Should this new method follow one of those
approaches?

Done.

[maniscalco, 2015-01-26 19:17:34]

https://codereview.chromium.org/867793003/diff/80001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/867793003/diff/80001/sync/syncable/directory....
sync/syncable/directory.cc:929: return
kernel_->persisted_info.HasEmptyDownloadProgress(type);
On 2015/01/26 19:02:09, stanisc wrote:
> On 2015/01/24 00:55:59, maniscalco wrote:
> > Normally, when Directory methods access kernel_ they either take a
ScopedLock
> > (like GetEntriesCount) or they assert the thread already has a lock (like
> > GetTransactionVersion).  Should this new method follow one of those
> approaches?
> 
> Done.

Since Directory::HasEmptyDownloadProgress is called from PurgeEntriesWithTypeIn
while a ScopedKernelLock on "this" is held, I'm thinking
Directory::HasEmptyDownloadProgress shouldn't take a lock because SKLs are
non-recursive, right?  As is, I would think the code might DCHECK:

https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...

Unless, maybe I'm missing something?

Thinking more about this... WDYT about adding const ScopedKernelLock& to
Directory::HasEmptyDownloadProgress ?  We do that elsewhere in Directory to
ensure the caller is holding a lock (e.g. ClearDirtyMetahandles).  That way,
it's harder to misuse.

[Nicolas Zea, 2015-01-26 23:21:53]

https://codereview.chromium.org/867793003/diff/100001/sync/engine/directory_u...
File sync/engine/directory_update_handler.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/engine/directory_u...
sync/engine/directory_update_handler.cc:74:
IsValidProgressMarker(progress_marker)) {
Should IsValidProgressMarker be performing the check for an empty token?

https://codereview.chromium.org/867793003/diff/100001/sync/engine/directory_u...
sync/engine/directory_update_handler.cc:95: DVLOG(1) << "Type root folder "
Comment that this is expected for older versions of the sync server?

https://codereview.chromium.org/867793003/diff/100001/sync/engine/syncer_unit...
File sync/engine/syncer_unittest.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/engine/syncer_unit...
sync/engine/syncer_unittest.cc:4262: // This verifies transition to implicit
permanent folders
nit: period at end.

https://codereview.chromium.org/867793003/diff/100001/sync/engine/syncer_util.cc
File sync/engine/syncer_util.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/engine/syncer_util...
sync/engine/syncer_util.cc:189: // The client creates type root folders on
demand with a local ID on
nit: "on demand with a local id on demand"?

https://codereview.chromium.org/867793003/diff/100001/sync/engine/syncer_util...
sync/engine/syncer_util.cc:192: // type. It will does that during the transition
period until support for
nit: " will does"

https://codereview.chromium.org/867793003/diff/100001/sync/engine/syncer_util...
sync/engine/syncer_util.cc:256: // potential errors.
nit: move to previous line

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/directory...
sync/syncable/directory.cc:959: // TODO: change these to not rely on the folders
nit: TODO's typically have an owner and a bug, e.g. TODO(stanisc):
crbug.com/xxxxx something here

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/directory...
sync/syncable/directory.h:207: size_t GetEntriesCount() const;
nit: comment description for method?

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/model_neu...
File sync/syncable/model_neutral_mutable_entry.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/model_neu...
sync/syncable/model_neutral_mutable_entry.cc:56: 
CHECK that type isn't BOOKMARKS or NIGORI?

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/syncable_...
File sync/syncable/syncable_id.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/syncable_...
sync/syncable/syncable_id.cc:38: if (!server_id.empty()) {
This just for testing right? Or was there another reason for this?

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/syncable_...
File sync/syncable/syncable_util.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/syncable_...
sync/syncable/syncable_util.cc:35: return true;
Should this be false here for bookmarks?

[stanisc, 2015-01-28 07:30:06]

Answered some questions. I'll address the remaining comments tomorrow.

https://codereview.chromium.org/867793003/diff/100001/sync/engine/directory_u...
File sync/engine/directory_update_handler.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/engine/directory_u...
sync/engine/directory_update_handler.cc:74:
IsValidProgressMarker(progress_marker)) {
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> Should IsValidProgressMarker be performing the check for an empty token?

I guess that should be OK. I'll change this.

https://codereview.chromium.org/867793003/diff/100001/sync/engine/directory_u...
sync/engine/directory_update_handler.cc:95: DVLOG(1) << "Type root folder "
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> Comment that this is expected for older versions of the sync server?

Actually for an older version of the server this would be called before the same
folder is created by the server. The only scenario where I think we might hit
this is the case where the progress marker gets reset on the client. I think
there is one codepath that does that. I'll look into that and add an appropriate
comment.

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/syncable_...
File sync/syncable/syncable_id.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/syncable_...
sync/syncable/syncable_id.cc:38: if (!server_id.empty()) {
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> This just for testing right? Or was there another reason for this?

Once the server transition is done, most of parent ID strings in server updates
would be empty. 
Checking it everywhere this function is called would be tedious.
Checking it here seemed like a much more elegant solution.

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/syncable_...
File sync/syncable/syncable_util.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/syncable_...
sync/syncable/syncable_util.cc:35: return true;
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> Should this be false here for bookmarks?

Yes, I agree. I'll see if this check could be done outside based on the
datatype.

[stanisc, 2015-01-29 00:27:38]

Addressed CR feedback and bumped client-to-server protocol version to allow the
server to distinguish an old client that doesn't support this feature from a new
one.

https://codereview.chromium.org/867793003/diff/80001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/867793003/diff/80001/sync/syncable/directory....
sync/syncable/directory.cc:929: return
kernel_->persisted_info.HasEmptyDownloadProgress(type);
On 2015/01/26 19:17:34, maniscalco wrote:
> On 2015/01/26 19:02:09, stanisc wrote:
> > On 2015/01/24 00:55:59, maniscalco wrote:
> > > Normally, when Directory methods access kernel_ they either take a
> ScopedLock
> > > (like GetEntriesCount) or they assert the thread already has a lock (like
> > > GetTransactionVersion).  Should this new method follow one of those
> > approaches?
> > 
> > Done.
> 
> Since Directory::HasEmptyDownloadProgress is called from
PurgeEntriesWithTypeIn
> while a ScopedKernelLock on "this" is held, I'm thinking
> Directory::HasEmptyDownloadProgress shouldn't take a lock because SKLs are
> non-recursive, right?  As is, I would think the code might DCHECK:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...
> 
> Unless, maybe I'm missing something?
> 
> Thinking more about this... WDYT about adding const ScopedKernelLock& to
> Directory::HasEmptyDownloadProgress ?  We do that elsewhere in Directory to
> ensure the caller is holding a lock (e.g. ClearDirtyMetahandles).  That way,
> it's harder to misuse.

Directory::HasEmptyDownloadProgress isn't called from PurgeEntriesWithTypeIn
anymore. I changed that code back to calling
Directory::PersistedKernelInfo::HasEmptyDownloadProgress directly to avoid
exactly the problem you described. I did it at the same time when I added the
lock to Directory::HasEmptyDownloadProgress. In general none of functions in
PersistedKernelInfo get ScopedKernelLock& as one of parameters. I didn't want to
change the existing pattern so I decided it would be better to inline the call
in PurgeEntriesWithTypeIn instead.

https://codereview.chromium.org/867793003/diff/100001/sync/engine/directory_u...
File sync/engine/directory_update_handler.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/engine/directory_u...
sync/engine/directory_update_handler.cc:74:
IsValidProgressMarker(progress_marker)) {
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> Should IsValidProgressMarker be performing the check for an empty token?

Done.

https://codereview.chromium.org/867793003/diff/100001/sync/engine/syncer_unit...
File sync/engine/syncer_unittest.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/engine/syncer_unit...
sync/engine/syncer_unittest.cc:4262: // This verifies transition to implicit
permanent folders
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> nit: period at end.

Done.

https://codereview.chromium.org/867793003/diff/100001/sync/engine/syncer_util.cc
File sync/engine/syncer_util.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/engine/syncer_util...
sync/engine/syncer_util.cc:189: // The client creates type root folders on
demand with a local ID on
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> nit: "on demand with a local id on demand"?

Done.

https://codereview.chromium.org/867793003/diff/100001/sync/engine/syncer_util...
sync/engine/syncer_util.cc:192: // type. It will does that during the transition
period until support for
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> nit: " will does"

Done.

https://codereview.chromium.org/867793003/diff/100001/sync/engine/syncer_util...
sync/engine/syncer_util.cc:256: // potential errors.
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> nit: move to previous line

Done.

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/directory...
sync/syncable/directory.cc:959: // TODO: change these to not rely on the folders
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> nit: TODO's typically have an owner and a bug, e.g. TODO(stanisc):
> crbug.com/xxxxx something here

Done.

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/directory.h
File sync/syncable/directory.h (right):

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/directory...
sync/syncable/directory.h:207: size_t GetEntriesCount() const;
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> nit: comment description for method?

This isn't a new method. I've just moved it down to improve grouping.
I've added a description.

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/model_neu...
File sync/syncable/model_neutral_mutable_entry.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/model_neu...
sync/syncable/model_neutral_mutable_entry.cc:56: 
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> CHECK that type isn't BOOKMARKS or NIGORI?

Done.

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/syncable_...
File sync/syncable/syncable_util.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/syncable/syncable_...
sync/syncable/syncable_util.cc:35: return true;
On 2015/01/26 23:21:53, Nicolas Zea wrote:
> Should this be false here for bookmarks?

Done.

[maniscalco, 2015-01-29 00:36:34]

https://codereview.chromium.org/867793003/diff/80001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/867793003/diff/80001/sync/syncable/directory....
sync/syncable/directory.cc:929: return
kernel_->persisted_info.HasEmptyDownloadProgress(type);
On 2015/01/29 00:27:37, stanisc wrote:
> On 2015/01/26 19:17:34, maniscalco wrote:
> > On 2015/01/26 19:02:09, stanisc wrote:
> > > On 2015/01/24 00:55:59, maniscalco wrote:
> > > > Normally, when Directory methods access kernel_ they either take a
> > ScopedLock
> > > > (like GetEntriesCount) or they assert the thread already has a lock
(like
> > > > GetTransactionVersion).  Should this new method follow one of those
> > > approaches?
> > > 
> > > Done.
> > 
> > Since Directory::HasEmptyDownloadProgress is called from
> PurgeEntriesWithTypeIn
> > while a ScopedKernelLock on "this" is held, I'm thinking
> > Directory::HasEmptyDownloadProgress shouldn't take a lock because SKLs are
> > non-recursive, right?  As is, I would think the code might DCHECK:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...
> > 
> > Unless, maybe I'm missing something?
> > 
> > Thinking more about this... WDYT about adding const ScopedKernelLock& to
> > Directory::HasEmptyDownloadProgress ?  We do that elsewhere in Directory to
> > ensure the caller is holding a lock (e.g. ClearDirtyMetahandles).  That way,
> > it's harder to misuse.
> 
> Directory::HasEmptyDownloadProgress isn't called from PurgeEntriesWithTypeIn
> anymore. I changed that code back to calling
> Directory::PersistedKernelInfo::HasEmptyDownloadProgress directly to avoid
> exactly the problem you described. I did it at the same time when I added the
> lock to Directory::HasEmptyDownloadProgress. In general none of functions in
> PersistedKernelInfo get ScopedKernelLock& as one of parameters. I didn't want
to
> change the existing pattern so I decided it would be better to inline the call
> in PurgeEntriesWithTypeIn instead.

Ah, you're right.  I misread which HasEmptyDownloadProgress was being called
(Directory's vs Directory::PersistedKernelInfo's).

[Nicolas Zea, 2015-01-30 00:07:21]

LGTM with the nit mentioned (not sure if you still wanted to add a comment
there)

https://codereview.chromium.org/867793003/diff/100001/sync/engine/directory_u...
File sync/engine/directory_update_handler.cc (right):

https://codereview.chromium.org/867793003/diff/100001/sync/engine/directory_u...
sync/engine/directory_update_handler.cc:95: DVLOG(1) << "Type root folder "
On 2015/01/28 07:30:05, stanisc wrote:
> On 2015/01/26 23:21:53, Nicolas Zea wrote:
> > Comment that this is expected for older versions of the sync server?
> 
> Actually for an older version of the server this would be called before the
same
> folder is created by the server. The only scenario where I think we might hit
> this is the case where the progress marker gets reset on the client. I think
> there is one codepath that does that. I'll look into that and add an
appropriate
> comment.

Was there a comment you meant to put here?

[stanisc, 2015-01-30 20:15:03]

The CQ bit was checked by stanisc@chromium.org

[commit-bot: I haz the power, 2015-01-30 20:16:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/867793003/160001

[commit-bot: I haz the power, 2015-01-30 20:21:09]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2015-01-30 20:22:05]

Patchset 5 (id:??) landed as
https://crrev.com/8d4046a92a3314bf5e1b6f1dd6a3c4aed5449e64
Cr-Commit-Position: refs/heads/master@{#313977}