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..a1672e7ebfb8f03d73d995a5d0eb0e5fa0a59acf 100644
--- a/net/sdch/sdch_owner.cc
+++ b/net/sdch/sdch_owner.cc
@@ -6,10 +6,18 @@
 
 #include "base/bind.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"
 
+// Notes on the persistent prefs store.

[Bernhard Bauer, 2015/01/29 17:42:23]

Yes? :)

[Randy Smith (Not in Mondays), 2015/01/30 20:11:10]

Sorry, moved the notes that used to be here to later in the file :-}.  Removed.

+//
+
+namespace net {
+
 namespace {
 
 enum DictionaryFate {
@@ -41,6 +49,29 @@ enum DictionaryFate {
   DICTIONARY_FATE_MAX = 9
 };
 
+struct FetchData : public SdchDictionaryFetcher::Data {
+  ~FetchData() override {}
+
+  base::Time last_used;
+  int use_count;
+
+  FetchData() : use_count(0) {}
+  FetchData(const base::Time& last_used, int use_count)
+      : last_used(last_used), use_count(use_count) {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FetchData);
+};
+
+// Dictionaries that haven't been touched in 24 hours may be evicted
+// to make room for new dictionaries.
+const static int kHoursFreshSinceLastUsed = 24;
+
+// Newly added dictionaries are consider to have been last touched 23 hours
+// ago, so they aren't kept around long if they're a one-off, but dictionary
+// loading doesn't thrash if many dictionaries are seen in quick succession.
+const static int kInitialHoursSinceLastUsed = 23;
+
 void RecordDictionaryFate(enum DictionaryFate fate) {
   UMA_HISTOGRAM_ENUMERATION("Sdch3.DictionaryFate", fate, DICTIONARY_FATE_MAX);
 }
@@ -54,9 +85,69 @@ 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]
+//          }
+//      }
+const static char kPreferenceName[] = "SDCH";
+const static char kVersionName[] = "version";
+const static char kDictionariesName[] = "dictionaries";
+const static char kDictionaryUrlName[] = "url";
+const static char kDictionaryLastUsedName[] = "last_used";
+const static char kDictionaryUseCountName[] = "use_count";
+
+const static int kVersion = 1;

[mmenke, 2015/01/30 20:18:41]

statics not needed, since we're in an anonymous namespace

[Randy Smith (Not in Mondays), 2015/01/31 02:53:29]

Done.

+
+// This function returns store[kPreferenceName]
+base::DictionaryValue* GetPersistentStoreMap(WriteablePrefStore* store) {
+  base::Value* result = nullptr;
+  bool success = store->GetMutableValue(kPreferenceName, &result);
+  DCHECK(success);
+
+  base::DictionaryValue* dictionary_result = nullptr;
+  success = result->GetAsDictionary(&dictionary_result);
+  DCHECK(success);
+  DCHECK(dictionary_result);
+
+  return dictionary_result;
+}
 
-namespace net {
+// This function returns store[kPreferenceName/kDictionaries]
+base::DictionaryValue* GetPersistentStoreDictionaryMap(
+    WriteablePrefStore* store) {
+  base::DictionaryValue* dictionary_result = nullptr;
+  bool success = GetPersistentStoreMap(store)
+                     ->GetDictionary(kDictionariesName, &dictionary_result);
+  DCHECK(success);
+  DCHECK(dictionary_result);
+
+  return dictionary_result;
+}
+
+// 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(kVersionName, kVersion);
+  empty_store->Set(kDictionariesName, scoped_ptr<base::DictionaryValue>(
+                                          new base::DictionaryValue).Pass());

[Bernhard Bauer, 2015/01/29 17:42:23]

This is aligned weirdly. The four-space indent is really only with respect to
the indentation on the previous line, not with respect to aligned parameters
(*either* you align parameters, *or* you indent by four spaces). As an
alternative, if you clang-format this, I will accept whatever the output is :)

Also, you can use make_scoped_ptr, which is a bit shorter.

Also also, if you create a brand new scoped_ptr, you don't need .Pass (you only
need it for an existing scoped_ptr, where you want to *pass* ownership).

[Randy Smith (Not in Mondays), 2015/01/30 20:11:10]

Huh; I thought I had (net/ now requires it at a warning level for uploads).   I
will (and upload) before replying to your comments.  
Right, forgot about that.  Thank you.  
Ah, I didn't know that!  That will make some code cleaner.  Thank you.

[mmenke, 2015/01/30 20:18:41]

I don't think you need the Pass() here, when you create a new dictionary.

[Randy Smith (Not in Mondays), 2015/01/31 02:53:29]

Done.

+  store->RemoveValue(kPreferenceName);

[Bernhard Bauer, 2015/01/29 17:42:23]

This isn't necessary if you set a value right afterwards.

[Randy Smith (Not in Mondays), 2015/01/30 20:11:10]

Ah, cool.  The comments in WriteablePrefStore weren't clear as to whether set
replaced or not (to me, at least).  Done.

+  store->SetValue(kPreferenceName, empty_store);
+}
+
+}  // namespace
 
 // Adjust SDCH limits downwards for mobile.
 #if defined(OS_ANDROID) || defined(OS_IOS)
