Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(222)

Issue 273243002: Revert 269415 "Introduce a new framework for back-and-forth trac..." (Closed)

Created:
6 years, 7 months ago by ananta
Modified:
6 years, 7 months ago
Reviewers:
gab
CC:
chromium-reviews
Visibility:
Public.

Description

Revert 269415 "Introduce a new framework for back-and-forth trac..." Reverting because this appears to have caused Linux TSAN redness. > Introduce a new framework for back-and-forth tracked preference migration > between Protected Preferences and unprotected Preferences. > > Migration from unprotected Preferences to Protected Preferences was previously > done after both stores had been initialized. This was inherently incorrect as > some operations (PrefHashFilter::FilterOnLoad) would occur before the values > had been moved to the proper store. It also introduced a weird method in > PrefHashFilter::MigrateValues which required an independent PrefHashFilter > (backed by a copy of the real PrefHashStore). This after-the-fact migration > caused Settings.TrackedPreferenceCleared spikes when changing a value from > being enforced to not being enforced (as we'd have a MAC, but no value yet in > this store when running FilterOnLoad()) and more importantly it also caused > issue 365769 -- both of these issues highlight the incorrectness of the > current approach. > > The migration back from Protected Preferences to unprotected Preferences when > enforcement was disabled was using yet another mechanism which would only kick > in when a given pref was written to (ref. old non-const > SegregatedPrefStore::StoreForKey()). > > The new framework intercepts PrefFilter::FilterOnLoad() events for both stores > and does the back-and-forth migration in place before it even hands them back > to the PrefFilter::FinalizeFilterOnLoad() which then hands it back to the > JsonPrefStores (so that they are agnostic to the migration; from their point > of view their values were always in their store as they received it). > Furthermore, this new framework will easily allow us to later move MACs out of > Local State into their respective stores (which is a task on our radar which > we currently have no easy way to accomplish). > > The new framework also handles read errors better. For example, it was > previously possible for the unprotected->protected migration to result in data > loss if the protected store was somehow read-only from a read error while the > unprotected store wasn't -- resulting in an in-memory migration only flushed > to disk in the store from which the value was deleted... The new framework > handles those cases, preferring temporary data duplication over potential data > loss (duplicated data is cleaned up once confirmation is obtained that the new > authority for this data has been successfully written to disk -- it will even > try again in following Chrome runs if it doesn't succeed in this one). > > Note: This CL helped LSAN discover an existing leak in post_task_and_reply_impl.cc, see issue 371974 for details. > > BUG=365769, 371974 > TEST= > A) Make sure all kTrackedPrefs consistently report > Settings.TrackedPreferenceUnchanged across changes from various enforcement > levels (using --force-fieldtrials). > B) Make sure the prefs are properly migrated to their new store (and > subsequently cleaned up from their old store) when changing the > enforcement_level across multiple runs. > C) Make sure prefs are properly migrated in a quick startup/shutdown with a > new enforcement_level and that their old value is properly cleaned up in a > subsequent startup at the same enforcement_level (or re-migrated at another > enforcement_level). > > R=bauerb@chromium.org, robertshield@chromium.org, stuartmorgan@chromium.org, thakis@chromium.org > > Initially Committed in: https://src.chromium.org/viewvc/chrome?view=rev&revision=269346 > Reverted in: https://src.chromium.org/viewvc/chrome?view=rev&revision=269367 > > Review URL: https://codereview.chromium.org/257003007 TBR=gab@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269438

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+481 lines, -1609 lines) Patch
M trunk/src/base/files/important_file_writer.h View 4 chunks +0 lines, -15 lines 0 comments Download
M trunk/src/base/files/important_file_writer.cc View 4 chunks +10 lines, -27 lines 0 comments Download
M trunk/src/base/files/important_file_writer_unittest.cc View 4 chunks +0 lines, -73 lines 0 comments Download
M trunk/src/base/prefs/json_pref_store.h View 4 chunks +6 lines, -42 lines 0 comments Download
M trunk/src/base/prefs/json_pref_store.cc View 7 chunks +49 lines, -101 lines 0 comments Download
M trunk/src/base/prefs/json_pref_store_unittest.cc View 8 chunks +4 lines, -172 lines 0 comments Download
M trunk/src/base/prefs/persistent_pref_store.h View 1 chunk +11 lines, -14 lines 0 comments Download
M trunk/src/base/prefs/pref_filter.h View 2 chunks +7 lines, -19 lines 0 comments Download
M trunk/src/chrome/browser/prefs/chrome_pref_service_factory.cc View 1 chunk +1 line, -16 lines 0 comments Download
D trunk/src/chrome/browser/prefs/interceptable_pref_filter.h View 1 chunk +0 lines, -64 lines 0 comments Download
D trunk/src/chrome/browser/prefs/interceptable_pref_filter.cc View 1 chunk +0 lines, -38 lines 0 comments Download
M trunk/src/chrome/browser/prefs/pref_hash_filter.h View 3 chunks +17 lines, -14 lines 0 comments Download
M trunk/src/chrome/browser/prefs/pref_hash_filter.cc View 4 chunks +80 lines, -36 lines 0 comments Download
M trunk/src/chrome/browser/prefs/pref_hash_filter_unittest.cc View 27 chunks +165 lines, -89 lines 0 comments Download
M trunk/src/chrome/browser/prefs/profile_pref_store_manager.h View 1 chunk +4 lines, -0 lines 0 comments Download
M trunk/src/chrome/browser/prefs/profile_pref_store_manager.cc View 6 chunks +44 lines, -30 lines 0 comments Download
M trunk/src/chrome/browser/prefs/profile_pref_store_manager_unittest.cc View 4 chunks +6 lines, -14 lines 0 comments Download
M trunk/src/chrome/browser/prefs/tracked/segregated_pref_store.h View 3 chunks +12 lines, -5 lines 0 comments Download
M trunk/src/chrome/browser/prefs/tracked/segregated_pref_store.cc View 4 chunks +36 lines, -16 lines 0 comments Download
M trunk/src/chrome/browser/prefs/tracked/segregated_pref_store_unittest.cc View 5 chunks +29 lines, -2 lines 0 comments Download
D trunk/src/chrome/browser/prefs/tracked/tracked_preferences_migration.h View 1 chunk +0 lines, -37 lines 0 comments Download
D trunk/src/chrome/browser/prefs/tracked/tracked_preferences_migration.cc View 1 chunk +0 lines, -267 lines 0 comments Download
D trunk/src/chrome/browser/prefs/tracked/tracked_preferences_migration_unittest.cc View 1 chunk +0 lines, -501 lines 0 comments Download
M trunk/src/chrome/chrome_browser.gypi View 2 chunks +0 lines, -4 lines 0 comments Download
M trunk/src/chrome/chrome_tests_unit.gypi View 1 chunk +0 lines, -1 line 0 comments Download
M trunk/src/tools/lsan/suppressions.txt View 1 chunk +0 lines, -3 lines 0 comments Download
M trunk/src/tools/valgrind/memcheck/suppressions.txt View 1 chunk +0 lines, -9 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
ananta
6 years, 7 months ago (2014-05-09 22:30:27 UTC) #1
ananta
6 years, 7 months ago (2014-05-09 22:30:50 UTC) #2
Message was sent while issue was closed.
Committed patchset #1 manually as r269438 (tree was closed).

Powered by Google App Engine
This is Rietveld 408576698