Make SDCH dictionaries persistent across browser restart.

net/sdch/sdch_owner.cc

Index: net/sdch/sdch_owner.cc
diff --git a/net/sdch/sdch_owner.cc b/net/sdch/sdch_owner.cc
index b0e440569646d0aa8ebd30d2a9ea05297a766b9c..042af4ae9b28f02cd480949c1a40f1b28bc4574a 100644
--- a/net/sdch/sdch_owner.cc
+++ b/net/sdch/sdch_owner.cc
@@ -5,11 +5,17 @@
 #include "net/sdch/sdch_owner.h"
 
 #include "base/bind.h"
+#include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/prefs/persistent_pref_store.h"
+#include "base/prefs/value_map_pref_store.h"
 #include "base/time/default_clock.h"
+#include "base/values.h"
 #include "net/base/sdch_manager.h"
 #include "net/base/sdch_net_log_params.h"
 
+namespace net {
+
 namespace {
 
 enum DictionaryFate {
@@ -26,8 +32,9 @@ enum DictionaryFate {
   // A successful fetch was refused by the SdchManager.
   DICTIONARY_FATE_FETCH_MANAGER_REFUSED = 4,
 
-  // A dictionary was successfully added.
-  DICTIONARY_FATE_ADDED = 5,
+  // A dictionary was successfully added based on
+  // a Get-Dictionary header in a response.
+  DICTIONARY_FATE_ADD_RESPONSE_TRIGGERED = 5,
 
   // A dictionary was evicted by an incoming dict.
   DICTIONARY_FATE_EVICT_FOR_DICT = 6,
@@ -38,13 +45,42 @@ enum DictionaryFate {
   // A dictionary was evicted on destruction.
   DICTIONARY_FATE_EVICT_FOR_DESTRUCTION = 8,
 
-  DICTIONARY_FATE_MAX = 9
+  // A dictionary was successfully added based on
+  // persistence from a previous browser revision.
+  DICTIONARY_FATE_ADD_PERSISTENCE_TRIGGERED = 9,
+
+  DICTIONARY_FATE_MAX = 10
+};
+
+enum PersistenceFailureReason {
+  // File didn't exist; is being created.
+  PERSISTENCE_FAILURE_REASON_NO_FILE = 1,
+
+  // Error reading in information, but should be able to write.
+  PERSISTENCE_FAILURE_REASON_READ_FAILED = 2,
+
+  // Error leading to abort on attempted persistence.
+  PERSISTENCE_FAILURE_REASON_WRITE_FAILED = 3,
+
+  PERSISTENCE_FAILURE_REASON_MAX = 4
 };
 
+// Dictionaries that haven't been touched in 24 hours may be evicted
+// to make room for new dictionaries.
+const int kFreshnessLifetimeHours = 24;
+
+// Dictionaries that have never been used only stay fresh for one hour.
+const int kNeverUsedFreshnessLifetimeHours = 1;
+
 void RecordDictionaryFate(enum DictionaryFate fate) {
   UMA_HISTOGRAM_ENUMERATION("Sdch3.DictionaryFate", fate, DICTIONARY_FATE_MAX);
 }
 
+void RecordPersistenceFailure(PersistenceFailureReason failure_reason) {
+  UMA_HISTOGRAM_ENUMERATION("Sdch3.PersistenceFailureReason", failure_reason,
+                            PERSISTENCE_FAILURE_REASON_MAX);
+}
+
 void RecordDictionaryEviction(int use_count, DictionaryFate fate) {
   DCHECK(fate == DICTIONARY_FATE_EVICT_FOR_DICT ||
          fate == DICTIONARY_FATE_EVICT_FOR_MEMORY ||
@@ -54,9 +90,154 @@ void RecordDictionaryEviction(int use_count, DictionaryFate fate) {
   RecordDictionaryFate(fate);
 }
 
-}  // namespace
+// Schema specifications and access routines.
+
+// The persistent prefs store is conceptually shared with any other network
+// stack systems that want to persist data over browser restarts, and so
+// use of it must be namespace restricted.
+// Schema:
+//      pref_store_->GetValue(kPreferenceName) -> Dictionary {
+//          'version' -> 1 [int]
+//          'dictionaries' -> Dictionary {
+//              server_hash -> {
+//                  'url' -> URL [string]
+//                  'last_used' -> seconds since unix epoch [double]
+//                  'use_count' -> use count [int]
+//                  'size' -> size [int]
+//          }
+//      }
+const char kPreferenceName[] = "SDCH";
+const char kVersionKey[] = "version";
+const char kDictionariesKey[] = "dictionaries";
+const char kDictionaryUrlKey[] = "url";
+const char kDictionaryLastUsedKey[] = "last_used";
+const char kDictionaryUseCountKey[] = "use_count";
+const char kDictionarySizeKey[] = "size";
+
+const int kVersion = 1;
+
+// This function returns store[kPreferenceName/kDictionariesKey].  The caller
+// is responsible for making sure any needed calls to
+// |store->ReportValueChanged()| occur.
+base::DictionaryValue* GetPersistentStoreDictionaryMap(
+    WriteablePrefStore* store) {
+  base::Value* result = nullptr;
+  bool success = store->GetMutableValue(kPreferenceName, &result);
+  DCHECK(success);
+
+  base::DictionaryValue* preference_dictionary = nullptr;
+  success = result->GetAsDictionary(&preference_dictionary);
+  DCHECK(success);
+  DCHECK(preference_dictionary);
+
+  base::DictionaryValue* dictionary_list_dictionary = nullptr;
+  success = preference_dictionary->GetDictionary(kDictionariesKey,
+                                                 &dictionary_list_dictionary);
+  DCHECK(success);
+  DCHECK(dictionary_list_dictionary);
+
+  return dictionary_list_dictionary;
+}
 
-namespace net {
+// This function initializes a pref store with an empty version of the
+// above schema, removing anything previously in the store under
+// kPreferenceName.
+void InitializePersistentStore(WriteablePrefStore* store) {
+  base::DictionaryValue* empty_store(new base::DictionaryValue);
+  empty_store->SetInteger(kVersionKey, kVersion);
+  empty_store->Set(kDictionariesKey,
+                   make_scoped_ptr(new base::DictionaryValue));
+  store->SetValue(kPreferenceName, empty_store);
+}
+
+// A class to allow iteration over all dictionaries in the pref store, and
+// easy lookup of the information associated with those dictionaries.
+// Note that this is an "Iterator" in the same sense (and for the same
+// reasons) that base::Dictionary::Iterator is an iterator--it allows
+// iterating over all the dictionaries in the preference store, but it
+// does not allow use as an STL iterator because the container it
+// is iterating over does not export begin()/end() methods.
+class DictionaryPreferenceIterator {
+ public:
+  explicit DictionaryPreferenceIterator(WriteablePrefStore* pref_store);
+
+  bool IsAtEnd() const { return dictionary_iterator_.IsAtEnd(); }

[mmenke, 2015/02/06 22:29:41]

Seems weird to inline this, but not the other methods use capital camel case -
suggest de-inlining it.

[Elly Fong-Jones, 2015/02/11 21:23:51]

Done.

+  void Advance();
+
+  const std::string& server_hash() const { return server_hash_; }
+  GURL url() const { return url_; }

[mmenke, 2015/02/06 22:29:41]

const GURL&

[Elly Fong-Jones, 2015/02/11 21:23:51]

Done.

+  base::Time last_used() const { return last_used_; }
+  int use_count() const { return use_count_; }
+  int size() const { return size_; }
+
+ private:
+  void LoadDictionaryOrDie();
+
+  std::string server_hash_;
+  GURL url_;
+  base::Time last_used_;
+  int use_count_;
+  int size_;
+
+  base::DictionaryValue::Iterator dictionary_iterator_;
+};
+
+DictionaryPreferenceIterator::DictionaryPreferenceIterator(
+    WriteablePrefStore* pref_store)
+    : dictionary_iterator_(*GetPersistentStoreDictionaryMap(pref_store)) {
+  if (!IsAtEnd())
+    LoadDictionaryOrDie();
+}
+
+void DictionaryPreferenceIterator::Advance() {
+  dictionary_iterator_.Advance();
+  if (!IsAtEnd())
+    LoadDictionaryOrDie();
+}
+
+void DictionaryPreferenceIterator::LoadDictionaryOrDie() {
+  double last_used_seconds_from_epoch;
+  const base::DictionaryValue* dict = nullptr;
+  bool success =
+      dictionary_iterator_.value().GetAsDictionary(&dict);
+  DCHECK(success);
+
+  server_hash_ = dictionary_iterator_.key();
+
+  std::string url_spec;
+  success = dict->GetString(kDictionaryUrlKey, &url_spec);
+  DCHECK(success);
+  url_ = GURL(url_spec);
+
+  success = dict->GetDouble(kDictionaryLastUsedKey,
+                                     &last_used_seconds_from_epoch);

[Bernhard Bauer, 2015/02/06 20:12:18]

This seems to be aligned weirdly.

[Elly Fong-Jones, 2015/02/11 21:23:51]

Done.

+  DCHECK(success);
+  last_used_ = base::Time::FromDoubleT(last_used_seconds_from_epoch);
+
+  success = dict->GetInteger(kDictionaryUseCountKey, &use_count_);
+  DCHECK(success);
+
+  success = dict->GetInteger(kDictionarySizeKey, &size_);
+  DCHECK(success);
+}
+
+// Triggers a ReportValueChanged() on the specified WriteablePrefStore
+// when the object goes out of scope.
+class ScopedPrefNotifier {
+ public:
+  // Caller must guarantee lifetime of |*pref_store| exceeds the
+  // lifetime of this object.
+  ScopedPrefNotifier(WriteablePrefStore* pref_store)
+      : pref_store_(pref_store) {}
+  ~ScopedPrefNotifier() { pref_store_->ReportValueChanged(kPreferenceName); }
+
+ private:
+  WriteablePrefStore* pref_store_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedPrefNotifier);
+};
+
+}  // namespace
 
 // Adjust SDCH limits downwards for mobile.
 #if defined(OS_ANDROID) || defined(OS_IOS)
@@ -76,16 +257,14 @@ const size_t SdchOwner::kMinSpaceForDictionaryFetch = 50 * 1000;
 SdchOwner::SdchOwner(net::SdchManager* sdch_manager,
                      net::URLRequestContext* context)

[mmenke, 2015/02/06 22:29:41]

While you're here, these net::'s aren't needed.

[Elly Fong-Jones, 2015/02/11 21:23:51]

Done.

     : manager_(sdch_manager),
-      fetcher_(context,
-               base::Bind(&SdchOwner::OnDictionaryFetched,
-                          // Because |fetcher_| is owned by SdchOwner, the
-                          // SdchOwner object will be available for the lifetime
-                          // of |fetcher_|.
-                          base::Unretained(this))),
+      fetcher_(context),
       total_dictionary_bytes_(0),
       clock_(new base::DefaultClock),
       max_total_dictionary_size_(kMaxTotalDictionarySize),
       min_space_for_dictionary_fetch_(kMinSpaceForDictionaryFetch),
+      in_memory_pref_store_(new ValueMapPrefStore()),
+      external_pref_store_(nullptr),
+      pref_store_(in_memory_pref_store_.get()),
       memory_pressure_listener_(
           base::Bind(&SdchOwner::OnMemoryPressure,
                      // Because |memory_pressure_listener_| is owned by
@@ -93,15 +272,30 @@ SdchOwner::SdchOwner(net::SdchManager* sdch_manager,
                      // for the lifetime of |memory_pressure_listener_|.
                      base::Unretained(this))) {
   manager_->AddObserver(this);
+  InitializePersistentStore(pref_store_);

[mmenke, 2015/02/06 22:29:42]

"EnablePersistentStorage" is used to mean the on-disk store, so it seems weird
to be calling "InitializePersistentStore" on the in-memory store.  Suggest
renaming it to something like "InitializePreferenceStore"

[Elly Fong-Jones, 2015/02/11 21:23:51]

Done.

 }
 
 SdchOwner::~SdchOwner() {
-  for (auto it = local_dictionary_info_.begin();
-       it != local_dictionary_info_.end(); ++it) {
-    RecordDictionaryEviction(it->second.use_count,
+  for (DictionaryPreferenceIterator it(pref_store_); !it.IsAtEnd();
+       it.Advance()) {
+    RecordDictionaryEviction(it.use_count(),
                              DICTIONARY_FATE_EVICT_FOR_DESTRUCTION);

[mmenke, 2015/02/06 22:29:41]

Hrm...Not sure "evict" is still accurate here, at least if we're using an
on-disk store

[Elly Fong-Jones, 2015/02/11 21:23:51]

Done.

   }
   manager_->RemoveObserver(this);
+
+  // This object only observes the external store during loading,
+  // i.e. before it's made the default preferences store.
+  if (external_pref_store_)
+    external_pref_store_->RemoveObserver(this);
+}
+
+void SdchOwner::EnablePersistentStorage(PersistentPrefStore* pref_store) {
+  DCHECK(!external_pref_store_);
+  external_pref_store_ = pref_store;
+  external_pref_store_->AddObserver(this);
+
+  if (external_pref_store_->IsInitializationComplete())
+    OnInitializationCompleted(true);
 }
 
 void SdchOwner::SetMaxTotalDictionarySize(size_t max_total_dictionary_size) {
@@ -113,7 +307,9 @@ void SdchOwner::SetMinSpaceForDictionaryFetch(
   min_space_for_dictionary_fetch_ = min_space_for_dictionary_fetch;
 }
 
-void SdchOwner::OnDictionaryFetched(const std::string& dictionary_text,
+void SdchOwner::OnDictionaryFetched(base::Time last_used,
+                                    int use_count,
+                                    const std::string& dictionary_text,
                                     const GURL& dictionary_url,
                                     const net::BoundNetLog& net_log) {
   struct DictionaryItem {
@@ -138,16 +334,23 @@ void SdchOwner::OnDictionaryFetched(const std::string& dictionary_text,
     }
   };
 
+  // Figure out if there is space for the incoming dictionary; evict
+  // stale dictionaries if needed to make space.
+
   std::vector<DictionaryItem> stale_dictionary_list;
   size_t recoverable_bytes = 0;
-  base::Time stale_boundary(clock_->Now() - base::TimeDelta::FromDays(1));
-  for (auto used_it = local_dictionary_info_.begin();
-       used_it != local_dictionary_info_.end(); ++used_it) {
-    if (used_it->second.last_used < stale_boundary) {
-      stale_dictionary_list.push_back(
-          DictionaryItem(used_it->second.last_used, used_it->first,
-                         used_it->second.use_count, used_it->second.size));
-      recoverable_bytes += used_it->second.size;
+  base::Time now(clock_->Now());
+  base::Time stale_boundary(
+      now - base::TimeDelta::FromHours(kFreshnessLifetimeHours));
+  base::Time never_used_stale_boundary(

[mmenke, 2015/02/06 22:29:42]

Think these two "boundary" times are worth a short comment before each.

[Elly Fong-Jones, 2015/02/11 21:23:51]

Done.

+      now - base::TimeDelta::FromHours(kNeverUsedFreshnessLifetimeHours));
+  for (DictionaryPreferenceIterator it(pref_store_); !it.IsAtEnd();
+       it.Advance()) {
+    if (it.last_used() < stale_boundary ||
+        (it.use_count() == 0 && it.last_used() < never_used_stale_boundary)) {
+      stale_dictionary_list.push_back(DictionaryItem(
+          it.last_used(), it.server_hash(), it.use_count(), it.size()));
+      recoverable_bytes += it.size();
     }
   }

[mmenke, 2015/02/06 22:29:42]

Probably a bit beyond the scope of this cl, but should we even bother doing this
if total_dictionary_bytes_ + dictionary_text.size() >
max_total_dictionary_size_?

I guess we don't keep too many dictionaries around, but this takes O(n^2) time
during startup.

[Elly Fong-Jones, 2015/02/11 21:23:51]

It does, but I think n is small. I am going to not worry about this for now.

 
@@ -161,21 +364,9 @@ void SdchOwner::OnDictionaryFetched(const std::string& dictionary_text,
     return;
   }
 
-  // Evict from oldest to youngest until we have space.
-  std::sort(stale_dictionary_list.begin(), stale_dictionary_list.end());
-  size_t avail_bytes = max_total_dictionary_size_ - total_dictionary_bytes_;
-  auto stale_it = stale_dictionary_list.begin();
-  while (avail_bytes < dictionary_text.size() &&
-         stale_it != stale_dictionary_list.end()) {
-    manager_->RemoveSdchDictionary(stale_it->server_hash);
-    RecordDictionaryEviction(stale_it->use_count,
-                             DICTIONARY_FATE_EVICT_FOR_DICT);
-    local_dictionary_info_.erase(stale_it->server_hash);
-    avail_bytes += stale_it->dictionary_size;
-    ++stale_it;
-  }
-  DCHECK(avail_bytes >= dictionary_text.size());
-
+  // Add the new dictionary.  This is done before removing the stale
+  // dictionaries so that no state change will occur if dictionary addition
+  // fails.
   std::string server_hash;
   net::SdchProblemCode rv = manager_->AddSdchDictionary(
       dictionary_text, dictionary_url, &server_hash);
@@ -188,25 +379,95 @@ void SdchOwner::OnDictionaryFetched(const std::string& dictionary_text,
     return;
   }
 
-  RecordDictionaryFate(DICTIONARY_FATE_ADDED);
+  base::DictionaryValue* pref_dictionary_map =
+      GetPersistentStoreDictionaryMap(pref_store_);
+  ScopedPrefNotifier scoped_pref_notifier(pref_store_);
+
+  // Remove the old dictionaries.
+  std::sort(stale_dictionary_list.begin(), stale_dictionary_list.end());
+  size_t avail_bytes = max_total_dictionary_size_ - total_dictionary_bytes_;
+  auto stale_it = stale_dictionary_list.begin();
+  while (avail_bytes < dictionary_text.size() &&
+         stale_it != stale_dictionary_list.end()) {
+    manager_->RemoveSdchDictionary(stale_it->server_hash);
+
+    DCHECK(pref_dictionary_map->HasKey(stale_it->server_hash));
+    bool success = pref_dictionary_map->RemoveWithoutPathExpansion(
+        stale_it->server_hash, nullptr);
+    DCHECK(success);
+
+    avail_bytes += stale_it->dictionary_size;
+
+    RecordDictionaryEviction(stale_it->use_count,
+                             DICTIONARY_FATE_EVICT_FOR_DICT);
+
+    ++stale_it;
+  }
+  DCHECK_GE(avail_bytes, dictionary_text.size());
+
+  RecordDictionaryFate(
+      // Distinguish between loads triggered by network responses and
+      // loads triggered by persistence.
+      last_used.is_null() ? DICTIONARY_FATE_ADD_RESPONSE_TRIGGERED
+                          : DICTIONARY_FATE_ADD_PERSISTENCE_TRIGGERED);
+
+  // If a dictionary has never been used, its dictionary addition time
+  // is recorded as its last used time.  Never used dictionaries are treated
+  // specially in the freshness logic.
+  if (last_used.is_null())
+    last_used = clock_->Now();
 
-  DCHECK(local_dictionary_info_.end() ==
-         local_dictionary_info_.find(server_hash));
   total_dictionary_bytes_ += dictionary_text.size();
-  local_dictionary_info_[server_hash] = DictionaryInfo(
-      // Set the time last used to something to avoid thrashing, but not recent,
-      // to avoid taking too much time/space with useless dictionaries/one-off
-      // visits to web sites.
-      clock_->Now() - base::TimeDelta::FromHours(23), dictionary_text.size());
+
+  // Record the addition in the pref store.
+  scoped_ptr<base::DictionaryValue> dictionary_description(
+      new base::DictionaryValue());
+  dictionary_description->SetString(kDictionaryUrlKey, dictionary_url.spec());
+  dictionary_description->SetDouble(kDictionaryLastUsedKey,
+                                    last_used.ToDoubleT());
+  dictionary_description->SetInteger(kDictionaryUseCountKey, use_count);
+  dictionary_description->SetInteger(kDictionarySizeKey,
+                                     dictionary_text.size());
+  pref_dictionary_map->Set(server_hash, dictionary_description.Pass());
 }
 
 void SdchOwner::OnDictionaryUsed(SdchManager* manager,
                                  const std::string& server_hash) {
-  auto it = local_dictionary_info_.find(server_hash);
-  DCHECK(local_dictionary_info_.end() != it);
-
-  it->second.last_used = clock_->Now();
-  it->second.use_count++;
+  base::Time now(clock_->Now());
+  base::DictionaryValue* pref_dictionary_map =
+      GetPersistentStoreDictionaryMap(pref_store_);
+  ScopedPrefNotifier scoped_pref_notifier(pref_store_);
+
+  base::Value* value = nullptr;
+  bool success = pref_dictionary_map->Get(server_hash, &value);
+  // TODO(rdsmith): Is the behavior of the SdchManager certain enough for
+  // the following DCHECKs?

[mmenke, 2015/02/06 22:29:41]

This TODOs seem rather concerned.  If they're not, we're introducing a crasher. 
Figure that out before landing?

[Elly Fong-Jones, 2015/02/11 21:23:51]

I admit that I have no idea what this TODO is referring to. The pref dictionary
map is internal to this class, so its consistency is our problem. I would guess
that it refers to the possibility of SdchManager sending us this event for a
dictionary that SdchOwner doesn't know about, which I think should be a
DCHECK-worthy condition. In any case, this TODO doesn't need to be here; either
SdchManager is buggy and this DCHECK will catch that, or it's not and it won't.
Removing the DCHECK would just introduce a hard crash here when we try to use
the dictionary.

I have removed the TODO.

[Randy Smith (Not in Mondays), 2015/02/16 01:10:04]

+1.  This was in the way of a note to myself to just confirm what you've said
above, which I believe is all true.  The DCHECK is asserting that the manager
and the owner are in sync with regard to what dictionaries are currently active,
and if it trips, that's a bug in the code (either manager or owner).

+  DCHECK(success);
+  base::DictionaryValue* specific_dictionary_map = nullptr;
+  success = value->GetAsDictionary(&specific_dictionary_map);
+  DCHECK(success);
+
+  double last_used_time = 0.0;
+  success = specific_dictionary_map->GetDouble(kDictionaryLastUsedKey,
+                                               &last_used_time);

[mmenke, 2015/02/06 22:29:42]

nit:  Should use consistent variable names here and in the similar OrDie method.

[Elly Fong-Jones, 2015/02/11 21:23:51]

Done.

+  DCHECK(success);
+  int use_count = 0;
+  success =
+      specific_dictionary_map->GetInteger(kDictionaryUseCountKey, &use_count);
+  DCHECK(success);
+
+  base::TimeDelta time_since_last_used(now -
+                                       base::Time::FromDoubleT(last_used_time));
+
+  // TODO(rdsmith): Distinguish between "Never used" and "Actually not
+  // touched for 48 hours".
+  UMA_HISTOGRAM_CUSTOM_TIMES(
+      "Sdch3.UsageInterval",
+      use_count ? time_since_last_used : base::TimeDelta::FromHours(48),
+      base::TimeDelta(), base::TimeDelta::FromHours(48), 50);
+
+  specific_dictionary_map->SetDouble(kDictionaryLastUsedKey, now.ToDoubleT());
+  specific_dictionary_map->SetInteger(kDictionaryUseCountKey, use_count + 1);
 }
 
 void SdchOwner::OnGetDictionary(net::SdchManager* manager,
@@ -214,10 +475,10 @@ void SdchOwner::OnGetDictionary(net::SdchManager* manager,
                                 const GURL& dictionary_url) {
   base::Time stale_boundary(clock_->Now() - base::TimeDelta::FromDays(1));
   size_t avail_bytes = 0;
-  for (auto it = local_dictionary_info_.begin();
-       it != local_dictionary_info_.end(); ++it) {
-    if (it->second.last_used < stale_boundary)
-      avail_bytes += it->second.size;
+  for (DictionaryPreferenceIterator it(pref_store_); !it.IsAtEnd();
+       it.Advance()) {
+    if (it.last_used() < stale_boundary)
+      avail_bytes += it.size();
   }
 
   // Don't initiate the fetch if we wouldn't be able to store any
@@ -234,13 +495,100 @@ void SdchOwner::OnGetDictionary(net::SdchManager* manager,
     return;
   }
 
-  fetcher_.Schedule(dictionary_url);
+  fetcher_.Schedule(dictionary_url,
+                    base::Bind(&SdchOwner::OnDictionaryFetched,
+                               // SdchOwner will outlive its member variables.
+                               base::Unretained(this), base::Time(), 0));
 }
 
 void SdchOwner::OnClearDictionaries(net::SdchManager* manager) {
   total_dictionary_bytes_ = 0;
-  local_dictionary_info_.clear();
   fetcher_.Cancel();
+
+  InitializePersistentStore(pref_store_);
+}
+
+void SdchOwner::OnPrefValueChanged(const std::string& key) {
+}
+
+void SdchOwner::OnInitializationCompleted(bool succeeded) {
+  // Errors on load are self-correcting; if dictionaries were not
+  // persisted from the last instance of the browser, they will be
+  // faulted in by user action over time.  However, if a load error
+  // means that the dictionary information won't be able to be persisted,
+  // the in memory pref store is left in place.
+  switch (external_pref_store_->GetReadError()) {
+    case PersistentPrefStore::PREF_READ_ERROR_NONE:
+      if (succeeded)
+        break;

[mmenke, 2015/02/06 22:29:41]

Wonder if we should record something here - otherwise, not sure how meaningful
the error histograms are

[Elly Fong-Jones, 2015/02/11 21:23:51]

They still tell us the relative frequency of the different kinds of error.

[Randy Smith (Not in Mondays), 2015/02/16 01:10:04]

+1, sorta to each point :-}.  I think these histograms are useful as written as
they tell us the relative error frequencies, and that information will be easier
to get visually if we don't get the success case.  It doesn't give us the total
number of times we successfully loaded from disk (to compare against the error
cases) and that is a lack--it doesn't allow us to answer the basic question of
"How often is this thing working?".  If we want that, we could probably get that
number from the total number of browser restarts, which I gotta believe is
available somewhere in UMA.  But that's not very convenient.

I guess that ends me up with leaning towards including the success case.  But
I'm willing to go along with whatever comes out of the conversation between the
two of you.

+
+    case PersistentPrefStore::PREF_READ_ERROR_NO_FILE:
+      // First time reading; the file will be created.
+      if (succeeded) {
+        RecordPersistenceFailure(PERSISTENCE_FAILURE_REASON_NO_FILE);
+        break;
+      }
+
+    case PersistentPrefStore::PREF_READ_ERROR_JSON_PARSE:
+    case PersistentPrefStore::PREF_READ_ERROR_JSON_TYPE:
+    case PersistentPrefStore::PREF_READ_ERROR_FILE_OTHER:
+    case PersistentPrefStore::PREF_READ_ERROR_FILE_LOCKED:
+    case PersistentPrefStore::PREF_READ_ERROR_JSON_REPEAT:
+    case PersistentPrefStore::PREF_READ_ERROR_LEVELDB_IO:
+    case PersistentPrefStore::PREF_READ_ERROR_LEVELDB_CORRUPTION_READ_ONLY:
+    case PersistentPrefStore::PREF_READ_ERROR_LEVELDB_CORRUPTION:
+      // If the load was marked as succeeding (meaning the directory
+      // was present), even if the file wasn't read, it can be written.
+      if (succeeded) {
+        RecordPersistenceFailure(PERSISTENCE_FAILURE_REASON_READ_FAILED);
+        break;
+      }

[mmenke, 2015/02/06 22:29:41]

optional:  I'm not a big fan of fallthrough behavior.  Think it's easy to miss
when reading code.

I'd suggest something like:

if (!succeeded) {
  DCHECK_NE(error, PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE);
  DCHECK_NE(...);
  RecordPersistenceFailure(PERSISTENCE_FAILURE_REASON_WRITE_FAILED);
  return;
}

Then have a switch for recoverable errors.  One difference is we'd be handling
success and PREF_READ_ERROR_ACCESS_DENIED / PREF_READ_ERROR_FILE_NOT_SPECIFIED
differently (Can we see success on those errors?)

[Elly Fong-Jones, 2015/02/11 21:23:51]

Done.

+
+    case PersistentPrefStore::PREF_READ_ERROR_ACCESS_DENIED:
+    case PersistentPrefStore::PREF_READ_ERROR_FILE_NOT_SPECIFIED:
+      // Unrecoverable failure; we default to the internal pref store.
+      external_pref_store_->RemoveObserver(this);
+      external_pref_store_ = nullptr;
+      RecordPersistenceFailure(PERSISTENCE_FAILURE_REASON_WRITE_FAILED);
+      return;
+
+    case PersistentPrefStore::PREF_READ_ERROR_ASYNCHRONOUS_TASK_INCOMPLETE:
+    case PersistentPrefStore::PREF_READ_ERROR_MAX_ENUM:
+      // Shouldn't ever happen.
+      NOTREACHED();
+      break;
+  }
+
+  // TODO(rdsmith): Implement versioning.
+
+  // Load in what was stored before chrome exited previously.
+  const base::Value* sdch_persistence_value = nullptr;
+  const base::DictionaryValue* sdch_persistence_dictionary = nullptr;
+
+  // The GetPersistentStore() routine above assumes data formatted
+  // according to the schema described at the top of this file.  Since
+  // this data comes from disk, to avoid disk corruption resulting in
+  // persistent chrome errors this code avoids those assupmtions.
+  if (external_pref_store_->GetValue(kPreferenceName,
+                                     &sdch_persistence_value) &&
+      sdch_persistence_value->GetAsDictionary(&sdch_persistence_dictionary)) {
+    SchedulePersistedDictionaryLoads(*sdch_persistence_dictionary);
+  }
+
+  // Reset the persistent store and update it with the accumulated
+  // information from the local store.
+  InitializePersistentStore(external_pref_store_);
+
+  ScopedPrefNotifier scoped_pref_notifier(external_pref_store_);
+  GetPersistentStoreDictionaryMap(external_pref_store_)
+      ->Swap(GetPersistentStoreDictionaryMap(in_memory_pref_store_.get()));
+
+  // This object can stop waiting on (i.e. observing) the external preference
+  // store and switch over to using it as the primary preference store.
+  pref_store_ = external_pref_store_;
+  external_pref_store_->RemoveObserver(this);

[mmenke, 2015/02/06 22:29:42]

Should we do this on failure, too?  (And maybe null out the external pref store
in that case, too?)

[Elly Fong-Jones, 2015/02/11 21:23:51]

We do do this on the failure case, I think?

[mmenke, 2015/02/12 16:51:32]

Oops...You're right, we are.

+  external_pref_store_ = nullptr;
+  in_memory_pref_store_ = nullptr;
 }
 
 void SdchOwner::SetClockForTesting(scoped_ptr<base::Clock> clock) {
@@ -251,10 +599,9 @@ void SdchOwner::OnMemoryPressure(
     base::MemoryPressureListener::MemoryPressureLevel level) {
   DCHECK_NE(base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_NONE, level);
 
-  for (auto it = local_dictionary_info_.begin();
-       it != local_dictionary_info_.end(); ++it) {
-    RecordDictionaryEviction(it->second.use_count,
-                             DICTIONARY_FATE_EVICT_FOR_MEMORY);
+  for (DictionaryPreferenceIterator it(pref_store_); !it.IsAtEnd();
+       it.Advance()) {
+    RecordDictionaryEviction(it.use_count(), DICTIONARY_FATE_EVICT_FOR_MEMORY);
   }
 
   // TODO(rdsmith): Make a distinction between moderate and critical
@@ -262,4 +609,52 @@ void SdchOwner::OnMemoryPressure(
   manager_->ClearData();
 }
 
+bool SdchOwner::SchedulePersistedDictionaryLoads(
+    const base::DictionaryValue& persisted_info) {
+  // Any schema error will result in dropping the persisted info.
+  int version = 0;
+  if (!persisted_info.GetInteger(kVersionKey, &version))
+    return false;
+
+  // Any version mismatch will result in dropping the persisted info;
+  // it will be faulted in at small performance cost as URLs using
+  // dictionaries for encoding are visited.
+  if (version != kVersion)
+    return false;
+
+  const base::DictionaryValue* dictionary_set = nullptr;
+  if (!persisted_info.GetDictionary(kDictionariesKey, &dictionary_set))
+    return false;
+
+  // Any formatting error will result in skipping that particular
+  // dictionary.
+  for (base::DictionaryValue::Iterator dict_it(*dictionary_set);
+       !dict_it.IsAtEnd(); dict_it.Advance()) {
+    const base::DictionaryValue* dict_info = nullptr;
+    if (!dict_it.value().GetAsDictionary(&dict_info))
+      continue;
+
+    std::string url_string;
+    if (!dict_info->GetString(kDictionaryUrlKey, &url_string))
+      continue;
+    GURL dict_url(url_string);
+
+    double last_used;
+    if (!dict_info->GetDouble(kDictionaryLastUsedKey, &last_used))
+      continue;
+
+    int use_count;
+    if (!dict_info->GetInteger(kDictionaryUseCountKey, &use_count))
+      continue;
+
+    fetcher_.ScheduleReload(
+        dict_url, base::Bind(&SdchOwner::OnDictionaryFetched,
+                             // SdchOwner will outlive its member variables.
+                             base::Unretained(this),
+                             base::Time::FromDoubleT(last_used), use_count));
+  }
+
+  return true;
+}
+
 }  // namespace net