|
|
Created:
3 years, 9 months ago by Sam McNally Modified:
3 years, 8 months ago CC:
Aaron Boodman, abarth-chromium, chrome-apps-syd-reviews_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, qsr+mojo_chromium.org, rginda+watch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPref service: enable for user prefs in chrome behind a flag.
This:
- changes ProfilePrefStoreManager to, when the PrefService feature is
enabled, connect to the pref service, send it the user prefs
configuration and create a PrefStore that is backed by the pref
service
- changes PrefHashFilter to discard its on_reset_on_load callback after
the point where it may call it; when running in the perf service, the
callback owns a mojo connection that can be torn down at that point
- changes BrowserProcessImpl to wait for a task to run on the IO thread
before waiting for blocking IO to complete for each profile; the pref
service runs on the IO thread so waiting for a task to run there is
necessary
- fixes a bug in PrefServiceSyncableFactory where enabling the pref
service would crash if any non-user pref stores are null
BUG=654988
Review-Url: https://codereview.chromium.org/2746023002
Cr-Commit-Position: refs/heads/master@{#460273}
Committed: https://chromium.googlesource.com/chromium/src/+/2a1451c0b54e230d212035525fc6b373abea7f5b
Patch Set 1 : On for testing #Patch Set 2 : #
Total comments: 8
Patch Set 3 : rebase #Patch Set 4 : #Patch Set 5 : rebase off https://codereview.chromium.org/2745563005/ #Patch Set 6 : rebase #Patch Set 7 : #Patch Set 8 : #
Total comments: 2
Patch Set 9 : rebase #Patch Set 10 : #Patch Set 11 : rebase #Patch Set 12 : no, the other pref service #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 113 (96 generated)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:100001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:120001) has been deleted
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Pref service: Enable in chrome behind a flag. BUG=654988 ========== to ========== Pref service: enable for user prefs in chrome behind a flag. This: - changes ProfilePrefStoreManager to, when the PrefService feature is enabled, connect to the pref service, send it the user prefs configuration and create a PrefStore that is backed by the pref service - changes PrefHashFilter to discard its on_reset_on_load callback after the point where it may call it; when running in the perf service, the callback owns a mojo connection that can be torn down at that point - changes BrowserProcessImpl to wait for a task to run on the IO thread before waiting for blocking IO to complete for each profile; the pref service runs on the IO thread so waiting for a task to run there is necessary - allows sync mojo IPCs on the browser main thread before the MessageLoop starts; user prefs need to be loaded synchronously before the MessageLoop starts - fixes a bug in PrefServiceSyncableFactory where enabling the pref service would crash if any non-user pref stores are null BUG=654988 ==========
Description was changed from ========== Pref service: enable for user prefs in chrome behind a flag. This: - changes ProfilePrefStoreManager to, when the PrefService feature is enabled, connect to the pref service, send it the user prefs configuration and create a PrefStore that is backed by the pref service - changes PrefHashFilter to discard its on_reset_on_load callback after the point where it may call it; when running in the perf service, the callback owns a mojo connection that can be torn down at that point - changes BrowserProcessImpl to wait for a task to run on the IO thread before waiting for blocking IO to complete for each profile; the pref service runs on the IO thread so waiting for a task to run there is necessary - allows sync mojo IPCs on the browser main thread before the MessageLoop starts; user prefs need to be loaded synchronously before the MessageLoop starts - fixes a bug in PrefServiceSyncableFactory where enabling the pref service would crash if any non-user pref stores are null BUG=654988 ========== to ========== Pref service: enable for user prefs in chrome behind a flag. This: - changes ProfilePrefStoreManager to, when the PrefService feature is enabled, connect to the pref service, send it the user prefs configuration and create a PrefStore that is backed by the pref service - changes PrefHashFilter to discard its on_reset_on_load callback after the point where it may call it; when running in the perf service, the callback owns a mojo connection that can be torn down at that point - changes BrowserProcessImpl to wait for a task to run on the IO thread before waiting for blocking IO to complete for each profile; the pref service runs on the IO thread so waiting for a task to run there is necessary - allows sync mojo IPCs on the browser main thread before the MessageLoop starts; user prefs need to be loaded synchronously before the MessageLoop starts - fixes a bug in PrefServiceSyncableFactory where enabling the pref service would crash if any non-user pref stores are null BUG=654988 ==========
sammc@chromium.org changed reviewers: + gab@chromium.org, mbarbella@chromium.org
sammc@chromium.org changed reviewers: + mlerman@chromium.org
sammc@chromium.org changed reviewers: - mbarbella@chromium.org, mlerman@chromium.org
Please review the prefs changes.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sammc@chromium.org changed reviewers: - gab@chromium.org
sammc@chromium.org changed reviewers: + bauerb@chromium.org, jam@chromium.org, mbarbella@chromium.org, mlerman@chromium.org, sky@chromium.org
+mlerman for //chrome/browser/profiles +sky for chrome/browser/browser_process_impl.cc +bauerb for //chrome/browser/prefs, //components/sync_preferences and //components/user_prefs/tracked +jam for //content and //mojo +mbarbella for chrome/browser/chrome_content_browser_manifest_overlay.json
https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:528: base::Time start = base::Time::Now(); Use timeticks? https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:529: if (rundown_counter->TimedWait(kEndSessionTimeout) && pref_service_enabled) { What does a return value of true mean for TimedWait (It doesn't document what it means, and should). https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:532: for (auto& profile_writer_runner : profile_writer_runners) { no {} https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:535: profile_write_rundown_counter->TimedWait(kEndSessionTimeout - Does RundownTaskCounter deal with potentially negative time deltas?
mlerman@chromium.org changed reviewers: + anthonyvd@chromium.org - mlerman@chromium.org
On 2017/03/15 15:19:17, sky wrote: > https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... > File chrome/browser/browser_process_impl.cc (right): > > https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... > chrome/browser/browser_process_impl.cc:528: base::Time start = > base::Time::Now(); > Use timeticks? > > https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... > chrome/browser/browser_process_impl.cc:529: if > (rundown_counter->TimedWait(kEndSessionTimeout) && pref_service_enabled) { > What does a return value of true mean for TimedWait (It doesn't document what it > means, and should). > > https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... > chrome/browser/browser_process_impl.cc:532: for (auto& profile_writer_runner : > profile_writer_runners) { > no {} > > https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... > chrome/browser/browser_process_impl.cc:535: > profile_write_rundown_counter->TimedWait(kEndSessionTimeout - > Does RundownTaskCounter deal with potentially negative time deltas? Defering to Anthony.
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #3 (id:180001) has been deleted
Patchset #3 (id:200001) has been deleted
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:528: base::Time start = base::Time::Now(); On 2017/03/15 15:19:17, sky wrote: > Use timeticks? Done. https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:529: if (rundown_counter->TimedWait(kEndSessionTimeout) && pref_service_enabled) { On 2017/03/15 15:19:17, sky wrote: > What does a return value of true mean for TimedWait (It doesn't document what it > means, and should). It returns whether everything finished before the timeout. Documented it and extracted a variable with a name that should make it clearer why the code here cares. https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:532: for (auto& profile_writer_runner : profile_writer_runners) { On 2017/03/15 15:19:17, sky wrote: > no {} Done. https://codereview.chromium.org/2746023002/diff/160001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:535: profile_write_rundown_counter->TimedWait(kEndSessionTimeout - On 2017/03/15 15:19:17, sky wrote: > Does RundownTaskCounter deal with potentially negative time deltas? I think it should, but I've changed it to take an end time instead to avoid the unnecessary subtraction here. End times in the past are also valid.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
prefs LGTM
json lgtm
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jam, anthonyvd, sky: ping
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Sorry for the delay, I missed this cl. In the future, please IM me if I don't respond quickly. Regarding the sync call, this mechanism of avoiding sync calls was added precisely for the browser process. So as written, this would allow any code to make sync calls to other processes or even threads which is something we want to avoid. Can you instead make the friend class just the prefs code which makes the call?
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/22 00:03:48, jam wrote: > Sorry for the delay, I missed this cl. In the future, please IM me if I don't > respond quickly. > > Regarding the sync call, this mechanism of avoiding sync calls was added > precisely for the browser process. So as written, this would allow any code to > make sync calls to other processes or even threads which is something we want to > avoid. > > Can you instead make the friend class just the prefs code which makes the call? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2746023002/diff/320001/services/preferences/p... File services/preferences/public/cpp/persistent_pref_store_client.cc (right): https://codereview.chromium.org/2746023002/diff/320001/services/preferences/p... services/preferences/public/cpp/persistent_pref_store_client.cc:75: base::ThreadRestrictions::AssertWaitAllowed(); nit: curious why you added this? mojo::SyncCallRestrictions already has a ScopedAllowWait member. I think no need to worry about both restrictions, just the mojo one.
LGTM
Sorry about that, c/b/profiles lgtm
Patchset #9 (id:340001) has been deleted
Patchset #9 (id:360001) has been deleted
Patchset #9 (id:380001) has been deleted
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Pref service: enable for user prefs in chrome behind a flag. This: - changes ProfilePrefStoreManager to, when the PrefService feature is enabled, connect to the pref service, send it the user prefs configuration and create a PrefStore that is backed by the pref service - changes PrefHashFilter to discard its on_reset_on_load callback after the point where it may call it; when running in the perf service, the callback owns a mojo connection that can be torn down at that point - changes BrowserProcessImpl to wait for a task to run on the IO thread before waiting for blocking IO to complete for each profile; the pref service runs on the IO thread so waiting for a task to run there is necessary - allows sync mojo IPCs on the browser main thread before the MessageLoop starts; user prefs need to be loaded synchronously before the MessageLoop starts - fixes a bug in PrefServiceSyncableFactory where enabling the pref service would crash if any non-user pref stores are null BUG=654988 ========== to ========== Pref service: enable for user prefs in chrome behind a flag. This: - changes ProfilePrefStoreManager to, when the PrefService feature is enabled, connect to the pref service, send it the user prefs configuration and create a PrefStore that is backed by the pref service - changes PrefHashFilter to discard its on_reset_on_load callback after the point where it may call it; when running in the perf service, the callback owns a mojo connection that can be torn down at that point - changes BrowserProcessImpl to wait for a task to run on the IO thread before waiting for blocking IO to complete for each profile; the pref service runs on the IO thread so waiting for a task to run there is necessary - fixes a bug in PrefServiceSyncableFactory where enabling the pref service would crash if any non-user pref stores are null BUG=654988 ==========
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.win
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2746023002/diff/320001/services/preferences/p... File services/preferences/public/cpp/persistent_pref_store_client.cc (right): https://codereview.chromium.org/2746023002/diff/320001/services/preferences/p... services/preferences/public/cpp/persistent_pref_store_client.cc:75: base::ThreadRestrictions::AssertWaitAllowed(); On 2017/03/22 14:58:12, jam wrote: > nit: curious why you added this? mojo::SyncCallRestrictions already has a > ScopedAllowWait member. I think no need to worry about both restrictions, just > the mojo one. It was intended to detect inadvertent post-startup sync pref creation. Removed it.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by sammc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sammc@chromium.org
The CQ bit was checked by sammc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mbarbella@chromium.org, jam@chromium.org, anthonyvd@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2746023002/#ps460001 (title: "no, the other pref service")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 460001, "attempt_start_ts": 1490759047602910, "parent_rev": "d93a2b102eb48ba6dbaa0ac11dff9edc4d1eec9b", "commit_rev": "2a1451c0b54e230d212035525fc6b373abea7f5b"}
Message was sent while issue was closed.
Description was changed from ========== Pref service: enable for user prefs in chrome behind a flag. This: - changes ProfilePrefStoreManager to, when the PrefService feature is enabled, connect to the pref service, send it the user prefs configuration and create a PrefStore that is backed by the pref service - changes PrefHashFilter to discard its on_reset_on_load callback after the point where it may call it; when running in the perf service, the callback owns a mojo connection that can be torn down at that point - changes BrowserProcessImpl to wait for a task to run on the IO thread before waiting for blocking IO to complete for each profile; the pref service runs on the IO thread so waiting for a task to run there is necessary - fixes a bug in PrefServiceSyncableFactory where enabling the pref service would crash if any non-user pref stores are null BUG=654988 ========== to ========== Pref service: enable for user prefs in chrome behind a flag. This: - changes ProfilePrefStoreManager to, when the PrefService feature is enabled, connect to the pref service, send it the user prefs configuration and create a PrefStore that is backed by the pref service - changes PrefHashFilter to discard its on_reset_on_load callback after the point where it may call it; when running in the perf service, the callback owns a mojo connection that can be torn down at that point - changes BrowserProcessImpl to wait for a task to run on the IO thread before waiting for blocking IO to complete for each profile; the pref service runs on the IO thread so waiting for a task to run there is necessary - fixes a bug in PrefServiceSyncableFactory where enabling the pref service would crash if any non-user pref stores are null BUG=654988 Review-Url: https://codereview.chromium.org/2746023002 Cr-Commit-Position: refs/heads/master@{#460273} Committed: https://chromium.googlesource.com/chromium/src/+/2a1451c0b54e230d212035525fc6... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:460001) as https://chromium.googlesource.com/chromium/src/+/2a1451c0b54e230d212035525fc6...
Message was sent while issue was closed.
On 2017/03/29 04:15:01, commit-bot: I haz the power wrote: > Committed patchset #12 (id:460001) as > https://chromium.googlesource.com/chromium/src/+/2a1451c0b54e230d212035525fc6... Hello, Could you help me confirm whether or not this CL could cause ProfilePrefStoreManagerTest/ProfilePrefStoreManagerTest.UnprotectedToProtected/0 to be flaky? For more information: https://findit-for-me.appspot.com/waterfall/check-flake?key=ag9zfmZpbmRpdC1mb... Thank you! |