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

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 0f31d5cee8ad84e4ea2acacd8c06a52b786847b2..6010a9c4304f4e3b8301df5a6fe0c963f9b5607e 100644
--- a/net/base/sdch_manager.cc
+++ b/net/base/sdch_manager.cc
@@ -11,6 +11,7 @@
 #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 {
@@ -241,8 +242,7 @@ bool SdchManager::Dictionary::DomainMatch(const GURL& gurl,
 }
 
 //------------------------------------------------------------------------------
-SdchManager::SdchManager()
-    : fetches_count_for_testing_(0) {
+SdchManager::SdchManager() {
   DCHECK(CalledOnValidThread());
 }
 
@@ -257,14 +257,14 @@ SdchManager::~SdchManager() {
 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
@@ -272,11 +272,6 @@ 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;
@@ -377,13 +372,13 @@ bool SdchManager::IsInSupportedDomain(const GURL& url) {
   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,
@@ -499,6 +494,16 @@ void SdchManager::SetAllowLatencyExperiment(const GURL& url, bool enable) {
   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());