Pin dictionaries from being deleted while request is outstanding.

net/base/sdch_manager.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_BASE_SDCH_MANAGER_H_
 #define NET_BASE_SDCH_MANAGER_H_
 
 #include <map>
 #include <set>
 #include <string>
+#include <vector>
 
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/observer_list.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
 #include "net/base/net_export.h"
 #include "url/gurl.h"
 
 namespace net {
 
 class SdchObserver;
 
 // Provides global database of differential decompression dictionaries for the
 // SDCH filter (processes sdch enconded content).
 //
 // The SdchManager maintains a collection of memory resident dictionaries. It
 // can find a dictionary (based on a server specification of a hash), store a
 // dictionary, and make judgements about what URLs can use, set, etc. a
 // dictionary.
 
 // These dictionaries are acquired over the net, and include a header
 // (containing metadata) as well as a VCDIFF dictionary (for use by a VCDIFF
 // 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,
     FIXED_CONTENT_ENCODING = 2,
     FIXED_CONTENT_ENCODINGS = 3,
 
     // Content decoding errors.
     DECODE_HEADER_ERROR = 4,
     DECODE_BODY_ERROR = 5,
 
@@ skipping 30 matching lines @@
     DICTIONARY_SELECTED_FROM_NON_HTTP = 33,
     DICTIONARY_IS_TOO_LARGE= 34,
     DICTIONARY_COUNT_EXCEEDED = 35,
     DICTIONARY_ALREADY_SCHEDULED_TO_DOWNLOAD = 36,
     DICTIONARY_ALREADY_TRIED_TO_DOWNLOAD = 37,
     DICTIONARY_FETCH_READ_FAILED = 38,
 
     // 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,
 
     // Problematic decode recovery methods.
     META_REFRESH_RECOVERY = 70,            // Dictionary not found.
     // defunct =  71, // Almost the same as META_REFRESH_UNSUPPORTED.
     // defunct = 72,  // Almost the same as CACHED_META_REFRESH_UNSUPPORTED.
     // defunct = 73,  // PASSING_THROUGH_NON_SDCH plus
                       // RESPONSE_TENTATIVE_SDCH in ../filter/sdch_filter.cc.
     META_REFRESH_UNSUPPORTED = 74,         // Unrecoverable error.
@@ skipping 27 matching lines @@
 
     MAX_PROBLEM_CODE  // Used to bound histogram.
   };
 
   // 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.

[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.
     Dictionary(const std::string& dictionary_text,
                size_t offset,
                const std::string& client_hash,
                const GURL& url,
                const std::string& domain,
                const std::string& path,
                const base::Time& expiration,
                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,
                           const std::string& restriction);
 
     // 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_;
 
     // Part of the hash of text_ that the client uses to advertise the fact that
     // it has a specific dictionary pre-cached.
     std::string client_hash_;
 
     // The GURL that arrived with the text_ in a URL request to specify where
     // this dictionary may be used.
     const GURL url_;
 
     // Metadate "headers" in before dictionary text contained the following:
     // Each dictionary payload consists of several headers, followed by the text
     // of the dictionary. The following are the known headers.
     const std::string domain_;
     const std::string path_;
     const base::Time expiration_;  // Implied by max-age.
     const std::set<int> ports_;
 
     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();
 
   // Clear data (for browser data removal).
   void ClearData();
 
   // Record stats on various errors.
   static void SdchErrorRecovery(ProblemCodes problem);
 
   // Enables or disables SDCH compression.
@@ skipping 35 matching lines @@
   // Check to see if SDCH is enabled (globally), and the given URL is in a
   // supported domain (i.e., not blacklisted, and either the specific supported
   // domain, or all domains were assumed supported). If it is blacklist, reduce
   // by 1 the number of times it will be reported as blacklisted.
   bool IsInSupportedDomain(const GURL& url);
 
   // Send out appropriate events notifying observers that a Get-Dictionary
   // 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 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 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 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
   // otherwise private
   static void GenerateHash(const std::string& dictionary_text,
                            std::string* client_hash, std::string* server_hash);
 
   // For Latency testing only, we need to know if we've succeeded in doing a
   // round trip before starting our comparative tests. If ever we encounter
   // problems with SDCH, we opt-out of the test unless/until we perform a
   // complete SDCH decoding.
   bool AllowLatencyExperiment(const GURL& url) const;
 
   void SetAllowLatencyExperiment(const GURL& url, bool enable);
 
   // Add an SDCH dictionary to our list of availible
   // dictionaries. This addition will fail if addition is illegal
   // (data in the dictionary is not acceptable from the
   // dictionary_url; dictionary already added, etc.).
   void AddSdchDictionary(const std::string& dictionary_text,
                          const GURL& dictionary_url);
 
   // Registration for events generated by the SDCH subsystem.
   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;
 
   // Determines whether a "Get-Dictionary" header is legal (dictionary
   // url has appropriate relationship to referrer url) in the SDCH
   // protocol.  Return true if fetch is legal.
   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_;
 
   // Support SDCH compression for HTTPS requests and responses. When supported,
   // HTTPS applicable dictionaries MUST have been acquired securely via HTTPS.
   static bool g_secure_scheme_supported_;
 
   // 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_;
 
   // List of hostnames for which a latency experiment is allowed (because a
   // round trip test has recently passed).
   ExperimentSet allow_latency_experiment_;
 
   // Observers that want to be notified of SDCH events.
   // Assert list is empty on destruction since if there is an observer
   // that hasn't removed itself from the list, that observer probably
   // has a reference to the SdchManager.
   ObserverList<SdchObserver, true> observers_;
 
   base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(SdchManager);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_SDCH_MANAGER_H_