@@ -86,6 +177,9 @@ SdchOwner::SdchOwner(net::SdchManager* sdch_manager,
       clock_(new base::DefaultClock),
       max_total_dictionary_size_(kMaxTotalDictionarySize),
       min_space_for_dictionary_fetch_(kMinSpaceForDictionaryFetch),
+      temporary_pref_store_(new ValueMapPrefStore()),
+      persistent_pref_store_(nullptr),
+      pref_store_(temporary_pref_store_.get()),
       memory_pressure_listener_(
           base::Bind(&SdchOwner::OnMemoryPressure,
                      // Because |memory_pressure_listener_| is owned by
@@ -93,6 +187,7 @@ SdchOwner::SdchOwner(net::SdchManager* sdch_manager,
                      // for the lifetime of |memory_pressure_listener_|.
                      base::Unretained(this))) {
   manager_->AddObserver(this);
+  InitializePersistentStore(pref_store_);
 }
 
 SdchOwner::~SdchOwner() {
@@ -102,6 +197,17 @@ SdchOwner::~SdchOwner() {
                              DICTIONARY_FATE_EVICT_FOR_DESTRUCTION);
   }
   manager_->RemoveObserver(this);
+  if (pref_store_)
+    pref_store_->RemoveObserver(this);
+}
+
+void SdchOwner::EnablePersistentStorage(PersistentPrefStore* pref_store) {
+  DCHECK(!persistent_pref_store_);
+  persistent_pref_store_ = pref_store;
+  persistent_pref_store_->AddObserver(this);
+
+  if (persistent_pref_store_->IsInitializationComplete())
+    OnInitializationCompleted(true);
 }
 
 void SdchOwner::SetMaxTotalDictionarySize(size_t max_total_dictionary_size) {
@@ -113,9 +219,11 @@ void SdchOwner::SetMinSpaceForDictionaryFetch(
   min_space_for_dictionary_fetch_ = min_space_for_dictionary_fetch;
 }
 
-void SdchOwner::OnDictionaryFetched(const std::string& dictionary_text,
-                                    const GURL& dictionary_url,
-                                    const net::BoundNetLog& net_log) {
+void SdchOwner::OnDictionaryFetched(
+    const std::string& dictionary_text,
+    const GURL& dictionary_url,
+    scoped_ptr<SdchDictionaryFetcher::Data> extra_data,

[Bernhard Bauer, 2015/01/29 17:42:23]

Where is this used?

[Randy Smith (Not in Mondays), 2015/01/30 20:11:10]

This has to do with wanting to get early comments on the direction of the CL
before finishing it.  The code that I intended to add in the future is simple
enough, though, so I just added it; if the dictionary was persisted, we also
persist the usage info.

+    const net::BoundNetLog& net_log) {
   struct DictionaryItem {
     base::Time last_used;
     std::string server_hash;
@@ -138,9 +246,13 @@ void SdchOwner::OnDictionaryFetched(const std::string& dictionary_text,
     }
   };
 
+  // Figure out if we have space for the incoming dictionary; evict
+  // stale dictionaries if needed to make space.

[mmenke, 2015/01/30 20:18:41]

Don't use "we" in comments.

[Randy Smith (Not in Mondays), 2015/01/31 02:53:29]

Slowly breaking myself of the habit :-{; done.

+
   std::vector<DictionaryItem> stale_dictionary_list;
   size_t recoverable_bytes = 0;
-  base::Time stale_boundary(clock_->Now() - base::TimeDelta::FromDays(1));
+  base::Time stale_boundary(
+      clock_->Now() - base::TimeDelta::FromHours(kHoursFreshSinceLastUsed));
   for (auto used_it = local_dictionary_info_.begin();
        used_it != local_dictionary_info_.end(); ++used_it) {
     if (used_it->second.last_used < stale_boundary) {
@@ -161,21 +273,32 @@ void SdchOwner::OnDictionaryFetched(const std::string& dictionary_text,
     return;
   }
 
-  // Evict from oldest to youngest until we have space.
+  base::DictionaryValue* pref_dictionary_map =
+      GetPersistentStoreDictionaryMap(pref_store_);
+
   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);
+
+    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(avail_bytes >= dictionary_text.size());
 
+  // Add the new dictionary.
   std::string server_hash;
   net::SdchProblemCode rv = manager_->AddSdchDictionary(
       dictionary_text, dictionary_url, &server_hash);
@@ -190,14 +313,25 @@ void SdchOwner::OnDictionaryFetched(const std::string& dictionary_text,
 
   RecordDictionaryFate(DICTIONARY_FATE_ADDED);
 
+  // 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.
+  base::Time last_used =
+      clock_->Now() - base::TimeDelta::FromHours(kInitialHoursSinceLastUsed);
   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());
+  local_dictionary_info_[server_hash] =
+      DictionaryInfo(last_used, dictionary_text.size());
+
+  // Record the addition in the pref store.
+  scoped_ptr<base::DictionaryValue> dictionary_description(
+      new base::DictionaryValue());
+  dictionary_description->SetString(kDictionaryUrlName, dictionary_url.spec());
+  dictionary_description->SetDouble(kDictionaryLastUsedName,
+                                    last_used.ToDoubleT());
+  dictionary_description->SetInteger(kDictionaryUseCountName, 0);
+  pref_dictionary_map->Set(server_hash, dictionary_description.Pass());
 }
 
 void SdchOwner::OnDictionaryUsed(SdchManager* manager,
@@ -207,6 +341,14 @@ void SdchOwner::OnDictionaryUsed(SdchManager* manager,
 
   it->second.last_used = clock_->Now();
   it->second.use_count++;
+
+  base::DictionaryValue* pref_dictionary_map =
+      GetPersistentStoreDictionaryMap(pref_store_);
+
+  pref_dictionary_map->SetInteger(server_hash + "/use_count",
+                                  it->second.use_count);
+  pref_dictionary_map->SetDouble(server_hash + "/last_used",
+                                 it->second.last_used.ToDoubleT());
 }
 
 void SdchOwner::OnGetDictionary(net::SdchManager* manager,
@@ -234,13 +376,51 @@ void SdchOwner::OnGetDictionary(net::SdchManager* manager,
     return;
   }
 
-  fetcher_.Schedule(dictionary_url);
+  fetcher_.Schedule(dictionary_url,
+                    scoped_ptr<SdchDictionaryFetcher::Data>().Pass());
 }
 
 void SdchOwner::OnClearDictionaries(net::SdchManager* manager) {
   total_dictionary_bytes_ = 0;
   local_dictionary_info_.clear();
   fetcher_.Cancel();
+
+  GetPersistentStoreMap(pref_store_)
+      ->Set(kDictionariesName, new base::DictionaryValue());
+}
+
+void SdchOwner::OnPrefValueChanged(const std::string& key) {
+}
+
+void SdchOwner::OnInitializationCompleted(bool succeeded) {
+  // Error handling is simple: Don't read in any dictionaries and let
+  // normal user behavior fault them back in as needed.
+  // TODO(rdsmith): Decide whether to actually flush things to the
+  // persistent store in this case.

[mmenke, 2015/01/30 20:18:41]

Hrm...Do we fail if the file is corrupt, or just if it can't be opened, or what?
 Would like to see this figured out sooner rather than later.

[Randy Smith (Not in Mondays), 2015/01/31 02:53:29]

If you're asking about the implementation of JsonPrefStore, it looks to me as if
the only time in which it signals a lack of success is if the directory
containing the specified file doesn't exist.  There can be errors returned even
if the initialization counts as successful; if the error has to do with file
corruption, the file is moved off to the side and no initialization is done (but
presumably the later writing to disk will work).  There are some other errors
that I would think would result in failure (and hence should probably not signal
initialization succeeded) such as not being able to read the file (probably
can't write it either, and it's not moved) and the file being locked.  

I think what this means is that I need to handle errors by explicitly checking
the error code.  I'll bucket the error code according to whether it indicates
that we will be able to write the file to disk or not; if it suggests we won't,
I won't switch to the persistent pref store (i.e. we we won't persist this
information at the next browser restart) and otherwise I will, but I won't worry
about whether the data is read in or not (since if it isn't, the remedial action
is to fault in the dictionaries again through user action).  Sound good?

+  if (!succeeded)
+    return;
+
+  // 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 cannot be used here as this
+  // data comes from disk, and hence can't be trusted.

[Bernhard Bauer, 2015/01/29 17:42:23]

What does it mean that the data can't be trusted? Are we actually concerned
about an attacker who can modify local files? (Usually we aren't). Or does that
mean something else?

[Randy Smith (Not in Mondays), 2015/01/30 20:11:10]

I'm not concerned about it from a security POV, but I am from a stability POV. 
I don't want disk corruption to result in Chrome crashes (or something else
ugly) from which the user can't recover because the corruption is persistent. 
I've rephrased the comment to try to make that more clear.

+  if (persistent_pref_store_->GetValue(kPreferenceName,
+                                       &sdch_persistence_value) &&
+      sdch_persistence_value->GetAsDictionary(&sdch_persistence_dictionary)) {
+    LoadPersistedDictionaries(*sdch_persistence_dictionary);
+  }
+
+  // Reset the persistent store and update it with the accumulated
+  // information from the local store.
+  InitializePersistentStore(persistent_pref_store_);
+  GetPersistentStoreDictionaryMap(persistent_pref_store_)
+      ->Swap(GetPersistentStoreDictionaryMap(temporary_pref_store_.get()));
+
+  // Switch the class pointer and free the old store.
+  pref_store_ = persistent_pref_store_;
+  temporary_pref_store_ = nullptr;
 }
 
 void SdchOwner::SetClockForTesting(scoped_ptr<base::Clock> clock) {
@@ -262,4 +442,42 @@ void SdchOwner::OnMemoryPressure(
   manager_->ClearData();
 }
 
+bool SdchOwner::LoadPersistedDictionaries(
+    const base::DictionaryValue& persisted_info) {
+  // Any schema error will result in dropping the persisted info.
+  int version = 0;
+  if (!persisted_info.GetInteger(kVersionName, &version))
+    return false;
+
+  // TODO(rdsmith): Handle version upgrades.
+  if (version != 1)
+    return false;
+
+  const base::DictionaryValue* dictionary_set = nullptr;
+  if (!persisted_info.GetDictionary(kDictionariesName, &dictionary_set))
+    return false;
+
+  for (base::DictionaryValue::Iterator dict_it(*dictionary_set);
+       !dict_it.IsAtEnd(); dict_it.Advance()) {

[mmenke, 2015/01/30 20:18:41]

Maybe use a nifty new range loop?

[Randy Smith (Not in Mondays), 2015/01/31 02:53:29]

So I suspect I'm missing something (I find the new range loops really cool, but
a bit of black magic--maybe there's some specific signature on an iterator that
allows them to be used?) but I don't think that'd work with this class. 
Specifically, I think base::DictonaryValue::Iterator is conceptually an iterator
in that it allows iterating through dictionary values, but it's not a C++
iterator; at least, there's no begin() method that can initialize it.  Just for
grins and giggles I tried the first syntax that came to mind for doing this (

  for (base::DictionaryValue::Iterator dict_it: *dictionary_set) {

)

and got a "no viable 'begin' functionality available", which fit what I thought
from reading the class decl.  So I don't think I can use a range loop.

[Bernhard Bauer, 2015/02/02 14:39:46]

Yeah, a C++11-style for-loop only works with classes that have begin() and end()
methods which return suitable iterators (or that have std::begin() and
std::end() overloaded, I think); see
http://en.cppreference.com/w/cpp/language/range-for.

+    GURL dict_url;

[mmenke, 2015/01/30 20:18:41]

This doesn't seem to be initialized

[Randy Smith (Not in Mondays), 2015/01/31 02:53:29]

Whoops, true that.  Fixed.

+    double last_used = 0.0;
+    int use_count = 0;
+
+    const base::DictionaryValue* dict_info = nullptr;
+    if (!dict_it.value().GetAsDictionary(&dict_info))
+      return false;
+    if (!dict_info->GetDouble(kDictionaryLastUsedName, &last_used))
+      return false;
+    if (!dict_info->GetInteger(kDictionaryUseCountName, &use_count))
+      return false;
+
+    scoped_ptr<FetchData> data(
+        new FetchData(base::Time::FromDoubleT(last_used), use_count));
+
+    fetcher_.ScheduleReload(dict_url, data.Pass());
+  }
+
+  return true;
+}
+
 }  // namespace net