[Sync] Stop deleting LevelDB files when deleting Directory

[Sync] Stop deleting LevelDB files when deleting Directory

The LevelDB folder backing ModelTypeStore is inside Sync Data folder.
While this change is being driven by race conditions from sync trying
to delete this folder while it is being used, this setup is fundamental
flawed. The ModelTypeStores will hold model type specific information
that needs to be persistent across sign outs and disabling sync.

The approach this CL takes to fix this problem is the modify the
deletion logic to only affect the Directory (sqllite3) files. This
means that the Sync Data folder will still exist even when sync is off.

BUG=673508, 673887
Committed: https://crrev.com/4234d3c65dc2b0ad0cf19fbf5e9e1e528bc3ddaf
Cr-Commit-Position: refs/heads/master@{#439835}

[skym, 2016-12-12 22:37:10]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 22:38:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2016-12-12 22:58:11]

Description was changed from

==========
Move DeviceInfo service initialization after engine initialization.

BUG=673508
==========

to

==========
Move DeviceInfo service initialization after engine initialization.

DeviceInfoSyncService and DeviceInfoSyncBridge were previously
initialized in ProfileSyncService::Initialize(). This was quite early,
and they were essentially lame ducks until their
LocalDeviceInfoProvider initialized. It was nice that we could create
them, and not return nullptr when asked for the service/bridge, but
they were ultimately quite pointless.

The problem we ran into with USS and DeviceInfoSyncBridge, is that by
calling the StoreFactoryFunction callback, it launched the task to
asynchronously create the LevelDB that would store all of its data in
its constructor. This was before the engine itself initialized. This
LevelDB lived inside of the "Sync Data" folder, the ProfileSyncService
created it as such. Unfortunately, this happens before the engine
initializes, and if sync has not performed a first sync before, it
clears out the sync directory folder. This introduced a race condition
where we're creating files and deleting files in the same place.

The approach this CL takes is to delay the creation of the DeviceInfo
implementations until after the ProfileSyncService received the
callback that the engine has initialized. This should mean it's safe to
make the LevelDB. This also signficantly delays the creation of the
DeviceInfo implementations, but the working theory is that this is
perfectly okay. They were unable to really act, as I mentioned above.
Now they are created right after the provider is ready, removing this
uninitialized period. Additionally, model types do not start
interacting with sync until the DataTypeManager creates all the
DataTypeControllers, and the new location for the creation of the
DeviceInfo implementations is right before the creation of the
DataTypeManager, so this should be safe.

BUG=673508
==========

[skym, 2016-12-12 22:58:11]

skym@chromium.org changed reviewers:
+ maxbogue@chromium.org

[skym, 2016-12-12 23:20:18]

Description was changed from

==========
Move DeviceInfo service initialization after engine initialization.

DeviceInfoSyncService and DeviceInfoSyncBridge were previously
initialized in ProfileSyncService::Initialize(). This was quite early,
and they were essentially lame ducks until their
LocalDeviceInfoProvider initialized. It was nice that we could create
them, and not return nullptr when asked for the service/bridge, but
they were ultimately quite pointless.

The problem we ran into with USS and DeviceInfoSyncBridge, is that by
calling the StoreFactoryFunction callback, it launched the task to
asynchronously create the LevelDB that would store all of its data in
its constructor. This was before the engine itself initialized. This
LevelDB lived inside of the "Sync Data" folder, the ProfileSyncService
created it as such. Unfortunately, this happens before the engine
initializes, and if sync has not performed a first sync before, it
clears out the sync directory folder. This introduced a race condition
where we're creating files and deleting files in the same place.

The approach this CL takes is to delay the creation of the DeviceInfo
implementations until after the ProfileSyncService received the
callback that the engine has initialized. This should mean it's safe to
make the LevelDB. This also signficantly delays the creation of the
DeviceInfo implementations, but the working theory is that this is
perfectly okay. They were unable to really act, as I mentioned above.
Now they are created right after the provider is ready, removing this
uninitialized period. Additionally, model types do not start
interacting with sync until the DataTypeManager creates all the
DataTypeControllers, and the new location for the creation of the
DeviceInfo implementations is right before the creation of the
DataTypeManager, so this should be safe.

