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 ff157e52a32e1cb04e955f467b3c524cba11b6a7..15769b7aa10df18d5211ff30b2e8917c15a5fafd 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"
@@ -35,10 +36,12 @@ class SdchObserver;
 // module) to decompress data.
 class NET_EXPORT SdchManager {
  public:
+  class DictionarySet;
+
   // A list of errors that appeared and were either resolved, or used to turn
   // off sdch encoding.
   enum ProblemCodes {
-    MIN_PROBLEM_CODE,
+    OK,

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

NO_PROBLEM? (to avoid confusion with net::OK)

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

I'm going to say that this issue will be moot by the next review you do, because
I'm going to be syncing with http://codereview.chromium.org/423813002 before
then and that pulls ProblemCodes out into its own header file and gives OK a
different name.  So if you look at the PS in which I respond to your comments,
this won't be fixed, but if you check the final PS of the CL, it should be.

 
     // Content-encoding correction problems.
     ADDED_CONTENT_ENCODING = 1,
@@ -89,11 +92,14 @@ class NET_EXPORT SdchManager {
     // Failsafe hack.
     ATTEMPT_TO_DECODE_NON_HTTP_DATA = 40,
 
-
     // Content-Encoding problems detected, with no action taken.
     MULTIENCODING_FOR_NON_SDCH_REQUEST = 50,
     SDCH_CONTENT_ENCODE_FOR_NON_SDCH_REQUEST = 51,
 
+    // A dictionary that wasn't advertised is being used for decoding.
+    UNADVERTISED_SDCH_DICTIONARY_USED = 52,
+    UNADVERTISED_SDCH_DICTIONARY_USED_CACHED = 53,
+
     // Dictionary manager issues.
     DOMAIN_BLACKLIST_INCLUDES_TARGET = 61,
 
@@ -142,16 +148,8 @@ class NET_EXPORT SdchManager {
 
   // There is one instance of |Dictionary| for each memory-cached SDCH
   // dictionary.

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

Is this still true? By making this public, you create the possibility that
multiple Dictionary objects exist for a given dictionary text.

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

Completely fair; now consumers can create their own Dictionary objects.  What is
true is that if consumers get a Dictionary through SdchManager, they'll always
get the same instance of the object for a given hash.  But thinking about where
to put that comment, I'm not sure it matters much for consumers--it's an
implementation detail.  Even if SdchManager returned each consumer their own
copy of the Dictionary, the consumers could use them fine.  So I just deleted
the comment.

-  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.
@@ -165,21 +163,21 @@ class NET_EXPORT SdchManager {
                const std::set<int>& ports);
     virtual ~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_; }
 
-    // Security method to check if we can advertise this dictionary for use
-    // if the |target_url| returns SDCH compressed data.
-    bool CanAdvertise(const GURL& target_url);
-
     // 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 bool CanSet(const std::string& domain, const std::string& path,
                        const std::set<int>& ports, const GURL& dictionary_url);
 
-    // Security method to check if we can use a dictionary to decompress a
-    // target that arrived with a reference to this dictionary.
-    bool CanUse(const GURL& referring_url);
+    // Security method to check if a dictionary is appropriate to use
+    // with the given URL.  Does not check expiration (it's appropriate to

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

single space

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

:-}.  Done (through file).

+    // use dictionaries that were expired while a request is outstanding).
+    SdchManager::ProblemCodes CanUse(const GURL& target_url);
 
     // Compare paths to see if they "match" for dictionary use.
     static bool PathMatch(const std::string& path,
@@ -188,7 +186,10 @@ 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;
 
+   private:
     // The actual text of the dictionary.
     std::string text_;
 
@@ -211,6 +212,59 @@ class NET_EXPORT SdchManager {
     DISALLOW_COPY_AND_ASSIGN(Dictionary);
   };
 
+  // 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> {

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

Why not just RefCountedData?
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/ref_co...

Also, with your move from having the dictionary be a (mostly private) structure
to a (mostly public) structure, does it make sense to make the data members
copyable? 

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

Cool; thanks for pointing that type out to me.  That's quite close to what I
want.

Having said that, this comment caused me a bit of flailing.  The problem is that
I actually would prefer not to have a public copy constructor on the Dictionary
object, as I want to discourage users of the API from creating their own
instances of dictionaries stored in the manager; the dictionary sizes are large
enough that I don't want to encourage taking up the memory footprint.  But
RefCountedData<> requires a copy constructor or a default constructor + setters.
 I eventually settled on using RefCountedData<> with a private copy constructor,
but I'm not thrilled about that option either, since it means we're copying the
dictionary on creation.  But that's not that big a hit, and I decided it was
worth it for not having a custom class to do the work of RefCountedData.  

I'd be very surprised if you don't have any comments on the result; I look
forward to finding out what they are :-}.

+   public:
+    typedef std::map<std::string, scoped_refptr<DictionaryWrapper> >
+        DictionaryMap;

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

Why is this a typedef on the Wrapper? Seems to be an abstraction violation - it
belongs to the thing that maps, not to the wrapper itself

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

Yeah, that's fair.  I was trying to avoid having the typedef in the public
section of SdchManager, as it's not part of the interface and I don't want the
consumers to have visibility into it, and I felt like putting it into
DictionaryWrapper hid it a bit.  But on reflection, that's silly, because it has
to be public in DictionaryWrapper so the other classes can see it.  In any case,
with the switch to RefCountedData<> it's back to being a public typedef.

+
+    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);
+  };
+
+  // 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();
+
+    // Append a comma separated list of client hashes onto the end
+    // of |client_hashes|.
+    void GetDictionaryClientHashList(std::string* client_hashes) const;

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

