New issue
Advanced search Search tips

Issue 867337 link

Starred by 1 user
Status: Fixed
Owner:
proberge@chromium.org
Closed: Aug 22
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 2
Type: Bug



Sign in to add a comment

Clearing a TrackedPref causes a crash in PrefHashStoreImpl

Project Member Reported by mpawlow...@opera.com, Jul 25 
Chrome Version: 69.0.3486.0 
OS: Win10, Linux (haven't checked other, looks system-independent)

All changes in Tracked Preferences made by the browser are followed with storing updated hashes via PrefHashFilter and PrefHashStore. The mechanism is that PrefHashStore's FilterUpdate() is called by PrefStore when a preference is changed and that pref's path is remembered in |changed_paths_|. Later, PrefHashFilter::FilterSerializeData() is called during serialization and for each stored |changed_path| PrefHashFilter attempts to get the corresponding Value from pref store contents in order to compute a new hash.

However, if the pref in question was not modified but *cleared* (via PrefStore::RemoveValue(), which is called in resposne to PrefService::ClearPref()), PrefHashStore::FilterUpdate() will still be called for the removed path, and eventually PrefHashFilter::FilterSerializeData() will be called but there will be no Value to get from pref store contents. This triggers a DCHECK and then a crash:


[5360:1052:0724/122256.122:FATAL:pref_hash_store_impl.cc(100)] Check failed: split_values. 
Backtrace:
	base::debug::StackTrace::StackTrace [0x00007FFD1A995445+101] (C:\dev\work\chromium\src\base\debug\stack_trace_win.cc:286)
	base::debug::StackTrace::StackTrace [0x00007FFD1A98B49F+31] (C:\dev\work\chromium\src\base\debug\stack_trace.cc:199)
	logging::LogMessage::~LogMessage [0x00007FFD1AA29CA6+134] (C:\dev\work\chromium\src\base\logging.cc:593)
	PrefHashStoreImpl::ComputeSplitMacs [0x00007FFD0B01AFA2+242] (C:\dev\work\chromium\src\services\preferences\tracked\pref_hash_store_impl.cc:102)
	PrefHashFilter::GetOnWriteSynchronousCallbacks [0x00007FFD0B00E761+1137] (C:\dev\work\chromium\src\services\preferences\tracked\pref_hash_filter.cc:348)
	PrefHashFilter::FilterSerializeData [0x00007FFD0B00DE51+97] (C:\dev\work\chromium\src\services\preferences\tracked\pref_hash_filter.cc:170)
	JsonPrefStore::SerializeData [0x00007FFCF4E4FFC1+353] (C:\dev\work\chromium\src\components\prefs\json_pref_store.cc:504)
	base::ImportantFileWriter::DoScheduledWrite [0x00007FFD1AA37010+288] (C:\dev\work\chromium\src\base\files\important_file_writer.cc:290)

Comment 1 by gab@chromium.org, Jul 27

Cc: -bauerb@chromium.org -gab@chromium.org
Owner: proberge@chromium.org
Status: Assigned (was: Untriaged)
@proberge is a better active owner for tracked preferences nowadays.

Comment 2 by proberge@chromium.org, Jul 27 

Thanks for the very detailed bug report! 

You mentioned that the DCHECK was triggered by clearing a preference. Did you observe this through a user action (ex. changing a settings in chrome://settings)? That would make things easier to test and fix.

Comment 3 by mpawlow...@opera.com, Jul 30 

Sadly, I don't know how to trigger this in Chrome. I've encountered this bug in Opera, we have a button to reset prefs to "factory defaults" and it was crashing. I've traced the bug to chromium code and figured other projects could use the fix as well.

The unit test I added simulates the scenario, it would crash without the patch.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 22 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0bbefa8002dcce358a7756ce5679f587d3f8a1c5

commit 0bbefa8002dcce358a7756ce5679f587d3f8a1c5
Author: Maciej Pawlowski <mpawlowski@opera.com>
Date: Wed Aug 22 16:32:36 2018

Fix PrefHashFilter's behavior when clearing a tracked pref

When a tracked pref is cleared, it will no longer be present in the
DictionaryValue that represents the pref store contents, but
FilterUpdate() and FilterSerializeData() will still be called. Make
the code handle nullptr Values safely.

Bug:  867337 
Change-Id: I1840c3f60bd815145a0f765adf5907a35bca1c87
Reviewed-on: https://chromium-review.googlesource.com/1150034
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: proberge <proberge@chromium.org>
Commit-Queue: proberge <proberge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585055}
[modify] https://crrev.com/0bbefa8002dcce358a7756ce5679f587d3f8a1c5/services/preferences/tracked/pref_hash_filter_unittest.cc
[modify] https://crrev.com/0bbefa8002dcce358a7756ce5679f587d3f8a1c5/services/preferences/tracked/pref_hash_store_impl.cc
[modify] https://crrev.com/0bbefa8002dcce358a7756ce5679f587d3f8a1c5/services/preferences/tracked/pref_hash_store_impl_unittest.cc

Comment 6 by proberge@chromium.org, Aug 22

Status: Fixed (was: Assigned)
Maciej's patch should make is so a DCHECK is no longer triggered when a pref is cleared. Marking as resolved.
►

Sign in to add a comment