Pin dictionaries from being deleted while request is outstanding.

net/base/sdch_manager.h

Index: net/base/sdch_manager.h
diff --git a/net/base/sdch_manager.h b/net/base/sdch_manager.h
index 016978a24a2d752f8c4434033f0e846fdecf9e62..a06a993227026d7fcab7ca2a2071b8d0df58d079 100644
--- a/net/base/sdch_manager.h
+++ b/net/base/sdch_manager.h
@@ -8,6 +8,7 @@
 #include <map>
 #include <set>
 #include <string>
+#include <vector>
 
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
@@ -20,6 +21,7 @@
 #include "url/gurl.h"
 
 namespace base {
+class Clock;
 class Value;
 }
 
@@ -40,23 +42,15 @@ class SdchObserver;
 // module) to decompress data.
 class NET_EXPORT SdchManager {
  public:
+  class DictionarySet;

[Ryan Sleevi, 2014/11/20 22:50:29]

You don't actually need to forward declare this here; this symbol is unused
until the full declaration is provided.

[Randy Smith (Not in Mondays), 2014/11/20 23:44:28]

Done.

+
   // Use the following static limits to block DOS attacks until we implement
   // a cached dictionary evicition strategy.
   static const size_t kMaxDictionarySize;
   static const size_t kMaxDictionaryCount;
 
-  // There is one instance of |Dictionary| for each memory-cached SDCH
-  // dictionary.
-  class NET_EXPORT_PRIVATE Dictionary : public base::RefCounted<Dictionary> {
+  class NET_EXPORT_PRIVATE Dictionary {
    public:
-    // Sdch filters can get our text to use in decoding compressed data.
-    const std::string& text() const { return text_; }
-
-   private:
-    friend class base::RefCounted<Dictionary>;
-    friend class SdchManager;  // Only manager can construct an instance.
-    FRIEND_TEST_ALL_PREFIXES(SdchManagerTest, PathMatch);
-
     // Construct a vc-diff usable dictionary from the dictionary_text starting
     // at the given offset. The supplied client_hash should be used to
     // advertise the dictionary's availability relative to the suppplied URL.
@@ -68,7 +62,11 @@ class NET_EXPORT SdchManager {
                const std::string& path,
                const base::Time& expiration,
                const std::set<int>& ports);
-    virtual ~Dictionary();
+
+    ~Dictionary();
+
+    // Sdch filters can get our text to use in decoding compressed data.
+    const std::string& text() const { return text_; }
 
     const GURL& url() const { return url_; }
     const std::string& client_hash() const { return client_hash_; }
@@ -77,10 +75,6 @@ class NET_EXPORT SdchManager {
     const base::Time& expiration() const { return expiration_; }
     const std::set<int>& ports() const { return ports_; }
 
-    // Security method to check if we can advertise this dictionary for use
-    // if the |target_url| returns SDCH compressed data.
-    SdchProblemCode CanAdvertise(const GURL& target_url) const;
-
     // Security methods to check if we can establish a new dictionary with the
     // given data, that arrived in response to get of dictionary_url.
     static SdchProblemCode CanSet(const std::string& domain,
@@ -99,6 +93,15 @@ class NET_EXPORT SdchManager {
     // Compare domains to see if the "match" for dictionary use.
     static bool DomainMatch(const GURL& url, const std::string& restriction);
 
+    // Is this dictionary expired?
+    bool Expired() const;
+
+    void SetClockForTesting(scoped_ptr<base::Clock> clock);
+
+   private:
+    friend class base::RefCountedData<Dictionary>;
+    Dictionary(const Dictionary& rhs);
+
     // The actual text of the dictionary.
     std::string text_;
 
@@ -118,7 +121,68 @@ class NET_EXPORT SdchManager {
     const base::Time expiration_;  // Implied by max-age.
     const std::set<int> ports_;
 
-    DISALLOW_COPY_AND_ASSIGN(Dictionary);
+    scoped_ptr<base::Clock> clock_;

[Ryan Sleevi, 2014/11/20 22:50:30]

Do you want each dictionary to carry a Clock? Should you just have the Manager
hold the clock, since the Manager will outlive any Dictionaries, per contract,
and just have the Dictionary take a pointer to the clock?

I don't think this is too terribly expensive - it's just the difference between
allocating a vtable (since DefaultClock is an empty object, but has virtual
methods) & storing a pointer vs just storing a pointer.

[Randy Smith (Not in Mondays), 2014/11/20 23:44:28]

If there's a contract to that effect, I need to rewrite it--Dictionaries could
outlive the manager.  DictionarySets keep a scoped_refptr<> to any dictionaries,
as does the manager, so when the manager goes away, only the Dictionaries
without DictionarySet references will go away.  I didn't see such a contract
anywhere, but if you did, please point me at it.  I've added a quick clarifying
comment to the SdchManager class comment.
Good, because I'm reluctant to change it for the above reason :-}.

+
+    // Copy and assignment of this class are forbidden for any public
+    // users, but the class implements a private copy constructor
+    // (declared above) for use by RefCountedData<>.  That implementation
+    // means that use of the DISALLOW_COPY_AND_ASSIGN() macro will break.
+    // The declaration below is the assignment disallow copied from that
+    // macro.

[Ryan Sleevi, 2014/11/20 22:50:29]

Rather than this longer comment, which describes the less surprising part, I
would just add a comment around line 103

// Private copy-constructor to support RefCountedData<>, which requires
// that an object stored in it be either DefaultConstructible or
// CopyConstructible

These terms are borrowed from the C++ spec and it's notion of Concepts (
http://en.cppreference.com/w/cpp/concept )

I wouldn't say anything about that here for the =delete.

[Randy Smith (Not in Mondays), 2014/11/20 23:44:29]

Done.  Having said that, I think it's a little confusing to have
DISALLOW_COPY_AND_ASSIGN functionality without that string appearing somewhere
in the class definition.  But I agree it's a lot of verbiage for a small amount
of possible confusion-remediation.

+    void operator=(const Dictionary&) = delete;
+  };
+
+  // Implementation type relevant for the private data members of
+  // DictionarySet and SdchManager. This class should not be used
+  // outside of sdch_manager.*.
+  class DictionaryWrapper : public base::RefCounted<DictionaryWrapper> {
+   public:
+    typedef std::map<std::string, scoped_refptr<DictionaryWrapper> >
+        DictionaryMap;
+
+    explicit DictionaryWrapper(scoped_ptr<Dictionary> dictionary);
+    Dictionary* dictionary() { return dictionary_.get(); }
+
+   private:
+    friend class base::RefCounted<DictionaryWrapper>;
+    ~DictionaryWrapper();
+
+    scoped_ptr<SdchManager::Dictionary> dictionary_;
+
+    DISALLOW_COPY_AND_ASSIGN(DictionaryWrapper);
+  };
+  typedef std::map<std::string, scoped_refptr<base::RefCountedData<Dictionary>>>

[Ryan Sleevi, 2014/11/20 22:50:29]

typedefs go up top -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

[Randy Smith (Not in Mondays), 2014/11/20 23:44:28]

Done.

+      DictionaryMap;
+
+  // A handle for one or more dictionaries which will keep the dictionaries
+  // alive and accessible for the handle's lifetime.
+  class NET_EXPORT_PRIVATE DictionarySet {
+   public:
+    ~DictionarySet();
+
+    // Return a comma separated list of client hashes.
+    std::string GetDictionaryClientHashList() const;
+
+    // Lookup a given dictionary based on server hash. Returned pointer
+    // is guaranteed to be valid for the lifetime of the DictionarySet.
+    // Returns NULL if hash is not a valid server hash for a dictionary
+    // named by DictionarySet.
+    const SdchManager::Dictionary* Dictionary(const std::string& hash) const;

[Ryan Sleevi, 2014/11/20 22:50:29]

nit: Naming - FindDictionary, LookupDictionary, GetDictionary, etc?

[Randy Smith (Not in Mondays), 2014/11/20 23:44:28]

*nod*  Changed to GetDictionary().

+
+    bool Empty() const;
+
+   private:
+    // A DictionarySet may only be constructed by the SdchManager.
+    friend class SdchManager;
+
+    DictionarySet();
+    void AddDictionary(const std::string& server_hash,
+                       scoped_refptr<base::RefCountedData<
+                           SdchManager::Dictionary>> dictionary);

[Ryan Sleevi, 2014/11/20 22:50:29]

const scoped_refptr<>&

only scoped_ptr<>'s get the move semantics.

[Randy Smith (Not in Mondays), 2014/11/20 23:44:28]

Done.

+
+    DictionaryMap dictionaries_;
+
+    DISALLOW_COPY_AND_ASSIGN(DictionarySet);
   };
 
   SdchManager();
@@ -178,23 +242,22 @@ class NET_EXPORT SdchManager {
   SdchProblemCode OnGetDictionary(const GURL& request_url,
                                   const GURL& dictionary_url);
 
-  // Find the vcdiff dictionary (the body of the sdch dictionary that appears
-  // after the meta-data headers like Domain:...) with the given |server_hash|
-  // to use to decompreses data that arrived as SDCH encoded content. Check to
-  // be sure the returned |dictionary| can be used for decoding content supplied
-  // in response to a request for |referring_url|.
-  // Return null in |dictionary| if there is no matching legal dictionary.
-  // Returns SDCH_OK if dictionary is not found, SDCH(-over-https) is disabled,
-  // or if matching legal dictionary exists. Otherwise returns the
-  // corresponding problem code.
-  SdchProblemCode GetVcdiffDictionary(const std::string& server_hash,
-                                      const GURL& referring_url,
-                                      scoped_refptr<Dictionary>* dictionary);
-
-  // Get list of available (pre-cached) dictionaries that we have already loaded
-  // into memory. The list is a comma separated list of (client) hashes per
-  // the SDCH spec.
-  void GetAvailDictionaryList(const GURL& target_url, std::string* list);
+  // Get a handle to the available dictionaries that might be used
+  // for encoding responses for the given URL. The return set will not
+  // include expired dictionaries. If no dictionaries
+  // are appropriate to use with the target_url, NULL is returned.
+  scoped_ptr<DictionarySet> GetDictionarySet(const GURL& target_url);
+
+  // Get a handle to a specific dictionary, by its server hash, confirming
+  // that that specific dictionary is appropriate to use with |target_url|.
+  // Expired dictionaries will be returned. If no dictionary with that
+  // hash exists that is usable with |target_url|, NULL is returned.
+  // If there is a usability problem, |*error_code| is set to the
+  // appropriate problem code.
+  scoped_ptr<DictionarySet> GetDictionarySetByHash(
+      const GURL& target_url,
+      const std::string& server_hash,
+      SdchProblemCode* problem_code);
 
   // Construct the pair of hashes for client and server to identify an SDCH
   // dictionary. This is only made public to facilitate unit testing, but is
@@ -225,6 +288,8 @@ class NET_EXPORT SdchManager {
   void AddObserver(SdchObserver* observer);
   void RemoveObserver(SdchObserver* observer);
 
+  static scoped_ptr<DictionarySet> CreateEmptyDictionarySetForTesting();
+
  private:
   struct BlacklistInfo {
     BlacklistInfo() : count(0), exponential_count(0), reason(SDCH_OK) {}
@@ -233,18 +298,16 @@ class NET_EXPORT SdchManager {
     int exponential_count;   // Current exponential backoff ratchet.
     SdchProblemCode reason;  // Why domain was blacklisted.
   };
+
   typedef std::map<std::string, BlacklistInfo> DomainBlacklistInfo;
   typedef std::set<std::string> ExperimentSet;
 
   // Determines whether a "Get-Dictionary" header is legal (dictionary
   // url has appropriate relationship to referrer url) in the SDCH
-  // protocol.  Return SDCH_OK if fetch is legal.
+  // protocol. Return SDCH_OK if fetch is legal.
   SdchProblemCode CanFetchDictionary(const GURL& referring_url,
                                      const GURL& dictionary_url) const;
 
-  // A map of dictionaries info indexed by the hash that the server provides.
-  typedef std::map<std::string, scoped_refptr<Dictionary> > DictionaryMap;
-
   // Support SDCH compression, by advertising in headers.
   static bool g_sdch_enabled_;