Why do you append, rather than just returning the comma-separated list?
Premature optimization? It just strikes me as odd that this isn't just
std::string GetDictionaryClientHashList() const ...

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

I think I was instinctively trying to avoid the performance cost of the copy,
but yeah, you're right, premature optimization.  Done.

+
+    // 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/14 03:21:25]

Should you return a const-ref then? That usually helps programatically clarify
the lifetime is tied to the parent object (c.f iterators)

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

I like that idea, and I agree that interface would communicate the lifetime in
an intuitive way.  But I tried to make this work in my mind, and I keep tripping
over "What do we return if a dictionary with that hash isn't present?"  We can
make it part of the interface contract not to do that (and add something into
the interface to probe for presence of hash), probably with DCHECKs, but I think
it's a likely enough misuse or the interface that we have to have some plan for
what it'd do in production (i.e. no DCHECKs).  And I don't think a CHECK is
right, and I can't think of anything else to do.  So after chewing on it a bit,
I decided to leave the interface the way it was, with comment.  Let me know what
you think.

+
+    bool Empty() const;
+
+   private:
+    // A DictionarySet may only be constructed by the SdchManager.
+    friend class SdchManager;
+
+    static const char* kSdchDictionariesHandleDataKey;

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

private static? seems unnecessary?

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

Whoops; serious leftover.  Gone.

+
+    DictionarySet();
+    void AddDictionary(const std::string& server_hash,
+                       scoped_refptr<DictionaryWrapper> dictionary);
+
+    DictionaryWrapper::DictionaryMap dictionaries_;
+
+    DISALLOW_COPY_AND_ASSIGN(DictionarySet);
+  };
+
   SdchManager();
   ~SdchManager();
 
@@ -266,20 +320,19 @@ class NET_EXPORT SdchManager {
   // header has been seen.
   void 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.
-  void 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.
+  scoped_ptr<DictionarySet> GetDictionarySetByHash(
+      const GURL& target_url,
+      const std::string& server_hash);
 
   // 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
@@ -306,18 +359,21 @@ class NET_EXPORT SdchManager {
   void AddObserver(SdchObserver* observer);
   void RemoveObserver(SdchObserver* observer);
 
+  static scoped_ptr<DictionarySet> CreateNullDictionarySetForTesting();

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

s/Null/Empty/ (as otherwise you'd just return a nullptr in the scoped_ptr)

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

Done.

+
  private:
   struct BlacklistInfo {
     BlacklistInfo()
         : count(0),
           exponential_count(0),
-          reason(MIN_PROBLEM_CODE) {}
+          reason(OK) {}
 
     int count;                   // # of times to refuse SDCH advertisement.
     int exponential_count;       // Current exponential backoff ratchet.
     ProblemCodes reason;         // Why domain was blacklisted.
 
   };
+
   typedef std::map<std::string, BlacklistInfo> DomainBlacklistInfo;
   typedef std::set<std::string> ExperimentSet;
 
@@ -327,9 +383,6 @@ class NET_EXPORT SdchManager {
   bool 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_;
 
@@ -340,7 +393,8 @@ class NET_EXPORT SdchManager {
   // A simple implementation of a RFC 3548 "URL safe" base64 encoder.
   static void UrlSafeBase64Encode(const std::string& input,
                                   std::string* output);
-  DictionaryMap dictionaries_;
+
+  DictionaryWrapper::DictionaryMap dictionaries_;

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

see comment re: DictionaryMap typedef

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

Moot.

 
   // List domains where decode failures have required disabling sdch.
   DomainBlacklistInfo blacklisted_domains_;