BUG=673508
==========

to

==========
[Sync] Move DeviceInfo service initialization after engine initialization.

DeviceInfoSyncService and DeviceInfoSyncBridge were previously
initialized in ProfileSyncService::Initialize(). This was quite early,
and they were essentially lame ducks until their
LocalDeviceInfoProvider initialized. It was nice that we could create
them, and not return nullptr when asked for the service/bridge, but
they were ultimately quite pointless.

The problem we ran into with USS and DeviceInfoSyncBridge, is that by
calling the StoreFactoryFunction callback, it launched the task to
asynchronously create the LevelDB that would store all of its data in
its constructor. This was before the engine itself initialized. This
LevelDB lived inside of the "Sync Data" folder, the ProfileSyncService
created it as such. Unfortunately, this happens before the engine
initializes, and if sync has not performed a first sync before, it
clears out the sync directory folder. This introduced a race condition
where we're creating files and deleting files in the same place.

The approach this CL takes is to delay the creation of the DeviceInfo
implementations until after the ProfileSyncService received the
callback that the engine has initialized. This should mean it's safe to
make the LevelDB. This also signficantly delays the creation of the
DeviceInfo implementations, but the working theory is that this is
perfectly okay. They were unable to really act, as I mentioned above.
Now they are created right after the provider is ready, removing this
uninitialized period. Additionally, model types do not start
interacting with sync until the DataTypeManager creates all the
DataTypeControllers, and the new location for the creation of the
DeviceInfo implementations is right before the creation of the
DataTypeManager, so this should be safe.

BUG=673508
==========

[skym, 2016-12-13 00:01:57]

skym@chromium.org changed reviewers:
+ pavely@chromium.org

[skym, 2016-12-13 00:01:58]

PTAL

[commit-bot: I haz the power, 2016-12-13 02:39:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 02:39:45]

Dry run: This issue passed the CQ dry run.

[skym, 2016-12-13 16:11:17]

On 2016/12/13 00:01:58, skym wrote:
> PTAL

Looks like I jumped the gun on posting this change, still broken on windows.

[skym, 2016-12-13 18:06:46]

Description was changed from

==========
[Sync] Move DeviceInfo service initialization after engine initialization.

DeviceInfoSyncService and DeviceInfoSyncBridge were previously
initialized in ProfileSyncService::Initialize(). This was quite early,
and they were essentially lame ducks until their
LocalDeviceInfoProvider initialized. It was nice that we could create
them, and not return nullptr when asked for the service/bridge, but
they were ultimately quite pointless.

The problem we ran into with USS and DeviceInfoSyncBridge, is that by
calling the StoreFactoryFunction callback, it launched the task to
asynchronously create the LevelDB that would store all of its data in
its constructor. This was before the engine itself initialized. This
LevelDB lived inside of the "Sync Data" folder, the ProfileSyncService
created it as such. Unfortunately, this happens before the engine
initializes, and if sync has not performed a first sync before, it
clears out the sync directory folder. This introduced a race condition
where we're creating files and deleting files in the same place.

The approach this CL takes is to delay the creation of the DeviceInfo
implementations until after the ProfileSyncService received the
callback that the engine has initialized. This should mean it's safe to
make the LevelDB. This also signficantly delays the creation of the
DeviceInfo implementations, but the working theory is that this is
perfectly okay. They were unable to really act, as I mentioned above.
Now they are created right after the provider is ready, removing this
uninitialized period. Additionally, model types do not start
interacting with sync until the DataTypeManager creates all the
DataTypeControllers, and the new location for the creation of the
DeviceInfo implementations is right before the creation of the
DataTypeManager, so this should be safe.

BUG=673508
==========

to

==========
[Sync] TODO

BUG=673508,664920
==========

[skym, 2016-12-13 18:06:50]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 18:07:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2016-12-13 18:12:15]

Description was changed from

==========
[Sync] TODO

BUG=673508,664920
==========

to

