Pin dictionaries from being deleted while request is outstanding.

net/base/sdch_manager.cc

Index: net/base/sdch_manager.cc
diff --git a/net/base/sdch_manager.cc b/net/base/sdch_manager.cc
index 1285ad9383aa1784d92ebd3481c910f69454b091..e064e525a76ba545df687c01066760d083966ec5 100644
--- a/net/base/sdch_manager.cc
+++ b/net/base/sdch_manager.cc
@@ -78,37 +78,6 @@ SdchManager::Dictionary::Dictionary(const std::string& dictionary_text,
 SdchManager::Dictionary::~Dictionary() {
 }
 
-bool SdchManager::Dictionary::CanAdvertise(const GURL& target_url) {
-  /* The specific rules of when a dictionary should be advertised in an
-     Avail-Dictionary header are modeled after the rules for cookie scoping. The
-     terms "domain-match" and "pathmatch" are defined in RFC 2965 [6]. A
-     dictionary may be advertised in the Avail-Dictionaries header exactly when
-     all of the following are true:
-      1. The server's effective host name domain-matches the Domain attribute of
-         the dictionary.
-      2. If the dictionary has a Port attribute, the request port is one of the
-         ports listed in the Port attribute.
-      3. The request URI path-matches the path header of the dictionary.
-      4. The request is not an HTTPS request.
-     We can override (ignore) item (4) only when we have explicitly enabled
-     HTTPS support AND the dictionary acquisition scheme matches the target
-     url scheme.
-    */
-  if (!DomainMatch(target_url, domain_))
-    return false;
-  if (!ports_.empty() && 0 == ports_.count(target_url.EffectiveIntPort()))
-    return false;
-  if (path_.size() && !PathMatch(target_url.path(), path_))
-    return false;
-  if (!SdchManager::secure_scheme_supported() && target_url.SchemeIsSecure())
-    return false;
-  if (target_url.SchemeIsSecure() != url_.SchemeIsSecure())
-    return false;
-  if (base::Time::Now() > expiration_)
-    return false;
-  return true;
-}
-
 //------------------------------------------------------------------------------
 // Security functions restricting loads and use of dictionaries.
 
@@ -171,8 +140,8 @@ bool SdchManager::Dictionary::CanSet(const std::string& domain,
   return true;
 }
 
-// static
-bool SdchManager::Dictionary::CanUse(const GURL& referring_url) {
+SdchManager::ProblemCodes
+SdchManager::Dictionary::CanUse(const GURL& target_url) {
   /*
     1. The request URL's host name domain-matches the Domain attribute of the
       dictionary.
@@ -184,37 +153,23 @@ bool SdchManager::Dictionary::CanUse(const GURL& referring_url) {
     HTTPS support AND the dictionary acquisition scheme matches the target
      url scheme.
   */
-  if (!DomainMatch(referring_url, domain_)) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_DOMAIN);
-    return false;
-  }
-  if (!ports_.empty()
-      && 0 == ports_.count(referring_url.EffectiveIntPort())) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_PORT_LIST);
-    return false;
-  }
-  if (path_.size() && !PathMatch(referring_url.path(), path_)) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_PATH);
-    return false;
-  }
-  if (!SdchManager::secure_scheme_supported() &&
-      referring_url.SchemeIsSecure()) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_SCHEME);
-    return false;
-  }
-  if (referring_url.SchemeIsSecure() != url_.SchemeIsSecure()) {
-    SdchErrorRecovery(DICTIONARY_FOUND_HAS_WRONG_SCHEME);
-    return false;
-  }
+  if (!DomainMatch(target_url, domain_))
+    return DICTIONARY_FOUND_HAS_WRONG_DOMAIN;
+  if (!ports_.empty() && 0 == ports_.count(target_url.EffectiveIntPort()))
+    return DICTIONARY_FOUND_HAS_WRONG_PORT_LIST;
+  if (path_.size() && !PathMatch(target_url.path(), path_))
+    return DICTIONARY_FOUND_HAS_WRONG_PATH;
+  if (!SdchManager::secure_scheme_supported() && target_url.SchemeIsSecure())
+    return DICTIONARY_FOUND_HAS_WRONG_SCHEME;
+  if (target_url.SchemeIsSecure() != url_.SchemeIsSecure())
+    return DICTIONARY_FOUND_HAS_WRONG_SCHEME;
 
   // TODO(jar): Remove overly restrictive failsafe test (added per security
   // review) when we have a need to be more general.
-  if (!referring_url.SchemeIsHTTPOrHTTPS()) {
-    SdchErrorRecovery(ATTEMPT_TO_DECODE_NON_HTTP_DATA);
-    return false;
-  }
+  if (!target_url.SchemeIsHTTPOrHTTPS())
+    return ATTEMPT_TO_DECODE_NON_HTTP_DATA;
 
