[Sync] Switch bots to run integ tests for autofill as USS.

[Sync] Switch bots to run integ tests for autofill as USS.

ModelTypeControllers are responsible for posting to the model thread.
They also typically grab the bridge or the processor through the bridge
when doing this. UI thread based controllers can simply go through
their SyncClient to do this, and all is well. However, for autofill,
and any WebDataService backed type, they cannot, they can only grab the
weak ref to their bridge on their model thread.

Initially, we were simply posting a task to the model thread that
contained a SyncClient pointer. This turns out to not be safe. During
shutdown, autofill would consistently crash here by posting a task to
the model thread, and before that gets resolved, the SyncClient is
destroyed. Presumably all the other posts to model thread were unsafe
as well, though hitting the race condition was more unlikely.

To get around this we now post a task with a ref counted
WebDataService. This required an ability to override the default
handling of the ModelTypeController, and any WebDataService backed
controller should use the new class created here.

While I was in there, some slight refactoring to ModelTypeController
and ModelTypeDebugInfo to make the other code changes as clean as
possible.

BUG=686172
Review-Url: https://codereview.chromium.org/2672493002
Cr-Commit-Position: refs/heads/master@{#450736}
Committed: https://chromium.googlesource.com/chromium/src/+/6261bc1e8181556af9c67ad860cca8ebfc3469e0

[skym, 2017-02-01 17:13:06]

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

[commit-bot: I haz the power, 2017-02-01 17:13:18]

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-02-01 17:50:12]

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

[commit-bot: I haz the power, 2017-02-01 17:50:13]

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

[skym, 2017-02-01 18:45:09]

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

[commit-bot: I haz the power, 2017-02-01 18:45:50]

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-02-01 19:17:26]

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

[commit-bot: I haz the power, 2017-02-01 19:17:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[skym, 2017-02-10 17:47:53]

Patchset #1 (id:1) has been deleted

[skym, 2017-02-10 17:48:04]

Patchset #1 (id:20001) has been deleted

[skym, 2017-02-10 17:48:19]

Description was changed from

==========
[Sync] Update integration tests to run AUTOFILL as USS.

BUG=686172
==========

to

==========
[Sync] Switch bots to run integ tests for autofill as USS.

BUG=686172
==========

[skym, 2017-02-10 17:48:36]

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

[commit-bot: I haz the power, 2017-02-10 17:49:24]

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

[skym, 2017-02-10 17:53:31]

Description was changed from

==========
[Sync] Switch bots to run integ tests for autofill as USS.

BUG=686172
==========

to

==========
[Sync] Switch bots to run integ tests for autofill as USS.

ModelTypeControllers are responsible for posting to the model thread.
They also typically grab the bridge or the processor through the bridge
when doing this. UI thread based controllers can simply go through
their SyncClient to do this, and all is well. However, for autofill,
and any WebDataService backed type, they cannot, they can only grab the
weak ref to their bridge on their model thread.

Initially, we were simply posting a task to the model thread that
contained a SyncClient pointer. This turns out to not be safe. During
shutdown, autofill could crash here by posting a task to the model
thread, and before that gets resolved, the SyncClient is destroyed.

To get around this we now post a task with a ref counted
WebDataService. This required an ability to override the default
handling of the ModelTypeController, and any WebDataService backed
controller should use the new class created here.

While I was in there, some slight refactoring to ModelTypeController
and ModelTypeDebugInfo to make the other code changes as clean as
possible.

BUG=686172
==========

[skym, 2017-02-10 17:53:31]

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

[skym, 2017-02-10 17:54:53]

Description was changed from

==========
[Sync] Switch bots to run integ tests for autofill as USS.

ModelTypeControllers are responsible for posting to the model thread.
They also typically grab the bridge or the processor through the bridge
when doing this. UI thread based controllers can simply go through
their SyncClient to do this, and all is well. However, for autofill,
and any WebDataService backed type, they cannot, they can only grab the
weak ref to their bridge on their model thread.

Initially, we were simply posting a task to the model thread that
contained a SyncClient pointer. This turns out to not be safe. During
shutdown, autofill could crash here by posting a task to the model
thread, and before that gets resolved, the SyncClient is destroyed.

To get around this we now post a task with a ref counted
WebDataService. This required an ability to override the default
handling of the ModelTypeController, and any WebDataService backed
controller should use the new class created here.

While I was in there, some slight refactoring to ModelTypeController
and ModelTypeDebugInfo to make the other code changes as clean as
possible.

BUG=686172
==========

to

==========
[Sync] Switch bots to run integ tests for autofill as USS.

ModelTypeControllers are responsible for posting to the model thread.
They also typically grab the bridge or the processor through the bridge
when doing this. UI thread based controllers can simply go through
their SyncClient to do this, and all is well. However, for autofill,
and any WebDataService backed type, they cannot, they can only grab the
weak ref to their bridge on their model thread.

Initially, we were simply posting a task to the model thread that
contained a SyncClient pointer. This turns out to not be safe. During
shutdown, autofill would consistently crash here by posting a task to
the model thread, and before that gets resolved, the SyncClient is
destroyed. Presumably all the other posts to model thread were unsafe
as well, though hitting the race condition was more unlikely.

To get around this we now post a task with a ref counted
WebDataService. This required an ability to override the default
handling of the ModelTypeController, and any WebDataService backed
controller should use the new class created here.

While I was in there, some slight refactoring to ModelTypeController
and ModelTypeDebugInfo to make the other code changes as clean as
possible.

BUG=686172
==========

[skym, 2017-02-10 17:59:58]

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

[commit-bot: I haz the power, 2017-02-10 18:00:31]

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

[skym, 2017-02-10 18:46:42]

Looks like all the tests are passing, PTAL.

[commit-bot: I haz the power, 2017-02-10 20:09:57]

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

[commit-bot: I haz the power, 2017-02-10 20:10:00]

Dry run: This issue passed the CQ dry run.

[skym, 2017-02-13 21:12:45]

ping

[pavely, 2017-02-14 01:36:50]

lgtm

[Gang Wu, 2017-02-14 17:59:44]

lgtm

https://codereview.chromium.org/2672493002/diff/60001/components/sync/driver/...
File components/sync/driver/model_type_controller.cc (right):

https://codereview.chromium.org/2672493002/diff/60001/components/sync/driver/...
components/sync/driver/model_type_controller.cc:48:
base::WeakPtr<ModelTypeSyncBridge> ReturnCapturedBridge(
can you add a comment about why you add this dummy function?

[skym, 2017-02-14 18:24:33]

https://codereview.chromium.org/2672493002/diff/60001/components/sync/driver/...
File components/sync/driver/model_type_controller.cc (right):

https://codereview.chromium.org/2672493002/diff/60001/components/sync/driver/...
components/sync/driver/model_type_controller.cc:48:
base::WeakPtr<ModelTypeSyncBridge> ReturnCapturedBridge(
On 2017/02/14 17:59:44, Gang Wu wrote:
> can you add a comment about why you add this dummy function?

Done.

[skym, 2017-02-14 18:24:38]

The CQ bit was checked by skym@chromium.org

[skym, 2017-02-14 18:24:38]

The patchset sent to the CQ was uploaded after l-g-t-m from gangwu@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2672493002/#ps80001
(title: "Updated for Gang's comment.")

[commit-bot: I haz the power, 2017-02-14 18:25:07]

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

[skym, 2017-02-14 18:30:43]

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

[skym, 2017-02-14 18:30:44]

Adding mathp@ for components/autofill/ PTAL

[commit-bot: I haz the power, 2017-02-14 18:36:52]

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

[commit-bot: I haz the power, 2017-02-14 18:36:53]

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...)

[Mathieu, 2017-02-14 18:59:30]

autofill lgtm

[skym, 2017-02-14 18:59:55]

The CQ bit was checked by skym@chromium.org

[commit-bot: I haz the power, 2017-02-14 19:00:44]

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-02-14 19:07:32]

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

[commit-bot: I haz the power, 2017-02-14 19:07:33]

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...)