==========
[Sync] Move LevelDB used for ModelTypeStore outside Sync Data folder

DO NOT SUBMIT. The name of the moved LevelDB folder needs to be agreed
upon first. The current name is a placeholder only.

Previously the LevelDB folder backing ModelTypeStore was inside the
Sync Data folder. While this change is being driven by race conditions
from sync trying to delete this folder while it is being used, this
setup is fundamental flawed. The ModelTypeStores will hold model type
specific information that needs to be persistent across sign outs and
disabling sync. This is not the case with the Sync Data folder, sync
drives full deletions quite frequently.

While this will make things easier in the short term, it is going to
put a burden on the model types. They will need to fully manager their
prefix scoped section of the LevelDB and make appropriate choices about
clearing out data.

BUG=673508,664920
==========

[commit-bot: I haz the power, 2016-12-13 18:16:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 18:16:09]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pavely, 2016-12-13 19:40:52]

lgtm after name change.
This change doesn't address 664920 though.

[skym, 2016-12-13 21:56:37]

Description was changed from

==========
[Sync] Move LevelDB used for ModelTypeStore outside Sync Data folder

DO NOT SUBMIT. The name of the moved LevelDB folder needs to be agreed
upon first. The current name is a placeholder only.

Previously the LevelDB folder backing ModelTypeStore was inside the
Sync Data folder. While this change is being driven by race conditions
from sync trying to delete this folder while it is being used, this
setup is fundamental flawed. The ModelTypeStores will hold model type
specific information that needs to be persistent across sign outs and
disabling sync. This is not the case with the Sync Data folder, sync
drives full deletions quite frequently.

While this will make things easier in the short term, it is going to
put a burden on the model types. They will need to fully manager their
prefix scoped section of the LevelDB and make appropriate choices about
clearing out data.

BUG=673508,664920
==========

to

==========
[Sync] Stop deleting LevelDB files when deleting Directory

The LevelDB folder backing ModelTypeStore is inside Sync Data folder.
While this change is being driven by race conditions from sync trying
to delete this folder while it is being used, this setup is fundamental
flawed. The ModelTypeStores will hold model type specific information
that needs to be persistent across sign outs and disabling sync.

The approach this CL takes to fix this problem is the modify the
deletion logic to only affect the Directory (sqllite3) files. This
means that the Sync Data folder will still exist even when sync is off.

BUG=673508,673887
==========

[skym, 2016-12-16 18:48:54]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 18:49:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 19:59:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 19:59:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[skym, 2016-12-19 21:09:30]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[skym, 2016-12-19 21:09:46]

Hopefully this attempt will pass the bots.

[commit-bot: I haz the power, 2016-12-19 21:10:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 22:06:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 22:06:08]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-12-20 00:54:06]

lgtm, though I can take another look once you fix the tests.

https://codereview.chromium.org/2568743004/diff/60001/components/sync/syncabl...
File components/sync/syncable/directory.cc (right):

https://codereview.chromium.org/2568743004/diff/60001/components/sync/syncabl...
components/sync/syncable/directory.cc:440: // use no folders. We also assume
that there might be chuild folders under
child

https://codereview.chromium.org/2568743004/diff/60001/components/sync/syncabl...
components/sync/syncable/directory.cc:448: LOG(DFATAL) << "Could not delete the
Sync Data folder.";
Maybe "Could not delete all sync directory files." would be more accurate now?

[skym, 2016-12-20 16:11:29]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 16:11:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2016-12-20 16:12:46]

https://codereview.chromium.org/2568743004/diff/60001/components/sync/syncabl...
File components/sync/syncable/directory.cc (right):

https://codereview.chromium.org/2568743004/diff/60001/components/sync/syncabl...
components/sync/syncable/directory.cc:440: // use no folders. We also assume
that there might be chuild folders under
On 2016/12/20 00:54:06, maxbogue wrote:
> child

Done.

https://codereview.chromium.org/2568743004/diff/60001/components/sync/syncabl...
components/sync/syncable/directory.cc:448: LOG(DFATAL) << "Could not delete the
Sync Data folder.";
On 2016/12/20 00:54:06, maxbogue wrote:
> Maybe "Could not delete all sync directory files." would be more accurate now?

