Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
169323003:
Implementation of leveldb-backed PrefStore (Closed)
|
Created:
6 years, 10 months ago by dgrogan Modified:
6 years, 6 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplementation of leveldb-backed PrefStore.
This is not hooked up yet, migration code from Json-backed stores is needed, among other things.
BUG=362814
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271602
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272671
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273174
Patch Set 1 #
Total comments: 59
Patch Set 2 : Include most of a PersistentPrefStore implementation #
Total comments: 22
Patch Set 3 : added Helper that owns the DB object #
Total comments: 15
Patch Set 4 : now with FileThreadDeserializer #
Total comments: 4
Patch Set 5 : Use PostTaskAndReplyWithResult instead of scoped_refptr #Patch Set 6 : OpenDB does repair; type test #Patch Set 7 : write every 10s #
Total comments: 46
Patch Set 8 : respond to comments #
Total comments: 15
Patch Set 9 : respond to comments #
Total comments: 4
Patch Set 10 : move to error bit masks #Patch Set 11 : fix pref service factory error #
Total comments: 14
Patch Set 12 : respond to comments; add histograms.xml entries #
Total comments: 5
Patch Set 13 : change cast to GetAsDictionary #Patch Set 14 : ToT #Patch Set 15 : Tot #Patch Set 16 : moved to chrome/browser/prefs #Patch Set 17 : removed BASE_PREFS_EXPORT #Patch Set 18 : ToT; small changes necessary #Patch Set 19 : remove unnecessary empty .log file #Patch Set 20 : ToT after revert #Patch Set 21 : Remove substantive code from DCHECK #Patch Set 22 : ToT after revert #Patch Set 23 : Close the database after each test #Messages
Total messages: 61 (0 generated)
Hi Mattias, Could you review this? In the interesting of making this initial review small I ripped out just about everything that wasn't necessary to make the two most basic tests pass. A more complete, yet still very incomplete, implementation is at https://codereview.chromium.org/25428002/
OK, here is a first round of comments. After reviewing, I realized that it might be better after all to review the more complete CL because that'll hopefully get us a basically functional PersistentPrefStore implementation. My earlier comment regarding limiting CL size was in anticipation of all the migration stuff needed - I'm totally fine reviewing the LevelDBPrefStore implementation in a singe CL. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:7: #include <algorithm> unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:8: #include <set> unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:12: #include "base/file_util.h" unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:13: #include "base/json/json_file_value_serializer.h" unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:16: #include "base/memory/ref_counted.h" unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:17: #include "base/message_loop/message_loop_proxy.h" Unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:18: #include "base/prefs/pref_filter.h" unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:20: #include "base/threading/sequenced_worker_pool.h" unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:22: #include "third_party/leveldatabase/src/include/leveldb/db.h" Also in .h, but that might be able to use a forward-decl. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:29: read_only_(false), What's the use case for this? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:71: DCHECK(status.ok()); Is this OK? What kind of errors can happen here? Even if they're safe to ignore, please at least put a comment justifying that we're ignoring them here. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:80: DCHECK(serializer.Serialize(*value)); Please no production code in DCHECK, it won't get executed in release builds :) https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:85: &LevelDBPrefStore::PersistOnFileThread, this, key, value_string)); How do you make sure |this| is still alive when PersistOnFileThead runs on the other thread? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:94: PersistFromUIThread(key, new_value.get()); JSONPrefStore batches writes and hits the file only once within 5 second IIRC. Does leveldb do this internally, or should we batch here as well? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:100: base::Value* value) { implement? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:103: void LevelDBPrefStore::RemoveValue(const std::string& key) { implement? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:129: return PREF_READ_ERROR_FILE_OTHER; Is it possible to get some more information about the nature of the error and map that to the PREF_READ_ERROR constants? You may create new ones as well. The story here is that we've always been seeing pref read errors in practice, and this enum gives us some insight on what's going on via UMA. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:141: DCHECK(json_value) << error_message; This should probably not be a DCHECK (unless leveldb catches file corruption?). If deserialization fails, you will want to skip the SetValue call in the line below. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:154: void LevelDBPrefStore::ReadPrefsAsync(ReadErrorDelegate* error_delegate) { implement? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:157: void LevelDBPrefStore::CommitPendingWrite() { If this is not needed, please add a comment explaining why. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:160: void LevelDBPrefStore::ReportValueChanged(const std::string& key) { implement? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:164: bool no_dir) { implement? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_store.h File base/prefs/leveldb_pref_store.h (right): https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.h:18: #include "third_party/leveldatabase/src/include/leveldb/db.h" could be forward-declared instead? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.h:59: void OnStorageRead(PrefReadError error, bool no_dir); Does this need to be public? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.h:73: // thread. This comment seems out of place here? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... File base/prefs/leveldb_pref_store_unittest.cc (right): https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:7: #include "base/file_util.h" unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:13: #include "base/strings/string_number_conversions.h" unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:14: #include "base/strings/string_util.h" unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:15: #include "base/threading/sequenced_worker_pool.h" unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:16: #include "base/threading/thread.h" unused? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:48: MessageLoop message_loop_; #include "base/message_loop/message_loop.h" https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:61: EXPECT_TRUE(value->Equals(actual_value)); This is dangerous, value is now owned by pref_store_ and might no longer be alive. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:73: value = new FundamentalValue(5); |value| leaks. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:76: } I'd like to see more tests, in particular covering the following: - Testing the full range of PersistentPrefStore interface, including RemoveValue, GetMutableValue, SetValueSilently - Various types of read errors - Asynchronous loading (once it's implemented) - Whether the observers are notified correctly
Thanks for the feedback. There's almost a complete PersistentPrefStore implementation here now. I need to work more on handling exceptional situations that arise when the database is opened. If you don't mind ignoring that, could you take a look? No big deal if you'd rather wait until that part is handled as well. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:12: #include "base/file_util.h" On 2014/02/24 09:00:40, Mattias Nissler wrote: > unused? Needed, but the rest were not. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:71: DCHECK(status.ok()); On 2014/02/24 09:00:40, Mattias Nissler wrote: > Is this OK? The corresponding error in JSONPrefStore is ignored, so I think it's ok > What kind of errors can happen here? The two overarching types are Corruption errors and I/O errors. > Even if they're safe to ignore, > please at least put a comment justifying that we're ignoring them here. Sounds good. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:80: DCHECK(serializer.Serialize(*value)); On 2014/02/24 09:00:40, Mattias Nissler wrote: > Please no production code in DCHECK, it won't get executed in release builds :) Done. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:85: &LevelDBPrefStore::PersistOnFileThread, this, key, value_string)); On 2014/02/24 09:00:40, Mattias Nissler wrote: > How do you make sure |this| is still alive when PersistOnFileThead runs on the > other thread? If I'm reading https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=206 correctly, base::Bind will keep reference counted objects alive correctly. Am I misunderstanding? https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:94: PersistFromUIThread(key, new_value.get()); On 2014/02/24 09:00:40, Mattias Nissler wrote: > JSONPrefStore batches writes and hits the file only once within 5 second IIRC. > Does leveldb do this internally, or should we batch here as well? LevelDB does not do this internally. I thought one of the reasons JSONPrefStore did that was because the whole file had to be written at once and you didn't want to write the whole file every time any single pref value changed. LevelDBPrefStore is able to write key-value pairs separately, avoiding the write-the-whole-file-at-once problem. So, at least initially, I'd like to avoid the extra complexity brought on by buffering/batching. Though, of course, if LevelDBPrefStore is shown to perform worse than JSONPrefStore, batching here and writing on a timer is one of the first things I would try. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:141: DCHECK(json_value) << error_message; On 2014/02/24 09:00:40, Mattias Nissler wrote: > This should probably not be a DCHECK (unless leveldb catches file corruption?). > If deserialization fails, you will want to skip the SetValue call in the line > below. Done. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:157: void LevelDBPrefStore::CommitPendingWrite() { On 2014/02/24 09:00:40, Mattias Nissler wrote: > If this is not needed, please add a comment explaining why. Done. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_store.h File base/prefs/leveldb_pref_store.h (right): https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.h:18: #include "third_party/leveldatabase/src/include/leveldb/db.h" On 2014/02/24 09:00:40, Mattias Nissler wrote: > could be forward-declared instead? Done. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.h:59: void OnStorageRead(PrefReadError error, bool no_dir); On 2014/02/24 09:00:40, Mattias Nissler wrote: > Does this need to be public? Nope, changed. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.h:73: // thread. On 2014/02/24 09:00:40, Mattias Nissler wrote: > This comment seems out of place here? Agreed, removed. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... File base/prefs/leveldb_pref_store_unittest.cc (right): https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:7: #include "base/file_util.h" On 2014/02/24 09:00:40, Mattias Nissler wrote: > unused? Needed for PathExists. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:13: #include "base/strings/string_number_conversions.h" On 2014/02/24 09:00:40, Mattias Nissler wrote: > unused? Done. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:14: #include "base/strings/string_util.h" On 2014/02/24 09:00:40, Mattias Nissler wrote: > unused? Done. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:15: #include "base/threading/sequenced_worker_pool.h" On 2014/02/24 09:00:40, Mattias Nissler wrote: > unused? Done. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:16: #include "base/threading/thread.h" On 2014/02/24 09:00:40, Mattias Nissler wrote: > unused? Done. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:48: MessageLoop message_loop_; On 2014/02/24 09:00:40, Mattias Nissler wrote: > #include "base/message_loop/message_loop.h" Done. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:61: EXPECT_TRUE(value->Equals(actual_value)); On 2014/02/24 09:00:40, Mattias Nissler wrote: > This is dangerous, value is now owned by pref_store_ and might no longer be > alive. Fixed. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:73: value = new FundamentalValue(5); On 2014/02/24 09:00:40, Mattias Nissler wrote: > |value| leaks. Done. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store_unittest.cc:76: } On 2014/02/24 09:00:40, Mattias Nissler wrote: > I'd like to see more tests, in particular covering the following: > - Testing the full range of PersistentPrefStore interface, including > RemoveValue, GetMutableValue, SetValueSilently > - Various types of read errors > - Asynchronous loading (once it's implemented) > - Whether the observers are notified correctly Included today is everything except various types of read errors, which I need to work on more.
https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:85: &LevelDBPrefStore::PersistOnFileThread, this, key, value_string)); On 2014/02/25 03:32:08, dgrogan wrote: > On 2014/02/24 09:00:40, Mattias Nissler wrote: > > How do you make sure |this| is still alive when PersistOnFileThead runs on the > > other thread? > > If I'm reading > https://code.google.com/p/chromium/codesearch#chromium/src/base/callback.h&l=206 > correctly, base::Bind will keep reference counted objects alive correctly. Am I > misunderstanding? Note that PrefStore inherits from base::RefCounted, not from base::RefCountedThreadSafe. Thus, using scoped_refptrs to jump threads is not OK in this case. FWIW, the reason for PrefStore being base::RefCounted is to be able to hook them up to multiple different PrefServices, but that all happens on the UI thread. So you'll need a different solution here. You might want to think about moving all the leveldb interactions into an internal object that lives on the respective thread. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:94: PersistFromUIThread(key, new_value.get()); On 2014/02/25 03:32:08, dgrogan wrote: > On 2014/02/24 09:00:40, Mattias Nissler wrote: > > JSONPrefStore batches writes and hits the file only once within 5 second IIRC. > > Does leveldb do this internally, or should we batch here as well? > > LevelDB does not do this internally. I thought one of the reasons JSONPrefStore > did that was because the whole file had to be written at once and you didn't > want to write the whole file every time any single pref value changed. > LevelDBPrefStore is able to write key-value pairs separately, avoiding the > write-the-whole-file-at-once problem. So, at least initially, I'd like to avoid > the extra complexity brought on by buffering/batching. > > Though, of course, if LevelDBPrefStore is shown to perform worse than > JSONPrefStore, batching here and writing on a timer is one of the first things I > would try. OK, are you planning to run performance tests on the two to find out? https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:65: // DCHECK is fine; the corresponding error is ignored in json_pref_store. nit: JsonPrefStore https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:66: // There's also no API available to surface the error back up to the caller. Can you add a sentence explaining the consequences? One difference between LevelDBPrefStore and JsonPrefStore is that latter as all-or-nothing semantics, whereas this code could run into an error for a single pref and then happily continue writing afterwards. So from the point of view of consuming code, the next Chrome restart will just time-warp to a point in the past with JsonPrefStore, whereas LevelDBPrefStore as it is now gives you a totally unpredictable result. Have you assessed whether that's OK? If not, another option might be to flip to read_only_ on the first error we see, which would match JsonPrefStore semantics more closely. Also, do we want UMA for this? https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:74: // DCHECK is fine; the corresponding error is ignored in json_pref_store. nit: JsonPrefStore https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:167: // TODO(dgrogan): Add UMA histogram? I think the PREF_READ_ERROR values are already histogrammed in some outer code layer, but it may make sense to add another one here for raw leveldb status. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:189: // TODO(dgrogan): Add error histogram. see comment above https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store.h (right): https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.h:26: class SequencedWorkerPool; not used? if it is, then please alphabetize https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store_unittest.cc (right): https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store_unittest.cc:49: DCHECK_EQ(LevelDBPrefStore::PREF_READ_ERROR_NONE, pref_store_->ReadPrefs()); This should probably be an EXPECT_EQ or ASSERT_EQ? https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store_unittest.cc:70: base::Value* value = new FundamentalValue(5); nit: no need for the |value| variable, just inline on next line. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store_unittest.cc:103: EXPECT_CALL(mock_observer, OnPrefValueChanged(key)).Times(1); The gmock docs say: Important note: Google Mock requires expectations to be set before the mock functions are called, otherwise the behavior is undefined. In particular, you mustn't interleave EXPECT_CALL()s and calls to the mock functions. (from https://code.google.com/p/googlemock/wiki/ForDummies) The only exception you get is Mock::VerifyAndClearExpectations to start a new cycle, which you should put after line 99 here. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store_unittest.cc:150: ->Get("key2", &inner_value)); The canonical way to do this is to call GetAsDictionary() on retrieved_value and GetInteger() on the result. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store_unittest.cc:164: EXPECT_TRUE(inner_fundamental_value->GetAsInteger(&inner_integer)); same comments regarding reading the integer here. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store_unittest.cc:198: Another test that makes sure all different Value types work would be nice to have. I suspect that this works thanks to JSON serialization working correctly, but then there are exceptions like base::Value::TYPE_BINARY, and the test may serve as documentation what's expected to work.
https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:85: &LevelDBPrefStore::PersistOnFileThread, this, key, value_string)); > So you'll need a different solution here. You might want to think about moving > all the leveldb interactions into an internal object that lives on the > respective thread. Wouldn't this just move the problem to the internal object? What would control the lifetime of the internal object? Do you know of somewhere in the codebase that follows this pattern that I can model after or look at?
https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:85: &LevelDBPrefStore::PersistOnFileThread, this, key, value_string)); On 2014/02/26 07:10:47, dgrogan wrote: > > So you'll need a different solution here. You might want to think about moving > > all the leveldb interactions into an internal object that lives on the > > respective thread. > > Wouldn't this just move the problem to the internal object? What would control > the lifetime of the internal object? The difference is that you can destroy that object on the respective thread. Thus, all operations and deletion are serialized on that thread, which solves the use-after-free. The LevelDBPrefStore object on the UI thread can still be the logical owner and post the destruction task on its own destruction. So the helper object would live on for a little while until tasks on the other thread complete. You'll still have the issue of reporting back read results to the LevelDBPrefStore object, but that can easily be solved with PostTaskAndReply + WeakPtr (note that WeakPtr may be passed across threads, but dereferencing must happen on a the same thread the WeakPtr was created). > Do you know of somewhere in the codebase > that follows this pattern that I can model after or look at? Code that just needs plain file access often uses one-off PostTaskAndReply sequences. One complicating factor here is that you need to hold on to the leveldb::DB* handle created by the open operation (although it might be possible to just pass the DB pointer back to the UI thread and on to subsequent operations if you make sure never to actually access the DB object on the UI thread). Bottom line: I don't know of a good example from the top of my head that I can see as the ideal fit for your situation. Maybe take a look at what other consumers of leveldb do? Another related note: Is the LevelDBPrefStore implementation also supposed to be used for g_browser_process->local_state()? If so, you're facing the additional complication that local_state() starts up before the threads are spinning and goes down after we've cleaned up all threads. Note that JsonPrefStore/ImportantFileWriter have special handling for the case task posting fails for exactly this reason.
On 2014/02/26 08:34:59, Mattias Nissler wrote: > https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... > File base/prefs/leveldb_pref_store.cc (right): > > https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... > base/prefs/leveldb_pref_store.cc:85: &LevelDBPrefStore::PersistOnFileThread, > this, key, value_string)); > On 2014/02/26 07:10:47, dgrogan wrote: > > > So you'll need a different solution here. You might want to think about > moving > > > all the leveldb interactions into an internal object that lives on the > > > respective thread. > > > > Wouldn't this just move the problem to the internal object? What would control > > the lifetime of the internal object? > > The difference is that you can destroy that object on the respective thread. > Thus, all operations and deletion are serialized on that thread, which solves > the use-after-free. The LevelDBPrefStore object on the UI thread can still be > the logical owner and post the destruction task on its own destruction. So the > helper object would live on for a little while until tasks on the other thread > complete. So far I'm with you, and the revision I just uploaded reflects my understanding. (Though that understanding may be off!) > You'll still have the issue of reporting back read results to the > LevelDBPrefStore object, Do you mean in the async case? In the sync case I was planning on having LevelDBPrefStore read from disk on the UI thread, like what JsonPrefStore does. Though this may regress performance enough that I need to make it always async... somehow. > but that can easily be solved with PostTaskAndReply + > WeakPtr (note that WeakPtr may be passed across threads, but dereferencing must > happen on a the same thread the WeakPtr was created). I don't follow... which object would the WeakPtr point at? > > Do you know of somewhere in the codebase > > that follows this pattern that I can model after or look at? > > Code that just needs plain file access often uses one-off PostTaskAndReply > sequences. One complicating factor here is that you need to hold on to the > leveldb::DB* handle created by the open operation (although it might be possible > to just pass the DB pointer back to the UI thread and on to subsequent > operations if you make sure never to actually access the DB object on the UI > thread). Which subsequent operations do you mean? I follow the strategy about passing the DB pointer through the UI thread, but to what end? > Bottom line: I don't know of a good example from the top of my head > that I can see as the ideal fit for your situation. Maybe take a look at what > other consumers of leveldb do? > > Another related note: Is the LevelDBPrefStore implementation also supposed to be > used for g_browser_process->local_state()? That is an open question. In the design doc you suggested that LevelDBPrefStore should replace JsonPrefStore everywhere just so that we don't have to maintain two implementations of PersistentPrefStore, but it's up to us. For now I would say I want to get something in that just works for the per-profile preferences that currently live in Preferences, and worry about Local State later. > If so, you're facing the additional > complication that local_state() starts up before the threads are spinning and > goes down after we've cleaned up all threads. Note that > JsonPrefStore/ImportantFileWriter have special handling for the case task > posting fails for exactly this reason. Thanks for your help, I appreciate it. Sorry to need to ask so many questions.
https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:167: helper_.reset(new Helper(make_scoped_ptr(db))); This seems sloppy, reading from disk on the UI thread, then passing over the DB handle to live on the File thread.
On 2014/02/27 03:16:34, dgrogan wrote: > On 2014/02/26 08:34:59, Mattias Nissler wrote: > > > https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... > > File base/prefs/leveldb_pref_store.cc (right): > > > > > https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... > > base/prefs/leveldb_pref_store.cc:85: &LevelDBPrefStore::PersistOnFileThread, > > this, key, value_string)); > > On 2014/02/26 07:10:47, dgrogan wrote: > > > > So you'll need a different solution here. You might want to think about > > moving > > > > all the leveldb interactions into an internal object that lives on the > > > > respective thread. > > > > > > Wouldn't this just move the problem to the internal object? What would > control > > > the lifetime of the internal object? > > > > The difference is that you can destroy that object on the respective thread. > > Thus, all operations and deletion are serialized on that thread, which solves > > the use-after-free. The LevelDBPrefStore object on the UI thread can still be > > the logical owner and post the destruction task on its own destruction. So the > > helper object would live on for a little while until tasks on the other thread > > complete. > > So far I'm with you, and the revision I just uploaded reflects my understanding. > (Though that understanding may be off!) > > > You'll still have the issue of reporting back read results to the > > LevelDBPrefStore object, > > Do you mean in the async case? In the sync case I was planning on having > LevelDBPrefStore read from disk on the UI thread, like what JsonPrefStore does. > Though this may regress performance enough that I need to make it always > async... somehow. Synchronous reads on the UI thread are actually intentional for Chrome startup when local_state and the first Profile get brought up. > > > but that can easily be solved with PostTaskAndReply + > > WeakPtr (note that WeakPtr may be passed across threads, but dereferencing > must > > happen on a the same thread the WeakPtr was created). > > I don't follow... which object would the WeakPtr point at? You can pass a WeakPtr<LevelDBPrefStore> to the async read function on the file thread that can then post a task back, or you can use PostTaskAndReply with said WeakPtr for the reply closure, that'll then only run if LevelDBPrefStore is still alive. > > > > Do you know of somewhere in the codebase > > > that follows this pattern that I can model after or look at? > > > > Code that just needs plain file access often uses one-off PostTaskAndReply > > sequences. One complicating factor here is that you need to hold on to the > > leveldb::DB* handle created by the open operation (although it might be > possible > > to just pass the DB pointer back to the UI thread and on to subsequent > > operations if you make sure never to actually access the DB object on the UI > > thread). > > Which subsequent operations do you mean? I follow the strategy about passing the > DB pointer through the UI thread, but to what end? You'll need the DB pointer for subsequent write and remove operations. I was alluding to the option of not rolling an explicit helper object living on the file thread, but instead passing the DB pointer to one-off closures running on the file thread for write/remove/close operations. > > > Bottom line: I don't know of a good example from the top of my head > > that I can see as the ideal fit for your situation. Maybe take a look at what > > other consumers of leveldb do? > > > > Another related note: Is the LevelDBPrefStore implementation also supposed to > be > > used for g_browser_process->local_state()? > > That is an open question. In the design doc you suggested that LevelDBPrefStore > should replace JsonPrefStore everywhere just so that we don't have to maintain > two implementations of PersistentPrefStore, but it's up to us. For now I would > say I want to get something in that just works for the per-profile preferences > that currently live in Preferences, and worry about Local State later. SGTM. > > > If so, you're facing the additional > > complication that local_state() starts up before the threads are spinning and > > goes down after we've cleaned up all threads. Note that > > JsonPrefStore/ImportantFileWriter have special handling for the case task > > posting fails for exactly this reason. > > Thanks for your help, I appreciate it. Sorry to need to ask so many questions.
OK, your new approach to threading looks reasonable - see also my other comments in reply to your reply :) https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:16: This should have a comment saying that it gets created on the UI thread but then operated exclusively on the file thread. https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:21: void PersistOnFileThread(const std::string& key, const std::string& value) { I'd just name the functions here WriteValue() and RemoveValue() or similar. By virtue of being defined on the helper object it's clear to me that they run on the file thread. https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:34: scoped_ptr<leveldb::DB> db_; nit: DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:88: return; nit: DCHECK(helper_) https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:104: nit: DCHECK(helper_) https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:167: helper_.reset(new Helper(make_scoped_ptr(db))); On 2014/02/27 03:19:48, dgrogan wrote: > This seems sloppy, reading from disk on the UI thread, then passing over the DB > handle to live on the File thread. If you want, add a comment saying that this file operation is intentionally running on the UI thread during browser startup and that code running later will have to go through ReadPrefsAsync(). Btw., this is enforced by ThreadRestrictions which will assert for file operations on the UI thread after browser startup. I hope it does so too for leveldb operations, but might be good to check. If it doesn't assert, I suggest putting an explicit ThreadRestrictions::AssertIOAllowed() here. https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:214: base::IgnoreResult(&LevelDBPrefStore::ReadPrefs), this)); This needs more love, you can't just updated |prefs_| from a different thread.
Hi Mattias, I haven't addressed all your comments yet* but wanted to get your feedback on the architecture as it's progressing. Namely, LevelDBPrefStore is now using a FileThreadDeserializer copied from JsonPrefStore to support the ReadPrefs(Async) functions in response to your "needs more love" comment. (*) An additional test that shows which types can be stored in a LevelDBPrefStore; and maybe flipping read_only to true when WriteValue fails. Thanks for all your help so far. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:94: PersistFromUIThread(key, new_value.get()); Yes. Note that I don't yet know how, but it's on my list of things to do before shipping. https://codereview.chromium.org/169323003/diff/1/base/prefs/leveldb_pref_stor... base/prefs/leveldb_pref_store.cc:129: return PREF_READ_ERROR_FILE_OTHER; On 2014/02/24 09:00:40, Mattias Nissler wrote: > Is it possible to get some more information about the nature of the error and > map that to the PREF_READ_ERROR constants? You may create new ones as well. The > story here is that we've always been seeing pref read errors in practice, and > this enum gives us some insight on what's going on via UMA. Done. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:65: // DCHECK is fine; the corresponding error is ignored in json_pref_store. On 2014/02/25 10:51:44, Mattias Nissler wrote: > nit: JsonPrefStore Done. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:74: // DCHECK is fine; the corresponding error is ignored in json_pref_store. On 2014/02/25 10:51:44, Mattias Nissler wrote: > nit: JsonPrefStore Done. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store.h (right): https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.h:26: class SequencedWorkerPool; On 2014/02/25 10:51:44, Mattias Nissler wrote: > not used? > if it is, then please alphabetize Done. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store_unittest.cc (right): https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store_unittest.cc:49: DCHECK_EQ(LevelDBPrefStore::PREF_READ_ERROR_NONE, pref_store_->ReadPrefs()); On 2014/02/25 10:51:44, Mattias Nissler wrote: > This should probably be an EXPECT_EQ or ASSERT_EQ? Done. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store_unittest.cc:70: base::Value* value = new FundamentalValue(5); On 2014/02/25 10:51:44, Mattias Nissler wrote: > nit: no need for the |value| variable, just inline on next line. Done. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store_unittest.cc:103: EXPECT_CALL(mock_observer, OnPrefValueChanged(key)).Times(1); Thanks, done. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store_unittest.cc:150: ->Get("key2", &inner_value)); On 2014/02/25 10:51:44, Mattias Nissler wrote: > The canonical way to do this is to call GetAsDictionary() on retrieved_value and > GetInteger() on the result. Done. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store_unittest.cc:164: EXPECT_TRUE(inner_fundamental_value->GetAsInteger(&inner_integer)); On 2014/02/25 10:51:44, Mattias Nissler wrote: > same comments regarding reading the integer here. Done. https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:16: On 2014/02/27 08:49:23, Mattias Nissler wrote: > This should have a comment saying that it gets created on the UI thread but then > operated exclusively on the file thread. Done. https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:21: void PersistOnFileThread(const std::string& key, const std::string& value) { On 2014/02/27 08:49:23, Mattias Nissler wrote: > I'd just name the functions here WriteValue() and RemoveValue() or similar. By > virtue of being defined on the helper object it's clear to me that they run on > the file thread. Done. https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:34: scoped_ptr<leveldb::DB> db_; On 2014/02/27 08:49:23, Mattias Nissler wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:88: return; On 2014/02/27 08:49:23, Mattias Nissler wrote: > nit: DCHECK(helper_) Done. https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:104: On 2014/02/27 08:49:23, Mattias Nissler wrote: > nit: DCHECK(helper_) Done. https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:167: helper_.reset(new Helper(make_scoped_ptr(db))); On 2014/02/27 08:49:23, Mattias Nissler wrote: > On 2014/02/27 03:19:48, dgrogan wrote: > > This seems sloppy, reading from disk on the UI thread, then passing over the > DB > > handle to live on the File thread. > > If you want, add a comment saying that this file operation is intentionally > running on the UI thread during browser startup and that code running later will > have to go through ReadPrefsAsync(). > > Btw., this is enforced by ThreadRestrictions which will assert for file > operations on the UI thread after browser startup. I hope it does so too for > leveldb operations, but might be good to check. If it doesn't assert, I suggest > putting an explicit ThreadRestrictions::AssertIOAllowed() here. Comment and ThreadRestrictions::AssertIOAllowed added. LevelDB does not have ThreadRestrictions::AssertIOAllowed in its functions that touch disk. https://codereview.chromium.org/169323003/diff/80001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:214: base::IgnoreResult(&LevelDBPrefStore::ReadPrefs), this)); On 2014/02/27 08:49:23, Mattias Nissler wrote: > This needs more love, you can't just updated |prefs_| from a different thread. I resurrected FileThreadDeserializer to go with a tried-and-true approach.
Just a high-level comment for now, let me know if you want me to do a deep dive. https://codereview.chromium.org/169323003/diff/90001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/90001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:42: : public base::RefCountedThreadSafe<FileThreadDeserializer> { See https://groups.google.com/a/chromium.org/d/msg/chromium-dev/o05a009KHsQ/pyw7a... for recent coverage of RefCountedThreadSafe. I wouldn't recommend using it in new code, and agree that clear ownership is usually the better design. In this case, you most likely want PostTaskAndReply with a static function that does the reading and passes back the results in the reply. https://codereview.chromium.org/169323003/diff/90001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store.h (right): https://codereview.chromium.org/169323003/diff/90001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.h:89: scoped_ptr<FileThreadSerializer> helper_; This should probably be named |serializer_|.
High-level comment is perfect for now, thanks. Will rework with PostTaskAndReply.
I reworked it to use PostTaskAndReplyWithResult instead of scoped_refptr. Comments welcome but if I don't hear from you, no big deal, I have a bunch of tests to write. https://codereview.chromium.org/169323003/diff/90001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/90001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:42: : public base::RefCountedThreadSafe<FileThreadDeserializer> { On 2014/03/05 18:28:15, Mattias Nissler wrote: > See > https://groups.google.com/a/chromium.org/d/msg/chromium-dev/o05a009KHsQ/pyw7a... > for recent coverage of RefCountedThreadSafe. I wouldn't recommend using it in > new code, and agree that clear ownership is usually the better design. > > In this case, you most likely want PostTaskAndReply with a static function that > does the reading and passes back the results in the reply. Done. https://codereview.chromium.org/169323003/diff/90001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store.h (right): https://codereview.chromium.org/169323003/diff/90001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.h:89: scoped_ptr<FileThreadSerializer> helper_; On 2014/03/05 18:28:15, Mattias Nissler wrote: > This should probably be named |serializer_|. Done.
This is ready for another look. It has been reworked to write only every 10 seconds like JsonPrefStore does. https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store.cc:66: // There's also no API available to surface the error back up to the caller. On 2014/02/25 10:51:44, Mattias Nissler wrote: > Can you add a sentence explaining the consequences? One difference between > LevelDBPrefStore and JsonPrefStore is that latter as all-or-nothing semantics, > whereas this code could run into an error for a single pref and then happily > continue writing afterwards. So from the point of view of consuming code, the > next Chrome restart will just time-warp to a point in the past with > JsonPrefStore, whereas LevelDBPrefStore as it is now gives you a totally > unpredictable result. Have you assessed whether that's OK? I think this is not ok, thanks for bringing it up. Existing clients might rely on the fact that preference values set in the same turn of the event loop will be persisted together; JsonPrefStore implicitly supports transaction-like semantics. To make LevelDBPrefStore support the same I'll need to do the batching and writing every 10 seconds like we talked about in another comment. > If not, another option might be to flip to read_only_ on the first error we see, > which would match JsonPrefStore semantics more closely. > > Also, do we want UMA for this? https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... File base/prefs/leveldb_pref_store_unittest.cc (right): https://codereview.chromium.org/169323003/diff/60001/base/prefs/leveldb_pref_... base/prefs/leveldb_pref_store_unittest.cc:198: On 2014/02/25 10:51:44, Mattias Nissler wrote: > Another test that makes sure all different Value types work would be nice to > have. I suspect that this works thanks to JSON serialization working correctly, > but then there are exceptions like base::Value::TYPE_BINARY, and the test may > serve as documentation what's expected to work. Added.
This looks reasonably good now, mostly nits that remain. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:22: ReadingResults() : db_owned(NULL), value_map_owned(NULL) { nit: no need for line break. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:25: leveldb::DB* db_owned; Why aren't these scoped_ptrs if they are owned? https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:83: status = leveldb::RepairDB(path.AsUTF8Unsafe(), options); Is it OK to pass options.create_if_missing = true here? https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:87: // destroying it? Yes, please. That way, we will have a better chance to get samples from the field for the corruption case for investigation. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:88: status = leveldb::DestroyDB(path.AsUTF8Unsafe(), options); again, create_if_missing OK here? https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:100: if (destroy_succeeded) nit: I suggest curly braces here; the line breaks below make this hard to read otherwise. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:105: PersistentPrefStore::PREF_READ_ERROR_LEVELDB_REPAIRED_REOPEN_FAILED; Should we destroy and retry in this case? https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:108: if (destroy_succeeded) nit: curly braces suggested https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:141: // TODO(dgrogan): UMA? Hm, good question. Another option would be to crash (i.e. LOG(FATAL)), which is appropriate if we're confident in level DB catching all corruption internally and we are OK with the assumption that nobody else messes with our DB. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:147: // TODO(dgrogan): UMA? I don't think that's necessary, the pref read error will get histogrammed anyways, no? https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:327: keys_to_set_.erase(key); Can you put a comment here explaining that the erase is needed so set and delete operations ending up in the same batch don't clobber each other? Also below. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:344: keys_to_set_[key] = value_string; This should be MarkForInsertion(key, value_string); https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:379: NOTREACHED() << "Unknown error: " << read_error_; You have introduced a bunch of new read errors, shouldn't they be handled here? https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.h (right): https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.h:65: struct ReadingResults; nit: should go at the top of the private section per style guide https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.h:98: class FileThreadSerializer; nit: move to the top of private section https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.h:99: scoped_ptr<FileThreadSerializer> serializer_; Probably worthwhile to put a comment regarding lifecycle management of the object. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store_unittest.cc (right): https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:12: #include "base/prefs/pref_filter.h" used? https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:73: pref_store_->SetValue(key, new FundamentalValue(5)); Might want to put a GetValue check here before running the loop - the new value should be visible immediately. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:147: EXPECT_TRUE(pref_store_->GetValue(key, &retrieved_value)); Suggestion: Instead of decoding the dictionary by hand below, it may be easier to just keep a dictionary in the expected state and call EXPECT_TRUE(Value::Equals()) https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:149: EXPECT_TRUE(retrieved_value->GetAsDictionary(&dictionary)); This should be ASSERT_TRUE() (here and below), the test will crash otherwise if |dictionary| is not valid below. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:158: EXPECT_TRUE(pref_store_->GetValue(key, &retrieved_value)); ... and you could reuse that expected dictionary here. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:184: TEST_F(LevelDBPrefStoreTest, RemoveFromDisk) { Isn't this mostly identical with RemoveFromMemory above? Might just want to merge the two tests. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:278: DictionaryValue* dict_value = new DictionaryValue; Again, I suggest keeping copies of these objects so you can do EXPECT_TRUE(Value::Equals()) below and safe some lines.
Hi Mattias, this is (finally) ready for another look. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:22: ReadingResults() : db_owned(NULL), value_map_owned(NULL) { On 2014/03/20 09:32:25, Mattias Nissler wrote: > nit: no need for line break. Constructor removed. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:25: leveldb::DB* db_owned; On 2014/03/20 09:32:25, Mattias Nissler wrote: > Why aren't these scoped_ptrs if they are owned? Oops, meant to switch them before sending out. Done now. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:83: status = leveldb::RepairDB(path.AsUTF8Unsafe(), options); On 2014/03/20 09:32:25, Mattias Nissler wrote: > Is it OK to pass options.create_if_missing = true here? Yes, it's ignored by RepairDB. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:87: // destroying it? On 2014/03/20 09:32:25, Mattias Nissler wrote: > Yes, please. That way, we will have a better chance to get samples from the > field for the corruption case for investigation. Done. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:88: status = leveldb::DestroyDB(path.AsUTF8Unsafe(), options); On 2014/03/20 09:32:25, Mattias Nissler wrote: > again, create_if_missing OK here? Yep. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:100: if (destroy_succeeded) On 2014/03/20 09:32:25, Mattias Nissler wrote: > nit: I suggest curly braces here; the line breaks below make this hard to read > otherwise. Done. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:105: PersistentPrefStore::PREF_READ_ERROR_LEVELDB_REPAIRED_REOPEN_FAILED; On 2014/03/20 09:32:25, Mattias Nissler wrote: > Should we destroy and retry in this case? Done. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:108: if (destroy_succeeded) On 2014/03/20 09:32:25, Mattias Nissler wrote: > nit: curly braces suggested Done. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:141: // TODO(dgrogan): UMA? On 2014/03/20 09:32:25, Mattias Nissler wrote: > Hm, good question. Another option would be to crash (i.e. LOG(FATAL)), which is > appropriate if we're confident in level DB catching all corruption internally > and we are OK with the assumption that nobody else messes with our DB. I'm confident LevelDB will catch all corruption internally but less so about someone messing with our DB. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:147: // TODO(dgrogan): UMA? On 2014/03/20 09:32:25, Mattias Nissler wrote: > I don't think that's necessary, the pref read error will get histogrammed > anyways, no? That's right. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:327: keys_to_set_.erase(key); On 2014/03/20 09:32:25, Mattias Nissler wrote: > Can you put a comment here explaining that the erase is needed so set and delete > operations ending up in the same batch don't clobber each other? Also below. Done. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:344: keys_to_set_[key] = value_string; On 2014/03/20 09:32:25, Mattias Nissler wrote: > This should be MarkForInsertion(key, value_string); Done. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:379: NOTREACHED() << "Unknown error: " << read_error_; On 2014/03/20 09:32:25, Mattias Nissler wrote: > You have introduced a bunch of new read errors, shouldn't they be handled here? Yes, not sure how I missed this. Thanks. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.h (right): https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.h:65: struct ReadingResults; On 2014/03/20 09:32:25, Mattias Nissler wrote: > nit: should go at the top of the private section per style guide Done. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.h:98: class FileThreadSerializer; On 2014/03/20 09:32:25, Mattias Nissler wrote: > nit: move to the top of private section Done. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.h:99: scoped_ptr<FileThreadSerializer> serializer_; On 2014/03/20 09:32:25, Mattias Nissler wrote: > Probably worthwhile to put a comment regarding lifecycle management of the > object. Done. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store_unittest.cc (right): https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:12: #include "base/prefs/pref_filter.h" On 2014/03/20 09:32:25, Mattias Nissler wrote: > used? Removed. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:73: pref_store_->SetValue(key, new FundamentalValue(5)); On 2014/03/20 09:32:25, Mattias Nissler wrote: > Might want to put a GetValue check here before running the loop - the new value > should be visible immediately. Removed the RunUntilIdle completely, it wasn't serving any purpose. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:147: EXPECT_TRUE(pref_store_->GetValue(key, &retrieved_value)); On 2014/03/20 09:32:25, Mattias Nissler wrote: > Suggestion: Instead of decoding the dictionary by hand below, it may be easier > to just keep a dictionary in the expected state and call > EXPECT_TRUE(Value::Equals()) Done. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:149: EXPECT_TRUE(retrieved_value->GetAsDictionary(&dictionary)); On 2014/03/20 09:32:25, Mattias Nissler wrote: > This should be ASSERT_TRUE() (here and below), the test will crash otherwise if > |dictionary| is not valid below. Obviated by your suggestion above. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:158: EXPECT_TRUE(pref_store_->GetValue(key, &retrieved_value)); On 2014/03/20 09:32:25, Mattias Nissler wrote: > ... and you could reuse that expected dictionary here. Done. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:184: TEST_F(LevelDBPrefStoreTest, RemoveFromDisk) { On 2014/03/20 09:32:25, Mattias Nissler wrote: > Isn't this mostly identical with RemoveFromMemory above? Might just want to > merge the two tests. The second was written because the first didn't catch a bug in the implementation. Instead of merging, I made them more distinct so it's clear they are testing different codepaths. https://codereview.chromium.org/169323003/diff/150001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:278: DictionaryValue* dict_value = new DictionaryValue; On 2014/03/20 09:32:25, Mattias Nissler wrote: > Again, I suggest keeping copies of these objects so you can do > EXPECT_TRUE(Value::Equals()) below and safe some lines. Done.
https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:106: if (destroy_succeeded) { nit: curlies not needed or you could convert lines 106-110 into a single ternary expression. https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:132: reading_results->db.reset(db); Quite some error code proliferation here now, see below for a proposal on how this could be handled in a better way. https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:145: if (!reading_results->db.get()) { nit: no need for .get() here, scoped_ptr works fine in boolean context. https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:239: scoped_ptr<std::set<std::string>> keys_to_delete(new std::set<std::string>); nit: >> is C++11, which I don't think is allowed in chromium src yet. https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:250: base::Passed(&keys_to_set), nit: Could use base::Owned() here and avoid the scoped_ptr in parameter decls for WriteToDatabase. https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:412: case PREF_READ_ERROR_LEVELDB_SOME_DATA_LOST_AFTER_REPAIR: The error codes here seem like they'd be better served by a bit mask with bits for destroyed, repaired, open-failed, data-lost. I wonder whether we can just handle them as such? It might be cleaner after all to just introduce a histogram for these bits (and sample all of the applicable bits) and report an report PREF_READ_IO_ERROR to the caller? WDYT? https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.h (right): https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.h:104: std::set<std::string> keys_to_delete_; Please document |keys_to_delete_|, |keys_to_set_| and |timer_|
https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:106: if (destroy_succeeded) { On 2014/04/14 10:11:41, Mattias Nissler wrote: > nit: curlies not needed or you could convert lines 106-110 into a single ternary > expression. Done. https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:132: reading_results->db.reset(db); On 2014/04/14 10:11:41, Mattias Nissler wrote: > Quite some error code proliferation here now, see below for a proposal on how > this could be handled in a better way. Yeah, I was dismayed at the explosion of errors... https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:145: if (!reading_results->db.get()) { On 2014/04/14 10:11:41, Mattias Nissler wrote: > nit: no need for .get() here, scoped_ptr works fine in boolean context. Done. https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:239: scoped_ptr<std::set<std::string>> keys_to_delete(new std::set<std::string>); On 2014/04/14 10:11:41, Mattias Nissler wrote: > nit: >> is C++11, which I don't think is allowed in chromium src yet. Done. https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:250: base::Passed(&keys_to_set), On 2014/04/14 10:11:41, Mattias Nissler wrote: > nit: Could use base::Owned() here and avoid the scoped_ptr in parameter decls > for WriteToDatabase. Done. https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:412: case PREF_READ_ERROR_LEVELDB_SOME_DATA_LOST_AFTER_REPAIR: On 2014/04/14 10:11:41, Mattias Nissler wrote: > The error codes here seem like they'd be better served by a bit mask with bits > for destroyed, repaired, open-failed, data-lost. I wonder whether we can just > handle them as such? I'd be worried about losing information about which exact code path was taken. > It might be cleaner after all to just introduce a histogram for these bits (and > sample all of the applicable bits) and report an report PREF_READ_IO_ERROR to > the caller? WDYT? Well, we'd need to report one of at least two error codes to the caller - one to signify read only and another to signify non read only - so that HandleReadError can show the proper error message to the user. But I think I understand what you mean. I'd be ok with recording to a histogram inside this class and just surfacing one of the two error codes mentioned above, except that I want to keep separate histograms for each consumer of this class. I suppose I could add yet another parameter to the LevelDBPrefStore constructor that would allow the caller to specify a string to be used in the UMA histogram names. WDYT? https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.h (right): https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.h:104: std::set<std::string> keys_to_delete_; On 2014/04/14 10:11:41, Mattias Nissler wrote: > Please document |keys_to_delete_|, |keys_to_set_| and |timer_| Done.
https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/170001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:412: case PREF_READ_ERROR_LEVELDB_SOME_DATA_LOST_AFTER_REPAIR: On 2014/04/17 01:10:27, dgrogan wrote: > On 2014/04/14 10:11:41, Mattias Nissler wrote: > > The error codes here seem like they'd be better served by a bit mask with bits > > for destroyed, repaired, open-failed, data-lost. I wonder whether we can just > > handle them as such? > > I'd be worried about losing information about which exact code path was taken. If you just binary-or flags and send the result to a UMA histogram, you'll not lose any information, and you'll still only have log N flags for N code paths :) > > > It might be cleaner after all to just introduce a histogram for these bits > (and > > sample all of the applicable bits) and report an report PREF_READ_IO_ERROR to > > the caller? WDYT? > > Well, we'd need to report one of at least two error codes to the caller - one to > signify read only and another to signify non read only - so that HandleReadError > can show the proper error message to the user. But I think I understand what you > mean. That should be relatively simple to implement by working internally with a flags bitmap and consulting it to determine whether we're read-only or not and what the PrefReadError to report should be. > > I'd be ok with recording to a histogram inside this class and just surfacing one > of the two error codes mentioned above, except that I want to keep separate > histograms for each consumer of this class. I suppose I could add yet another > parameter to the LevelDBPrefStore constructor that would allow the caller to > specify a string to be used in the UMA histogram names. That makes sense to me, but what's your justification for separate histograms? Do we expect different consumers to actually run into different error modes? If we don't have any indication of this happening in the wild, I'd just keep it simple for now and use a single UMA histogram name. > > WDYT? https://codereview.chromium.org/169323003/diff/190001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.h (right): https://codereview.chromium.org/169323003/diff/190001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.h:103: scoped_ptr<FileThreadSerializer> serializer_; nit: blank line here for readability https://codereview.chromium.org/169323003/diff/190001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.h:105: // stored in the database according to timer_. nit: member names are written as |member_|, see |sequenced_task_runner| above.
Mattias, PTAL? Let me know what you think of the error masks. I didn't fill out histograms.xml with the new labels yet in case what I have now needs to go through radical changes. > > I'd be ok with recording to a histogram inside this class and just surfacing > one > > of the two error codes mentioned above, except that I want to keep separate > > histograms for each consumer of this class. I suppose I could add yet another > > parameter to the LevelDBPrefStore constructor that would allow the caller to > > specify a string to be used in the UMA histogram names. > > That makes sense to me, but what's your justification for separate histograms? > Do we expect different consumers to actually run into different error modes? If > we don't have any indication of this happening in the wild, I'd just keep it > simple for now and use a single UMA histogram name. Well, there's no way to get that indication without separate histograms. Further, the presumption is that more third party programs are altering the Preferences file than other JsonPrefStore files. During and after the transition when Preferences goes from Json to LevelDB, we're going to want to be able to compare the corruption rates between Json and LevelDB and figure out what the causes are. Having said all that, I'm fine using one histogram for now, in the interest of getting the simplest thing that could possibly work checked in and revisiting the issue later.
https://codereview.chromium.org/169323003/diff/190001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.h (right): https://codereview.chromium.org/169323003/diff/190001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.h:103: scoped_ptr<FileThreadSerializer> serializer_; On 2014/04/17 08:50:50, Mattias Nissler wrote: > nit: blank line here for readability Done. https://codereview.chromium.org/169323003/diff/190001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.h:105: // stored in the database according to timer_. On 2014/04/17 08:50:50, Mattias Nissler wrote: > nit: member names are written as |member_|, see |sequenced_task_runner| above. Done.
This looks pretty good already now, just two more suggestions for simplification. https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:25: OPENED = 1, nit: write this as 1 << 0 for consistency https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:25: OPENED = 1, Do we need the OPENED constant? Isn't that redundant with the db pointer (i.e. we must have been able to open the DB if we get a DB pointer). https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:34: FILE_NOT_SPECIFIED = 1 << 9 nit: add trailing comma, that leads to simpler edits for additions/removals. https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:51: } nit: // namespace https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:108: ReadingResults* reading_results) { Hm, I'm wondering whether we can rewrite this function as a loop for clarity: retry = true while (retry) { try to open if (success) return db if ((error & (REPAIRED | REPAIR_FAILED)) == 0) try to repair if (success) continue else if MoveAside continue try to destroy if (success) continue; retry = false; } This would avoid any duplication of operations on the DB AFAICT, and the loop follows a pattern that's easy to understand: Try to open and handle failure by using the next biggest hammer. https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:419: case PREF_READ_ERROR_LEVELDB_CORRUPTION_READ_ONLY: Do we actually need the distinction between CORRUPTION_READ_ONLY and CORRUPTION in the PrefReadError enum? we could just look at reading_results here to determine how to set the read_only_ flag. In fact, you can probably avoid the entire switch statement here and just do if (reading_results->db) { serializer_.reset(...); prefs_.Swap(...); } else { read_only_ = true; } https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:420: DCHECK(reading_results->value_map.get() == NULL); No need to test for .get() == NULL, scoped_ptr works just fine in boolean context.
Ready for another look. Thanks. https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:25: OPENED = 1, On 2014/04/25 12:28:24, Mattias Nissler wrote: > nit: write this as 1 << 0 for consistency Done. https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:25: OPENED = 1, On 2014/04/25 12:28:24, Mattias Nissler wrote: > Do we need the OPENED constant? Isn't that redundant with the db pointer (i.e. > we must have been able to open the DB if we get a DB pointer). I want it for the histogram so that we'll know when the DB was actually opened. E.g. there is a big difference between (REPAIRED | DESTROYED) and (REPAIRED | DESTROYED | OPENED). But your comment made me realize that OPEN_FAILED was unnecessary: the existence of any of DESTROYED, REPAIRED, DESTROY_FAILED, REPAIR_FAILED <=> OPEN_FAILED. https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:34: FILE_NOT_SPECIFIED = 1 << 9 On 2014/04/25 12:28:24, Mattias Nissler wrote: > nit: add trailing comma, that leads to simpler edits for additions/removals. Done. https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:51: } On 2014/04/25 12:28:24, Mattias Nissler wrote: > nit: // namespace Done. https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:108: ReadingResults* reading_results) { I like the loop idea. Adopted a variant of it. https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:419: case PREF_READ_ERROR_LEVELDB_CORRUPTION_READ_ONLY: On 2014/04/25 12:28:24, Mattias Nissler wrote: > Do we actually need the distinction between CORRUPTION_READ_ONLY and CORRUPTION > in the PrefReadError enum? Without the distinction HandleReadError in chrome_pref_service_factory.cc won't know which error message to show the user. Though that is admittedly not a strong reason... > we could just look at reading_results here to > determine how to set the read_only_ flag. In fact, you can probably avoid the > entire switch statement here and just do > > if (reading_results->db) { > serializer_.reset(...); > prefs_.Swap(...); > } else { > read_only_ = true; > } Getting rid of the switch statement is good. Done. https://codereview.chromium.org/169323003/diff/230001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:420: DCHECK(reading_results->value_map.get() == NULL); On 2014/04/25 12:28:24, Mattias Nissler wrote: > No need to test for .get() == NULL, scoped_ptr works just fine in boolean > context. Done.
I like the latest patch set, just one last thing before the magic 4 laters. https://codereview.chromium.org/169323003/diff/250001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/250001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:116: reading_results->error |= OPENED; Note that (error & OPENED) is redundant with reading_results->db != NULL. Given that, let's remove OPENED and just test reading_results->db (you'll probably want to change IntToPrefReadError to take a const ReadingResults* parameter to enable it to do the db check). https://codereview.chromium.org/169323003/diff/250001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store_unittest.cc (right): https://codereview.chromium.org/169323003/diff/250001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:136: static_cast<base::DictionaryValue*>(actual_value); This is usually written as ASSERT_TRUE(actual_value->GetAsDictionary(&dict_value))
https://codereview.chromium.org/169323003/diff/250001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/250001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:116: reading_results->error |= OPENED; On 2014/04/30 09:58:21, Mattias Nissler wrote: > Note that (error & OPENED) is redundant with reading_results->db != NULL. Given > that, let's remove OPENED and just test reading_results->db (you'll probably > want to change IntToPrefReadError to take a const ReadingResults* parameter to > enable it to do the db check). Did you see my comment about this in the last round? Basically I want OPENED for the histogram, to be able to differentiate between REPAIRED | DESTROYED and REPAIRED | DESTROYED | OPENED, among a few others. https://codereview.chromium.org/169323003/diff/250001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store_unittest.cc (right): https://codereview.chromium.org/169323003/diff/250001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store_unittest.cc:136: static_cast<base::DictionaryValue*>(actual_value); On 2014/04/30 09:58:21, Mattias Nissler wrote: > This is usually written as > ASSERT_TRUE(actual_value->GetAsDictionary(&dict_value)) Done.
LGTM https://codereview.chromium.org/169323003/diff/250001/base/prefs/leveldb_pref... File base/prefs/leveldb_pref_store.cc (right): https://codereview.chromium.org/169323003/diff/250001/base/prefs/leveldb_pref... base/prefs/leveldb_pref_store.cc:116: reading_results->error |= OPENED; On 2014/05/01 09:00:15, dgrogan wrote: > On 2014/04/30 09:58:21, Mattias Nissler wrote: > > Note that (error & OPENED) is redundant with reading_results->db != NULL. > Given > > that, let's remove OPENED and just test reading_results->db (you'll probably > > want to change IntToPrefReadError to take a const ReadingResults* parameter to > > enable it to do the db check). > > Did you see my comment about this in the last round? Basically I want OPENED for > the histogram, to be able to differentiate between REPAIRED | DESTROYED and > REPAIRED | DESTROYED | OPENED, among a few others. Ah, I had missed the point about OPENED in the histogram. OK then.
Ilya, could you review this for histograms.xml?
histograms lgtm
Albert, could you review this as a base/ OWNER?
I don't think we should add a leveldb dep on base. I've never been a fan of base/prefs. I think this can probably be moved to a component in which case there would be no problems with this kind of thing. I think base/prefs happened before we had components so its possible we could just move the code with no issues.
Joi, what is your opinion on moving base/prefs to components? Should it go in components/user_prefs?
On 2014/05/03 00:52:41, brettw wrote: > I don't think we should add a leveldb dep on base. > > I've never been a fan of base/prefs. I think this can probably be moved to a > component in which case there would be no problems with this kind of thing. > > I think base/prefs happened before we had components so its possible we could > just move the code with no issues. That is right. //base/prefs - https://chromiumcodereview.appspot.com/11145019 - Oct 22, 2012 //components - https://chromiumcodereview.appspot.com/11648012 - Dec 27, 2012
On 2014/05/03 03:15:33, dgrogan wrote: > Joi, what is your opinion on moving base/prefs to components? Should it go in > components/user_prefs? Note that Jói is no longer with the team, so he'll probably not reply here. I seem to remember having discussed making prefs a component with Jói when he did the refactoring that ended up moving it to base/, but I don't remember details unfortunately :-/ You may want to check the review threads from back then for additional context. FWIW, I can see two way forwards: (1) Move prefs out ouf base/ into an own component and keep LevelDBPrefStore as part of the prefs component (2) Move LevelDBPrefStore into chrome/browser/prefs. All PrefService intialization gets triggered from there anyways, and it's easy to swap JsonPrefStore for LevelDBPrefStore at that point.
Hi guys, Sorry for the late response. I'm still a Chromium committer and am keeping up with my Chromium email and code reviews, just not as conscientiously as before. I think you should be careful about making //components a dumping ground for anything that doesn't belong elsewhere (since this will in the end give you a grouping no better than just letting everything be a separate top-level directory), but I will defer to the current OWNERS on this point (Colin Blundell and Jochen Eisinger, now CCed). I originally wanted //components to strictly be a layer between //content and //chrome (or other top-level applications) but was convinced that it would be better to allow components that do not depend on //content, with a definition for //components of something like "optional components that a browser-like application built on //content might want to incorporate". To me, Prefs is a lower-level type of thing that many kind of top-level applications, not just browser-like things that use content, might want to use. I don't have a strong opinion on where it should move if it really needs to move out of //base. It could be a top-level directory or I guess it could be a subdirectory of //components; just be careful of the direction you're taking //components if there is no good determinant of whether something belongs there or not. Cheers, Jói On Mon, May 5, 2014 at 11:26 AM, <mnissler@chromium.org> wrote: > On 2014/05/03 03:15:33, dgrogan wrote: >> >> Joi, what is your opinion on moving base/prefs to components? Should it go >> in >> components/user_prefs? > > > Note that Jói is no longer with the team, so he'll probably not reply here. > I > seem to remember having discussed making prefs a component with Jói when he > did > the refactoring that ended up moving it to base/, but I don't remember > details > unfortunately :-/ You may want to check the review threads from back then > for > additional context. > > FWIW, I can see two way forwards: > (1) Move prefs out ouf base/ into an own component and keep LevelDBPrefStore > as > part of the prefs component > (2) Move LevelDBPrefStore into chrome/browser/prefs. All PrefService > intialization gets triggered from there anyways, and it's easy to swap > JsonPrefStore for LevelDBPrefStore at that point. > > https://codereview.chromium.org/169323003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Looking at the code review a bit, I wonder if //components/user_prefs can be a model for what to do here. It contains code that is for a multi-user-profile browser to hang profile-specific preferences off of a BrowserContext. Can the LevelDB dependency similarly be externalized to a component that is specifically for browser-like applications, and that bit could live under components? Cheers, Jói On Mon, May 5, 2014 at 10:07 PM, Jói Sigurðsson <joi@chromium.org> wrote: > Hi guys, > > Sorry for the late response. I'm still a Chromium committer and am > keeping up with my Chromium email and code reviews, just not as > conscientiously as before. > > I think you should be careful about making //components a dumping > ground for anything that doesn't belong elsewhere (since this will in > the end give you a grouping no better than just letting everything be > a separate top-level directory), but I will defer to the current > OWNERS on this point (Colin Blundell and Jochen Eisinger, now CCed). I > originally wanted //components to strictly be a layer between > //content and //chrome (or other top-level applications) but was > convinced that it would be better to allow components that do not > depend on //content, with a definition for //components of something > like "optional components that a browser-like application built on > //content might want to incorporate". > > To me, Prefs is a lower-level type of thing that many kind of > top-level applications, not just browser-like things that use content, > might want to use. I don't have a strong opinion on where it should > move if it really needs to move out of //base. It could be a top-level > directory or I guess it could be a subdirectory of //components; just > be careful of the direction you're taking //components if there is no > good determinant of whether something belongs there or not. > > Cheers, > Jói > > > On Mon, May 5, 2014 at 11:26 AM, <mnissler@chromium.org> wrote: >> On 2014/05/03 03:15:33, dgrogan wrote: >>> >>> Joi, what is your opinion on moving base/prefs to components? Should it go >>> in >>> components/user_prefs? >> >> >> Note that Jói is no longer with the team, so he'll probably not reply here. >> I >> seem to remember having discussed making prefs a component with Jói when he >> did >> the refactoring that ended up moving it to base/, but I don't remember >> details >> unfortunately :-/ You may want to check the review threads from back then >> for >> additional context. >> >> FWIW, I can see two way forwards: >> (1) Move prefs out ouf base/ into an own component and keep LevelDBPrefStore >> as >> part of the prefs component >> (2) Move LevelDBPrefStore into chrome/browser/prefs. All PrefService >> intialization gets triggered from there anyways, and it's easy to swap >> JsonPrefStore for LevelDBPrefStore at that point. >> >> https://codereview.chromium.org/169323003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/05/05 22:10:30, Jói wrote: > Looking at the code review a bit, I wonder if //components/user_prefs > can be a model for what to do here. It contains code that is for a > multi-user-profile browser to hang profile-specific preferences off of > a BrowserContext. Can the LevelDB dependency similarly be externalized > to a component that is specifically for browser-like applications, and > that bit could live under components? It absolutely can, there's no reason why the LevelDBPrefStore implementation needs to live in //base - you can just plug the desired PersistentPrefStore implementation into your PrefService instance at construction time. If we only use LevelDBPrefStore in chrome, we can just move the implementation to chrome/browser/prefs. If something else wants a PrefService with a LevelDBPrefStore at some point, we can always move it into its own component. > > Cheers, > Jói > > > On Mon, May 5, 2014 at 10:07 PM, Jói Sigurðsson <mailto:joi@chromium.org> wrote: > > Hi guys, > > > > Sorry for the late response. I'm still a Chromium committer and am > > keeping up with my Chromium email and code reviews, just not as > > conscientiously as before. > > > > I think you should be careful about making //components a dumping > > ground for anything that doesn't belong elsewhere (since this will in > > the end give you a grouping no better than just letting everything be > > a separate top-level directory), but I will defer to the current > > OWNERS on this point (Colin Blundell and Jochen Eisinger, now CCed). I > > originally wanted //components to strictly be a layer between > > //content and //chrome (or other top-level applications) but was > > convinced that it would be better to allow components that do not > > depend on //content, with a definition for //components of something > > like "optional components that a browser-like application built on > > //content might want to incorporate". > > > > To me, Prefs is a lower-level type of thing that many kind of > > top-level applications, not just browser-like things that use content, > > might want to use. I don't have a strong opinion on where it should > > move if it really needs to move out of //base. It could be a top-level > > directory or I guess it could be a subdirectory of //components; just > > be careful of the direction you're taking //components if there is no > > good determinant of whether something belongs there or not. > > > > Cheers, > > Jói > > > > > > On Mon, May 5, 2014 at 11:26 AM, <mailto:mnissler@chromium.org> wrote: > >> On 2014/05/03 03:15:33, dgrogan wrote: > >>> > >>> Joi, what is your opinion on moving base/prefs to components? Should it go > >>> in > >>> components/user_prefs? > >> > >> > >> Note that Jói is no longer with the team, so he'll probably not reply here. > >> I > >> seem to remember having discussed making prefs a component with Jói when he > >> did > >> the refactoring that ended up moving it to base/, but I don't remember > >> details > >> unfortunately :-/ You may want to check the review threads from back then > >> for > >> additional context. > >> > >> FWIW, I can see two way forwards: > >> (1) Move prefs out ouf base/ into an own component and keep LevelDBPrefStore > >> as > >> part of the prefs component > >> (2) Move LevelDBPrefStore into chrome/browser/prefs. All PrefService > >> intialization gets triggered from there anyways, and it's easy to swap > >> JsonPrefStore for LevelDBPrefStore at that point. > >> > >> <crxref style="background-image: url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/__extension_id__.png);">https://codereview.chromium.org/169323003/</crxref> > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2014/05/05 22:07:55, Jói wrote: > > I think you should be careful about making //components a dumping > ground for anything that doesn't belong elsewhere This is basically what components is. Smallish re-usable bits of code with clear dependencies. Digging into this, prefs should definitely be a component, and it would have absolutely been put there instead of base had we had it at the time. Whether you decide to move it or not, I think we should proceed with the understanding that it will eventually be moved to components.
> This is basically what components is. Smallish re-usable bits of code with > clear dependencies. [...] If that is the only criteria for being in components, then components/README needs to be updated. It also becomes hard to know why something like src/ipc, src/sql etc. shouldn't also be moved into components (hence my point earlier that it would give you a grouping no better than having everything be top-level). I believe layering in the code base and avoiding dependency creep would be better served by grouping reusable bits of code with the topmost layer that they depend on. Hence, an optional but reusable something that depends on src/sql would probably belong as a subdirectory of src/sql (*). Prefs is another great example. Notice that the current discussion about whether it is OK for Prefs to depend on leveldb never would have happened if Prefs had been in components/prefs rather than base/prefs. I'll stop arguing now though; I removed myself from components/OWNERS and it'll be up to the folks who have taken over ownership to figure this out. I'm also not saying the current location for Prefs is necessarily the only correct location, just that loosening the criteria for what goes in components might not be such a great idea. Cheers, Jói (*) We actually discussed at one point whether the components layer should live in src/content/components (or possibly src/content_components) rather than src/components. Perhaps things would have been clearer if we had done it that way. OTOH, now that components are reused by Bling, perhaps that wouldn't have made sense. The joys of very-large-scale software projects :) On Tue, May 6, 2014 at 7:30 PM, <brettw@chromium.org> wrote: > On 2014/05/05 22:07:55, Jói wrote: > >> I think you should be careful about making //components a dumping >> ground for anything that doesn't belong elsewhere > > > This is basically what components is. Smallish re-usable bits of code with > clear > dependencies. Digging into this, prefs should definitely be a component, and > it > would have absolutely been put there instead of base had we had it at the > time. > > Whether you decide to move it or not, I think we should proceed with the > understanding that it will eventually be moved to components. > > https://codereview.chromium.org/169323003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Tue, May 6, 2014 at 1:46 PM, Jói Sigurðsson <joi@chromium.org> wrote: >> This is basically what components is. Smallish re-usable bits of code with >> clear dependencies. [...] > > If that is the only criteria for being in components, then > components/README needs to be updated. It also becomes hard to know > why something like src/ipc, src/sql etc. shouldn't also be moved into > components (hence my point earlier that it would give you a grouping > no better than having everything be top-level). I don't see any part of components/README that I disagree with. I think the only thing that distinguishes the components from toplevel directories is a judgement call on size and complexity. Brett To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I moved leveldb_pref_store* to //chrome/browser/prefs as suggested by Mattias, avoiding the components issue for now. Jói and Brett, thank you for your input. Mattias, do you want to give this a look before I put it in the cq? The changes in the move were trivial: * changed the header guard * added base:: a bunch of places in leveldb_pref_store_unittest.cc * added "explicit" to the FileThreadSerializer ctor - (new?) presubmit check caught this.
On 2014/05/06 21:01:03, brettw wrote: > On Tue, May 6, 2014 at 1:46 PM, Jói Sigurðsson <mailto:joi@chromium.org> wrote: > >> This is basically what components is. Smallish re-usable bits of code with > >> clear dependencies. [...] > > > > If that is the only criteria for being in components, then > > components/README needs to be updated. It also becomes hard to know > > why something like src/ipc, src/sql etc. shouldn't also be moved into > > components (hence my point earlier that it would give you a grouping > > no better than having everything be top-level). > > I don't see any part of components/README that I disagree with. I > think the only thing that distinguishes the components from toplevel > directories is a judgement call on size and complexity. > > Brett > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I basically agree with Brett's POV here. I think of top-level directories as "subsystems" and individual components as "features". There is no clear line delineating a "subsystem" from a "feature", but in practice it seems mostly clear which side of the line the code for a given piece of functionality sits on.
On 2014/05/08 00:57:28, dgrogan wrote: > I moved leveldb_pref_store* to //chrome/browser/prefs as suggested by Mattias, > avoiding the components issue for now. Jói and Brett, thank you for your input. > > Mattias, do you want to give this a look before I put it in the cq? > > The changes in the move were trivial: > * changed the header guard > * added base:: a bunch of places in leveldb_pref_store_unittest.cc > * added "explicit" to the FileThreadSerializer ctor - (new?) presubmit check > caught this. What is the intended usage for this class going forward? If it's to be used in code that will be componentized, it would be nice to have it start off in the right place (or move to the right place in the short term).
On 2014/05/09 11:32:58, blundell wrote: > On 2014/05/08 00:57:28, dgrogan wrote: > > I moved leveldb_pref_store* to //chrome/browser/prefs as suggested by Mattias, > > avoiding the components issue for now. Jói and Brett, thank you for your > input. > > > > Mattias, do you want to give this a look before I put it in the cq? > > > > The changes in the move were trivial: > > * changed the header guard > > * added base:: a bunch of places in leveldb_pref_store_unittest.cc > > * added "explicit" to the FileThreadSerializer ctor - (new?) presubmit check > > caught this. > > What is the intended usage for this class going forward? If it's to be used in > code that will be componentized, it would be nice to have it start off in the > right place (or move to the right place in the short term). That's a good point. Apparently IOS also uses PrefService, so putting this class into chrome/browser/prefs is probably not a good long-term plan. However, I suspect IOS already depends on chrome/browser/prefs, so that code needs proper componentization anyways. In that light, I'm fine with just committing this CL to chrome/browser/prefs now and then handling componentization of the code in a separate effort.
The CQ bit was checked by dgrogan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgrogan@chromium.org/169323003/380001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 271602
The CQ bit was checked by dgrogan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgrogan@chromium.org/169323003/420001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was checked by dgrogan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgrogan@chromium.org/169323003/420001
Message was sent while issue was closed.
Change committed as 272671
Message was sent while issue was closed.
On 2014/05/24 01:25:59, I haz the power (commit-bot) wrote: > Change committed as 272671 Reverted in r272700 because it broke asan/lsan bots. Some links to the failures: http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20... http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Te... The revert log includes some backtraces from the leaks.
The CQ bit was checked by dgrogan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dgrogan@chromium.org/169323003/460001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
Message was sent while issue was closed.
Change committed as 273174 |
