MD Settings: defer sending pref updates until all synchronous operations finish

MD Settings: defer sending pref updates until all synchronous operations finish

In the case of extension-controlled prefs, extensions like fillr use

  chrome.privacy.services.autofillEnabled.set({value: false});

To disable Chrome's native Autofill.

When this happens, code runs[1] in chrome to set a pref in the:

1) profile pref store
2) extensions pref store

We observe changes to the profile pref store.  So the order is:

1) set value in profile pref store
-> change happens
2) set value in extensions pref store

Unfortunately, we ask in the observer whether
pref->IsExtensionControlled()[2], which check which store the value comes from.

This will change on the next line of code, but isn't updated yet.

I considered just changing the order (which might be valid), but I think
asyncrifying potentially solves other types of observer order issues.

R=rdevlin.cronin@chromium.org
BUG=693226
[1] https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference/preference_api.cc?type=cs&q=passwordsavingenabled&l=448,446
[2] https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_private/prefs_util.cc?q=prefs_util.cc&sq=package:chromium&dr&l=486,751

Review-Url: https://codereview.chromium.org/2742663002
Cr-Commit-Position: refs/heads/master@{#456155}
Committed: https://chromium.googlesource.com/chromium/src/+/4a3253c3dc5edc041db7f64cca32476181f09013

[Dan Beam, 2017-03-09 05:08:28]

Description was changed from

==========
WIP: example post task for scottchen@

BUG=
==========

to

==========
MD Settings: defer sending pref updates until all synchronous operations finish

In the case of extension-controlled prefs, extensions like fillr use

  chrome.privacy.services.autofillEnabled.set({value: false});

To disable Chrome's native Autofill.

When this happens, code runs in chrome to set the:

1) value of pref in the profile store
2) value of the pref in the extensions pref store

If we're observing the profile store's pref, and it changes, and we ask
whether the pref->IsExtensionControlled(), which checks whether a
pref's effective value comes from extension store (which will be updated
in the next line but isn't yet), bad things happen.

https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...

That was fun to track down. Open to suggestions on other ways to fix.
I considered just changing the order (which might be valid), but I think
asyncrifying potentially solves other types of observer order issues.

R=rdevlin.cronin@chromium.org
BUG=693226

[1]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...
==========

[Dan Beam, 2017-03-09 05:08:28]

dbeam@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Dan Beam, 2017-03-09 05:09:43]

Description was changed from

==========
MD Settings: defer sending pref updates until all synchronous operations finish

In the case of extension-controlled prefs, extensions like fillr use

  chrome.privacy.services.autofillEnabled.set({value: false});

To disable Chrome's native Autofill.

When this happens, code runs in chrome to set the:

1) value of pref in the profile store
2) value of the pref in the extensions pref store

If we're observing the profile store's pref, and it changes, and we ask
whether the pref->IsExtensionControlled(), which checks whether a
pref's effective value comes from extension store (which will be updated
in the next line but isn't yet), bad things happen.

https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...

That was fun to track down. Open to suggestions on other ways to fix.
I considered just changing the order (which might be valid), but I think
asyncrifying potentially solves other types of observer order issues.

R=rdevlin.cronin@chromium.org
BUG=693226

[1]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...
==========

to

==========
MD Settings: defer sending pref updates until all synchronous operations finish

In the case of extension-controlled prefs, extensions like fillr use

  chrome.privacy.services.autofillEnabled.set({value: false});

To disable Chrome's native Autofill.

When this happens, code runs in chrome to set a pref in the:

1) profile pref store
2) extensions pref store

If we're observing the profile store's pref, and it changes, and we ask
whether the pref->IsExtensionControlled(), which checks whether a
pref's effective value comes from extension store (which will be updated
in the next line but isn't yet), bad things happen.

https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...

That was fun to track down. Open to suggestions on other ways to fix.
I considered just changing the order (which might be valid), but I think
asyncrifying potentially solves other types of observer order issues.