Done.

[skym, 2016-12-20 16:14:28]

The CQ bit was checked by skym@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 16:14:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2016-12-20 16:24:15]

Apparently I just didn't refresh and see PS#5 when I reviewed yesterday, oops.
PS#6 lgtm!

[commit-bot: I haz the power, 2016-12-20 17:02:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 17:02:39]

Dry run: This issue passed the CQ dry run.

[skym, 2016-12-20 17:24:18]

The CQ bit was checked by skym@chromium.org

[skym, 2016-12-20 17:24:19]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2568743004/#ps100001
(title: "Updated for Max's comments.")

[commit-bot: I haz the power, 2016-12-20 17:24:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 17:29:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1482254658711200,
"parent_rev": "a66d6f1bc40a1863ee66ee0fb1502fb953c17d2b", "commit_rev":
"bb3a8f88f9b552889c2ab7ef6a1ba6a73c3be12f"}

[commit-bot: I haz the power, 2016-12-20 17:30:12]

Description was changed from

==========
[Sync] Stop deleting LevelDB files when deleting Directory

The LevelDB folder backing ModelTypeStore is inside Sync Data folder.
While this change is being driven by race conditions from sync trying
to delete this folder while it is being used, this setup is fundamental
flawed. The ModelTypeStores will hold model type specific information
that needs to be persistent across sign outs and disabling sync.

The approach this CL takes to fix this problem is the modify the
deletion logic to only affect the Directory (sqllite3) files. This
means that the Sync Data folder will still exist even when sync is off.

BUG=673508,673887
==========

to

==========
[Sync] Stop deleting LevelDB files when deleting Directory

The LevelDB folder backing ModelTypeStore is inside Sync Data folder.
While this change is being driven by race conditions from sync trying
to delete this folder while it is being used, this setup is fundamental
flawed. The ModelTypeStores will hold model type specific information
that needs to be persistent across sign outs and disabling sync.

The approach this CL takes to fix this problem is the modify the
deletion logic to only affect the Directory (sqllite3) files. This
means that the Sync Data folder will still exist even when sync is off.

BUG=673508,673887

Review-Url: https://codereview.chromium.org/2568743004
==========

[commit-bot: I haz the power, 2016-12-20 17:30:14]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-20 17:32:14]

Description was changed from

==========
[Sync] Stop deleting LevelDB files when deleting Directory

The LevelDB folder backing ModelTypeStore is inside Sync Data folder.
While this change is being driven by race conditions from sync trying
to delete this folder while it is being used, this setup is fundamental
flawed. The ModelTypeStores will hold model type specific information
that needs to be persistent across sign outs and disabling sync.

The approach this CL takes to fix this problem is the modify the
deletion logic to only affect the Directory (sqllite3) files. This
means that the Sync Data folder will still exist even when sync is off.

BUG=673508,673887

Review-Url: https://codereview.chromium.org/2568743004
==========

to

==========
[Sync] Stop deleting LevelDB files when deleting Directory

The LevelDB folder backing ModelTypeStore is inside Sync Data folder.
While this change is being driven by race conditions from sync trying
to delete this folder while it is being used, this setup is fundamental
flawed. The ModelTypeStores will hold model type specific information
that needs to be persistent across sign outs and disabling sync.

The approach this CL takes to fix this problem is the modify the
deletion logic to only affect the Directory (sqllite3) files. This
means that the Sync Data folder will still exist even when sync is off.

BUG=673508,673887

Committed: https://crrev.com/4234d3c65dc2b0ad0cf19fbf5e9e1e528bc3ddaf
Cr-Commit-Position: refs/heads/master@{#439835}
==========

[commit-bot: I haz the power, 2016-12-20 17:32:15]

Patchset 6 (id:??) landed as
https://crrev.com/4234d3c65dc2b0ad0cf19fbf5e9e1e528bc3ddaf
Cr-Commit-Position: refs/heads/master@{#439835}