[Sync] Restrict types to configure to only include registered types

[Sync] Restrict types to configure to only include registered types

The issue is that the set of datatypes requested for configuration doesn't
match the set of update handlers registered with ModelTypeRegistry. This causes
segfault. The reason for it is that the types to configure are decided at the
time of configuration call in DataTypeManagerImpl on UI thread, while update
handlers are settled at the time of sync cycle execution (couple of posts on
sync thread later). Some update handlers can get unregistered by this time (the
bug has a description of scenario that can lead to this issue). This issue is
only applicable to configuration sync cycle since during normal sync cycle set
of datatypes is derived from ModelTypeRegistry.

The fix is to remove unregistered types from types to configure in
Syncer::ConfigureSyncShare.

BUG=703253
R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2804743004
Cr-Commit-Position: refs/heads/master@{#462727}
Committed: https://chromium.googlesource.com/chromium/src/+/e71ec0e8a8645902a8d45a9f0fc9b46aaea6c264

[pavely, 2017-04-06 20:21:43]

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

[commit-bot: I haz the power, 2017-04-06 20:23:23]

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, 2017-04-06 21:19:26]

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

[commit-bot: I haz the power, 2017-04-06 21:19:26]

Dry run: This issue passed the CQ dry run.

[skym, 2017-04-06 22:27:45]

lgtm

Also would be nice to see a unit test.

https://codereview.chromium.org/2804743004/diff/1/components/sync/engine_impl...
File components/sync/engine_impl/syncer.cc (right):

https://codereview.chromium.org/2804743004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/syncer.cc:82: VLOG(1) << "Configuring types " <<
ModelTypeSetToString(request_types);
I know this isn't you, but this line is now sometimes incorrect.

https://codereview.chromium.org/2804743004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/syncer.cc:92:
request_types.RetainAll(cycle->context()->GetEnabledTypes());
What do you think of moving this to
https://cs.chromium.org/chromium/src/components/sync/engine_impl/sync_schedul...

[pavely, 2017-04-06 23:47:52]

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

[pavely, 2017-04-06 23:48:26]

https://codereview.chromium.org/2804743004/diff/1/components/sync/engine_impl...
File components/sync/engine_impl/syncer.cc (right):

https://codereview.chromium.org/2804743004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/syncer.cc:82: VLOG(1) << "Configuring types " <<
ModelTypeSetToString(request_types);
On 2017/04/06 22:27:45, skym wrote:
> I know this isn't you, but this line is now sometimes incorrect.

You are right. Moving RetainAll above this line.

https://codereview.chromium.org/2804743004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/syncer.cc:92:
request_types.RetainAll(cycle->context()->GetEnabledTypes());
On 2017/04/06 22:27:45, skym wrote:
> What do you think of moving this to
>
https://cs.chromium.org/chromium/src/components/sync/engine_impl/sync_schedul...

I thought about it when adding this check last time. I think these locations are
equivalent, but scheduler should be concerned with "when" cycle happens, pass
parameters through and let syncer decide "what" to do.

[commit-bot: I haz the power, 2017-04-06 23:48:26]

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, 2017-04-07 00:40:38]

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

[commit-bot: I haz the power, 2017-04-07 00:40:39]

Dry run: This issue passed the CQ dry run.

[pavely, 2017-04-07 00:45:38]

The CQ bit was checked by pavely@chromium.org

[pavely, 2017-04-07 00:45:38]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2804743004/#ps20001
(title: "Address comments, adjust unittest.")

[commit-bot: I haz the power, 2017-04-07 00:46:04]

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, 2017-04-07 01:14:43]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491525938251080,
"parent_rev": "dc59bccacdd6aedddc578d80025a78910916bc38", "commit_rev":
"e71ec0e8a8645902a8d45a9f0fc9b46aaea6c264"}

[commit-bot: I haz the power, 2017-04-07 01:15:51]

Description was changed from

==========
[Sync] Restrict types to configure to only include registered types

The issue is that the set of datatypes requested for configuration doesn't
match the set of update handlers registered with ModelTypeRegistry. This causes
segfault. The reason for it is that the types to configure are decided at the
time of configuration call in DataTypeManagerImpl on UI thread, while update
handlers are settled at the time of sync cycle execution (couple of posts on
sync thread later). Some update handlers can get unregistered by this time (the
bug has a description of scenario that can lead to this issue). This issue is
only applicable to configuration sync cycle since during normal sync cycle set
of datatypes is derived from ModelTypeRegistry.

The fix is to remove unregistered types from types to configure in
Syncer::ConfigureSyncShare.

BUG=703253
R=skym@chromium.org
==========

to

==========
[Sync] Restrict types to configure to only include registered types

The issue is that the set of datatypes requested for configuration doesn't
match the set of update handlers registered with ModelTypeRegistry. This causes
segfault. The reason for it is that the types to configure are decided at the
time of configuration call in DataTypeManagerImpl on UI thread, while update
handlers are settled at the time of sync cycle execution (couple of posts on
sync thread later). Some update handlers can get unregistered by this time (the
bug has a description of scenario that can lead to this issue). This issue is
only applicable to configuration sync cycle since during normal sync cycle set
of datatypes is derived from ModelTypeRegistry.

The fix is to remove unregistered types from types to configure in
Syncer::ConfigureSyncShare.

BUG=703253
R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2804743004
Cr-Commit-Position: refs/heads/master@{#462727}
Committed:
https://chromium.googlesource.com/chromium/src/+/e71ec0e8a8645902a8d45a9f0fc9...
==========

[commit-bot: I haz the power, 2017-04-07 01:15:52]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e71ec0e8a8645902a8d45a9f0fc9...