R=rdevlin.cronin@chromium.org
BUG=693226

[1]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...
==========

[Dan Beam, 2017-03-09 05:13:34]

Description was changed from

==========
MD Settings: defer sending pref updates until all synchronous operations finish

In the case of extension-controlled prefs, extensions like fillr use

  chrome.privacy.services.autofillEnabled.set({value: false});

To disable Chrome's native Autofill.

When this happens, code runs in chrome to set a pref in the:

1) profile pref store
2) extensions pref store

If we're observing the profile store's pref, and it changes, and we ask
whether the pref->IsExtensionControlled(), which checks whether a
pref's effective value comes from extension store (which will be updated
in the next line but isn't yet), bad things happen.

https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...

That was fun to track down. Open to suggestions on other ways to fix.
I considered just changing the order (which might be valid), but I think
asyncrifying potentially solves other types of observer order issues.

R=rdevlin.cronin@chromium.org
BUG=693226

[1]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...
==========

to

==========
MD Settings: defer sending pref updates until all synchronous operations finish

In the case of extension-controlled prefs, extensions like fillr use

  chrome.privacy.services.autofillEnabled.set({value: false});

To disable Chrome's native Autofill.

When this happens, code runs[1] in chrome to set a pref in the:

1) profile pref store
2) extensions pref store

We observe changes to the profile pref store.  So the order is:

1) set value in profile pref store
-> change happens
2) set value in extensions pref store

Unfortunately, we ask in the observer whether
pref->IsExtensionControlled()[2], which check which store the value comes from.

This will change on the next line of code, but isn't updated yet.

That was fun to track down. Open to suggestions on other ways to fix.
I considered just changing the order (which might be valid), but I think
asyncrifying potentially solves other types of observer order issues.

R=rdevlin.cronin@chromium.org
BUG=693226

[1]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...
[2]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p...
==========

[Dan Beam, 2017-03-09 05:14:10]

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

[commit-bot: I haz the power, 2017-03-09 05:14:29]

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-03-09 05:46:33]

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

[commit-bot: I haz the power, 2017-03-09 05:46:34]

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

[Dan Beam, 2017-03-09 19:06:53]

Patchset #3 (id:40001) has been deleted

[Dan Beam, 2017-03-09 19:07:00]

Patchset #2 (id:20001) has been deleted

[Dan Beam, 2017-03-09 19:07:04]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2017-03-10 04:13:21]

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

[commit-bot: I haz the power, 2017-03-10 04:13:38]

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-03-10 04:56:44]

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

[commit-bot: I haz the power, 2017-03-10 04:56:45]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-03-10 08:03:09]

thanks for you help today, pretty sure that worked!

[Devlin, 2017-03-10 20:01:25]

lgtm

nit: Take this line out of the description; probably doesn't need to be archived
in the src history. :)
> That was fun to track down. Open to suggestions on other ways to fix.

https://codereview.chromium.org/2742663002/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/api/settings_private/settings_private_event_router.cc
(right):

https://codereview.chromium.org/2742663002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_private/settings_private_event_router.cc:128:
// as |pref_utils_->GetPref()| relies on this information to determine if a
s/pref_utils/prefs_util

[Dan Beam, 2017-03-10 20:04:43]

Description was changed from

==========
MD Settings: defer sending pref updates until all synchronous operations finish

In the case of extension-controlled prefs, extensions like fillr use

  chrome.privacy.services.autofillEnabled.set({value: false});

To disable Chrome's native Autofill.

When this happens, code runs[1] in chrome to set a pref in the:

1) profile pref store
2) extensions pref store

We observe changes to the profile pref store.  So the order is:

1) set value in profile pref store
-> change happens
2) set value in extensions pref store

Unfortunately, we ask in the observer whether
pref->IsExtensionControlled()[2], which check which store the value comes from.

This will change on the next line of code, but isn't updated yet.

That was fun to track down. Open to suggestions on other ways to fix.
I considered just changing the order (which might be valid), but I think
asyncrifying potentially solves other types of observer order issues.