[skym, 2017-02-14 19:08:49]

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

[skym, 2017-02-14 19:08:49]

Adding rkaplow@ for testing/variations/ PTAL

[rkaplow, 2017-02-15 15:24:35]

lgtm

field trial test config lg

[skym, 2017-02-15 16:43:37]

The CQ bit was checked by skym@chromium.org

[commit-bot: I haz the power, 2017-02-15 16:44:08]

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-02-15 17:27:02]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1487177017632580,
"parent_rev": "2810baeea98df657f5fa02572a5e59c680cc6210", "commit_rev":
"6261bc1e8181556af9c67ad860cca8ebfc3469e0"}

[commit-bot: I haz the power, 2017-02-15 17:27:40]

Description was changed from

==========
[Sync] Switch bots to run integ tests for autofill as USS.

ModelTypeControllers are responsible for posting to the model thread.
They also typically grab the bridge or the processor through the bridge
when doing this. UI thread based controllers can simply go through
their SyncClient to do this, and all is well. However, for autofill,
and any WebDataService backed type, they cannot, they can only grab the
weak ref to their bridge on their model thread.

Initially, we were simply posting a task to the model thread that
contained a SyncClient pointer. This turns out to not be safe. During
shutdown, autofill would consistently crash here by posting a task to
the model thread, and before that gets resolved, the SyncClient is
destroyed. Presumably all the other posts to model thread were unsafe
as well, though hitting the race condition was more unlikely.

To get around this we now post a task with a ref counted
WebDataService. This required an ability to override the default
handling of the ModelTypeController, and any WebDataService backed
controller should use the new class created here.

While I was in there, some slight refactoring to ModelTypeController
and ModelTypeDebugInfo to make the other code changes as clean as
possible.

BUG=686172
==========

to

==========
[Sync] Switch bots to run integ tests for autofill as USS.

ModelTypeControllers are responsible for posting to the model thread.
They also typically grab the bridge or the processor through the bridge
when doing this. UI thread based controllers can simply go through
their SyncClient to do this, and all is well. However, for autofill,
and any WebDataService backed type, they cannot, they can only grab the
weak ref to their bridge on their model thread.

Initially, we were simply posting a task to the model thread that
contained a SyncClient pointer. This turns out to not be safe. During
shutdown, autofill would consistently crash here by posting a task to
the model thread, and before that gets resolved, the SyncClient is
destroyed. Presumably all the other posts to model thread were unsafe
as well, though hitting the race condition was more unlikely.

To get around this we now post a task with a ref counted
WebDataService. This required an ability to override the default
handling of the ModelTypeController, and any WebDataService backed
controller should use the new class created here.

While I was in there, some slight refactoring to ModelTypeController
and ModelTypeDebugInfo to make the other code changes as clean as
possible.

BUG=686172

Review-Url: https://codereview.chromium.org/2672493002
Cr-Commit-Position: refs/heads/master@{#450736}
Committed:
https://chromium.googlesource.com/chromium/src/+/6261bc1e8181556af9c67ad860cc...
==========

[commit-bot: I haz the power, 2017-02-15 17:27:41]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/6261bc1e8181556af9c67ad860cc...