-  return true;
+  return OK;
 }
 
 bool SdchManager::Dictionary::PathMatch(const std::string& path,
@@ -241,6 +196,53 @@ bool SdchManager::Dictionary::DomainMatch(const GURL& gurl,
   return gurl.DomainIs(restriction.data(), restriction.size());
 }
 
+bool SdchManager::Dictionary::Expired() const {
+  return base::Time::Now() > expiration_;

[Ryan Sleevi, 2014/11/14 03:21:25]

Whenever people do this, they tend to then need to screw with the clock in order
to cover the whole "expired" test case.

Would a
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/clock.h&...
make more sense as an input here?

[Randy Smith (Not in Mondays), 2014/11/17 17:04:21]

Nice; hadn't known about that class.  From grepping, I take the standard idiom
here as putting in a SetClockForTesting() method, and I've followed that.  
Done.

+}
+
+//------------------------------------------------------------------------------

[Ryan Sleevi, 2014/11/14 03:21:24]

don't do this

[Randy Smith (Not in Mondays), 2014/11/17 17:04:21]

Done.

+SdchManager::DictionaryWrapper::DictionaryWrapper(
+    scoped_ptr<SdchManager::Dictionary> dictionary)
+    : dictionary_(dictionary.Pass()) {}
+
+SdchManager::DictionaryWrapper::~DictionaryWrapper() {}
+
+//------------------------------------------------------------------------------

[Ryan Sleevi, 2014/11/14 03:21:25]

don't do this

[Randy Smith (Not in Mondays), 2014/11/17 17:04:21]

Done.

+SdchManager::DictionarySet::DictionarySet() {}
+
+SdchManager::DictionarySet::~DictionarySet() {}
+
+void SdchManager::DictionarySet::GetDictionaryClientHashList(
+    std::string* client_hashes) const {
+  for (auto it = dictionaries_.begin(); it != dictionaries_.end(); ++it) {
+    if (it != dictionaries_.begin())
+      client_hashes->append(",");
+
+    client_hashes->append(it->second->dictionary()->client_hash());

[Ryan Sleevi, 2014/11/14 03:21:24]

Is it expected there will be a lot of client hashes? Is the client hash a fixed
size? If so, does it make sense to .reserve the (size + (fixed size * the
dictionaries_.size())) ?

(with a note indicating "Yes, I know this multiplication may overflow, but it's
just a reservation, and it'll blow up when it tries to append that much anyways)

[Randy Smith (Not in Mondays), 2014/11/17 17:04:21]

The common case is just one hash, and if we ever have enough hashes to make
something like that make sense, we're in a world of hurt because that number of
dictionaries is going to start putting us under some serious memory pressure, at
least on mobile.  So my inclination is to leave it like it is, but I don't feel
strongly and am willing to add the reserve + comment if you'd prefer.

+  }
+}
+
+const SdchManager::Dictionary* SdchManager::DictionarySet::Dictionary(
+    const std::string& hash) const {
+  auto it = dictionaries_.find(hash);
+  if (it == dictionaries_.end())
+    return NULL;
+
+  return it->second->dictionary();
+}
+
+bool SdchManager::DictionarySet::Empty() const {
+  return dictionaries_.empty();
+}
+
+void SdchManager::DictionarySet::AddDictionary(
+    const std::string& server_hash,
+    scoped_refptr<SdchManager::DictionaryWrapper> dictionary) {
+  DCHECK(dictionaries_.end() == dictionaries_.find(server_hash));

[Ryan Sleevi, 2014/11/14 03:21:24]

Is this really a DCHECK thing? Which is to say, is it programmer mistake, or can
the protocol itself cause this to happen?

[Randy Smith (Not in Mondays), 2014/11/17 17:04:21]

Yeah, it's a programmer mistake.  The only entity that calls
DictionarySet::AddDictionary() is the SdchManager in putting together the
DictionarySet, and it can't have more than one dictionary with a given server
hash.  That's not true at the SdchManager level (the server may have
dictionaries at two different URLs that have the same server hash) but the
SdchManager handles it.  

DictionarySet::AddDictionary() is a private method on DictionarySet only used by
the SdchManager, so I'm not inclined to add a comment to the header about this
interface requirement, but let me know if you disagree.

+
+  dictionaries_[server_hash] = dictionary;
+}
+
 //------------------------------------------------------------------------------

[Ryan Sleevi, 2014/11/14 03:21:24]

:'(

[Randy Smith (Not in Mondays), 2014/11/17 17:04:21]

I'm tempted to make a comment about consistency within a file, but that'll
probably just produce more crystalline tears :-}.  I've removed all of these
lines in the file.  

 SdchManager::SdchManager() {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -249,7 +251,7 @@ SdchManager::SdchManager() {
 SdchManager::~SdchManager() {
   DCHECK(thread_checker_.CalledOnValidThread());
   while (!dictionaries_.empty()) {
-    DictionaryMap::iterator it = dictionaries_.begin();
+    auto it = dictionaries_.begin();
     dictionaries_.erase(it->first);
   }
 }
@@ -322,7 +324,7 @@ void SdchManager::ClearDomainBlacklisting(const std::string& domain) {
   BlacklistInfo* blacklist_info = &blacklisted_domains_[
       base::StringToLowerASCII(domain)];
   blacklist_info->count = 0;
-  blacklist_info->reason = MIN_PROBLEM_CODE;
+  blacklist_info->reason = OK;
 }
 
 int SdchManager::BlackListDomainCount(const std::string& domain) {
@@ -366,7 +368,7 @@ bool SdchManager::IsInSupportedDomain(const GURL& url) {
     it->second.count = count;
   } else {
     it->second.count = 0;
-    it->second.reason = MIN_PROBLEM_CODE;
+    it->second.reason = OK;
   }
 
   return false;
@@ -416,45 +418,49 @@ bool SdchManager::CanFetchDictionary(const GURL& referring_url,
   return true;
 }
 
-void SdchManager::GetVcdiffDictionary(
-    const std::string& server_hash,
-    const GURL& referring_url,
-    scoped_refptr<Dictionary>* dictionary) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  *dictionary = NULL;
-  DictionaryMap::iterator it = dictionaries_.find(server_hash);
-  if (it == dictionaries_.end()) {
-    return;
-  }
-  scoped_refptr<Dictionary> matching_dictionary = it->second;
-  if (!IsInSupportedDomain(referring_url))
-    return;
-  if (!matching_dictionary->CanUse(referring_url))
-    return;
-  *dictionary = matching_dictionary;
-}
+scoped_ptr<SdchManager::DictionarySet>
+SdchManager::GetDictionarySet(const GURL& target_url) {
+  if (!IsInSupportedDomain(target_url))
+    return NULL;
 
-// TODO(jar): If we have evictions from the dictionaries_, then we need to
-// change this interface to return a list of reference counted Dictionary
-// instances that can be used if/when a server specifies one.
-void SdchManager::GetAvailDictionaryList(const GURL& target_url,
-                                         std::string* list) {
-  DCHECK(thread_checker_.CalledOnValidThread());
   int count = 0;
-  for (DictionaryMap::iterator it = dictionaries_.begin();
+  scoped_ptr<SdchManager::DictionarySet> result(new DictionarySet);
+  for (auto it = dictionaries_.begin();
        it != dictionaries_.end(); ++it) {

[Ryan Sleevi, 2014/11/14 03:21:24]

Range-based for loops are allowed - https://chromium-cpp.appspot.com/

for (const auto& dictionary : dictionaries_) {
  if (dictionary->second->dictionary() ...)

[Randy Smith (Not in Mondays), 2014/11/17 17:04:21]

Cool!  I like the syntax, so I went wild :-}.

-    if (!IsInSupportedDomain(target_url))
+    if (it->second->dictionary()->CanUse(target_url) != OK)
       continue;
-    if (!it->second->CanAdvertise(target_url))
+    if (it->second->dictionary()->Expired())
       continue;
     ++count;
-    if (!list->empty())
-      list->append(",");
-    list->append(it->second->client_hash());
+    result->AddDictionary(it->first, it->second);
   }
-  // Watch to see if we have corrupt or numerous dictionaries.
-  if (count > 0)
-    UMA_HISTOGRAM_COUNTS("Sdch3.Advertisement_Count", count);
+
+  if (count == 0)
+    return NULL;
+
+  UMA_HISTOGRAM_COUNTS("Sdch3.Advertisement_Count", count);
+
+  return result.Pass();
+}
+
+scoped_ptr<SdchManager::DictionarySet>
+SdchManager::GetDictionarySetByHash(
+    const GURL& target_url, const std::string& server_hash) {
+  scoped_ptr<SdchManager::DictionarySet> result;
+
+  auto it = dictionaries_.find(server_hash);
+  if (it == dictionaries_.end())
+    return result;
+
+  ProblemCodes ret = it->second->dictionary()->CanUse(target_url);
+  if (ret != OK) {
+    SdchErrorRecovery(ret);
+    return result;
+  }
+
+  result.reset(new DictionarySet);
+  result->AddDictionary(it->first, it->second);
+  return result;
 }
 
 // static
@@ -600,15 +606,21 @@ void SdchManager::AddSdchDictionary(const std::string& dictionary_text,
   UMA_HISTOGRAM_COUNTS("Sdch3.Dictionary size loaded", dictionary_text.size());
   DVLOG(1) << "Loaded dictionary with client hash " << client_hash
            << " and server hash " << server_hash;
-  Dictionary* dictionary =
+  scoped_ptr<Dictionary> dictionary(
       new Dictionary(dictionary_text, header_end + 2, client_hash,
                      dictionary_url_normalized, domain,
-                     path, expiration, ports);
-  dictionaries_[server_hash] = dictionary;
+                     path, expiration, ports));
+  dictionaries_[server_hash] = new DictionaryWrapper(dictionary.Pass());
   return;
 }
 
 // static
+scoped_ptr<SdchManager::DictionarySet>
+SdchManager::CreateNullDictionarySetForTesting() {
+  return scoped_ptr<DictionarySet>(new DictionarySet).Pass();
+}
+
+// static
 void SdchManager::UrlSafeBase64Encode(const std::string& input,
                                       std::string* output) {
   // Since this is only done during a dictionary load, and hashes are only 8