Restructure SDCH layering to allow more separation (observer/1->[0,n]

net/base/sdch_manager.cc

 // Copyright (c) 2012 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.
 
 #include "net/base/sdch_manager.h"
 
 #include "base/base64.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "crypto/sha2.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
+#include "net/base/sdch_observer.h"
 #include "net/url_request/url_request_http_job.h"
 
 namespace {
 
 void StripTrailingDot(GURL* gurl) {
   std::string host(gurl->host());
 
   if (host.empty())
     return;
 
@@ skipping 210 matching lines @@
 }
 
 // static
 bool SdchManager::Dictionary::DomainMatch(const GURL& gurl,
                                           const std::string& restriction) {
   // TODO(jar): This is not precisely a domain match definition.
   return gurl.DomainIs(restriction.data(), restriction.size());
 }
 
 //------------------------------------------------------------------------------
-SdchManager::SdchManager()
-    : fetches_count_for_testing_(0) {
+SdchManager::SdchManager() {
   DCHECK(CalledOnValidThread());
 }
 
 SdchManager::~SdchManager() {
   DCHECK(CalledOnValidThread());
   while (!dictionaries_.empty()) {
     DictionaryMap::iterator it = dictionaries_.begin();
     dictionaries_.erase(it->first);
   }
 }
 
 void SdchManager::ClearData() {
   blacklisted_domains_.clear();
   allow_latency_experiment_.clear();
-  if (fetcher_.get())
-    fetcher_->Cancel();
 
   // Note that this may result in not having dictionaries we've advertised
   // for incoming responses.  The window is relatively small (as ClearData()
   // is not expected to be called frequently), so we rely on meta-refresh
   // to handle this case.
   dictionaries_.clear();
+
+  FOR_EACH_OBSERVER(SdchObserver, observers_, OnClearDictionaries(this));
 }
 
 // static
 void SdchManager::SdchErrorRecovery(ProblemCodes problem) {
   UMA_HISTOGRAM_ENUMERATION("Sdch3.ProblemCodes_4", problem, MAX_PROBLEM_CODE);
 }
 
-void SdchManager::set_sdch_fetcher(scoped_ptr<SdchFetcher> fetcher) {
-  DCHECK(CalledOnValidThread());
-  fetcher_ = fetcher.Pass();
-}
-
 // static
 void SdchManager::EnableSdchSupport(bool enabled) {
   g_sdch_enabled_ = enabled;
 }
 
 // static
 void SdchManager::EnableSecureSchemeSupport(bool enabled) {
   g_secure_scheme_supported_ = enabled;
 }
 
@@ skipping 80 matching lines @@
   if (count > 0) {
     it->second.count = count;
   } else {
     it->second.count = 0;
     it->second.reason = MIN_PROBLEM_CODE;
   }
 
   return false;
 }
 
-void SdchManager::FetchDictionary(const GURL& request_url,
+void SdchManager::OnGetDictionary(const GURL& request_url,
                                   const GURL& dictionary_url) {
-  DCHECK(CalledOnValidThread());
-  if (CanFetchDictionary(request_url, dictionary_url) && fetcher_.get()) {
-    ++fetches_count_for_testing_;
-    fetcher_->Schedule(dictionary_url);
-  }
+  if (!CanFetchDictionary(request_url, dictionary_url))
+    return;
+
+  FOR_EACH_OBSERVER(SdchObserver, observers_,
+                    OnGetDictionary(this, request_url, dictionary_url));
 }
 
 bool SdchManager::CanFetchDictionary(const GURL& referring_url,
                                      const GURL& dictionary_url) const {
   DCHECK(CalledOnValidThread());
   /* The user agent may retrieve a dictionary from the dictionary URL if all of
      the following are true:
        1 The dictionary URL host name matches the referrer URL host name and
            scheme.
        2 The dictionary URL host name domain matches the parent domain of the
@@ skipping 95 matching lines @@
     allow_latency_experiment_.insert(url.host());
     return;
   }
   ExperimentSet::iterator it = allow_latency_experiment_.find(url.host());
   if (allow_latency_experiment_.end() == it)
     return;  // It was already erased, or never allowed.
   SdchErrorRecovery(LATENCY_TEST_DISALLOWED);
   allow_latency_experiment_.erase(it);
 }
 
+void SdchManager::AddObserver(SdchObserver* observer) {
+  observers_.AddObserver(observer);
+  observer->AddRef();
+}
+
+void SdchManager::RemoveObserver(SdchObserver* observer) {
+  observers_.RemoveObserver(observer);
+  observer->RemoveRef();

[Ryan Sleevi, 2014/10/20 22:42:04]

This sort of manual ref-counting strikes me as all sorts of danger zone.

Why is it necessary to AddRef/RemoveRef? Why isn't the Observer simply unowned?

[Randy Smith (Not in Mondays), 2014/10/21 19:05:36]

First, I want to call out that the Add/RemoveRef() on the observer are private
methods that are not visible to inherited classes/part of the Observer
interface.  I.e. this complexity has limited scope, only existing within the
SdchObserver (base class) and SdchManager.  The reason for it is purely because
of the DCHECK in the base class destructor; it's easy for a class to add itself
as an observer and forget to remove oneself before destruction, which leads
directly to a use-after-free error.  

I can remove it if you want, but my judgement was that complexity limited to the
implementation which raised a clear flag in the case of a use-after-free error
was a reasonable tradeoff.  If you have a different judgement, I'd value your
explaining it to me, though as I say, I'm happy to remove it if you don't like
it.

[Ryan Sleevi, 2014/10/21 23:09:29]

I have a different judgement. I dislike re-inventing these semantics for a
non-demonstrated problem. I'm all for removing footguns, but I'm not a fan of
'diagnostic' aids being littered in code. In particular, your implementation
doesn't adhere to what we'd "normally" expect from ref-counting (vis-a-vis
thread-safety), and comes with its own set of surprises. For example, does this
mean it's now valid to add the same observer multiple times?

The solution to this problem in terms of defense in depth is to use an RAII
pattern, not to use the ref-counting. Ensuring that deregistration is handled as
part of destruction, rather than has to happen before destruction, is a way to
guarantee the right rules are observed.

[Randy Smith (Not in Mondays), 2014/10/23 20:57:07]

Done.  

(I tried to do an RAII pattern where SdchObserver construction took an
SdchManager pointer and auto-added itself, but that caused a lot of grief in the
tests, where you'd like the observer to own the manager, and possibly several
managers, so I dropped it :-J.  I could also imagine it triggering the
implementation inheritance issues you raised elsewhere, though if the RAII isn't
implemented in SdchObserver, it doesn't constrain the subclasses.)

+}
+
 void SdchManager::AddSdchDictionary(const std::string& dictionary_text,
     const GURL& dictionary_url) {
   DCHECK(CalledOnValidThread());
   std::string client_hash;
   std::string server_hash;
   GenerateHash(dictionary_text, &client_hash, &server_hash);
   if (dictionaries_.find(server_hash) != dictionaries_.end()) {
     SdchErrorRecovery(DICTIONARY_ALREADY_LOADED);
     return;                             // Already loaded.
   }
@@ skipping 97 matching lines @@
 void SdchManager::UrlSafeBase64Encode(const std::string& input,
                                       std::string* output) {
   // Since this is only done during a dictionary load, and hashes are only 8
   // characters, we just do the simple fixup, rather than rewriting the encoder.
   base::Base64Encode(input, output);
   std::replace(output->begin(), output->end(), '+', '-');
   std::replace(output->begin(), output->end(), '/', '_');
 }
 
 }  // namespace net