R=rdevlin.cronin@chromium.org
BUG=693226

[1]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...
[2]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p...
==========

to

==========
MD Settings: defer sending pref updates until all synchronous operations finish

In the case of extension-controlled prefs, extensions like fillr use

  chrome.privacy.services.autofillEnabled.set({value: false});

To disable Chrome's native Autofill.

When this happens, code runs[1] in chrome to set a pref in the:

1) profile pref store
2) extensions pref store

We observe changes to the profile pref store.  So the order is:

1) set value in profile pref store
-> change happens
2) set value in extensions pref store

Unfortunately, we ask in the observer whether
pref->IsExtensionControlled()[2], which check which store the value comes from.

This will change on the next line of code, but isn't updated yet.

I considered just changing the order (which might be valid), but I think
asyncrifying potentially solves other types of observer order issues.

R=rdevlin.cronin@chromium.org
BUG=693226

[1]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...
[2]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p...
==========

[Dan Beam, 2017-03-10 20:06:22]

https://codereview.chromium.org/2742663002/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/api/settings_private/settings_private_event_router.cc
(right):

https://codereview.chromium.org/2742663002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/settings_private/settings_private_event_router.cc:128:
// as |pref_utils_->GetPref()| relies on this information to determine if a
On 2017/03/10 20:01:24, Devlin wrote:
> s/pref_utils/prefs_util

Done.

[Dan Beam, 2017-03-10 20:07:50]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2017-03-10 20:07:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2742663002/#ps100001
(title: "damn 's'")

[commit-bot: I haz the power, 2017-03-10 20:08:25]

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-03-10 20:56:07]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489176470454100,
"parent_rev": "7fdea0d7d730de0d5a34f6f1fb5f868fa8e40ece", "commit_rev":
"4a3253c3dc5edc041db7f64cca32476181f09013"}

[commit-bot: I haz the power, 2017-03-10 20:56:50]

Description was changed from

==========
MD Settings: defer sending pref updates until all synchronous operations finish

In the case of extension-controlled prefs, extensions like fillr use

  chrome.privacy.services.autofillEnabled.set({value: false});

To disable Chrome's native Autofill.

When this happens, code runs[1] in chrome to set a pref in the:

1) profile pref store
2) extensions pref store

We observe changes to the profile pref store.  So the order is:

1) set value in profile pref store
-> change happens
2) set value in extensions pref store

Unfortunately, we ask in the observer whether
pref->IsExtensionControlled()[2], which check which store the value comes from.

This will change on the next line of code, but isn't updated yet.

I considered just changing the order (which might be valid), but I think
asyncrifying potentially solves other types of observer order issues.

R=rdevlin.cronin@chromium.org
BUG=693226

[1]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...
[2]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p...
==========

to

==========
MD Settings: defer sending pref updates until all synchronous operations finish

In the case of extension-controlled prefs, extensions like fillr use

  chrome.privacy.services.autofillEnabled.set({value: false});

To disable Chrome's native Autofill.

When this happens, code runs[1] in chrome to set a pref in the:

1) profile pref store
2) extensions pref store

We observe changes to the profile pref store.  So the order is:

1) set value in profile pref store
-> change happens
2) set value in extensions pref store

Unfortunately, we ask in the observer whether
pref->IsExtensionControlled()[2], which check which store the value comes from.

This will change on the next line of code, but isn't updated yet.

I considered just changing the order (which might be valid), but I think
asyncrifying potentially solves other types of observer order issues.

R=rdevlin.cronin@chromium.org
BUG=693226

[1]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/preference...
[2]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/settings_p...

Review-Url: https://codereview.chromium.org/2742663002
Cr-Commit-Position: refs/heads/master@{#456155}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a3253c3dc5edc041db7f64cca32...
==========

[commit-bot: I haz the power, 2017-03-10 20:56:51]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4a3253c3dc5edc041db7f64cca32...