Clear SDCH information on "Clear browsing data" path.

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"
@@ skipping 201 matching lines @@
 
 //------------------------------------------------------------------------------
 SdchManager::SdchManager() {
   DCHECK(CalledOnValidThread());
 }
 
 SdchManager::~SdchManager() {
   DCHECK(CalledOnValidThread());
   while (!dictionaries_.empty()) {
     DictionaryMap::iterator it = dictionaries_.begin();
-    it->second->Release();
     dictionaries_.erase(it->first);
   }
 }
 
+void SdchManager::ClearData() {
+  blacklisted_domains_.clear();
+  exponential_blacklist_count_.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();

[jar (doing other things), 2014/06/17 05:48:55]

I suspect this is the source of the leak.  You are clearing the list of
dictionaries, but not releasing the pointers one by one (as you would do in the
destructor, on lines 220-223).

With the new code patch set 10 code, this would almost work via the
scoped_ptr... but then you'd have worse problems during map reallocations.

[jar (doing other things), 2014/06/17 17:03:28]

(comment): Using the scoped_refptr<>.... this construct will be fine here, and
resolve the leak.

[Randy Smith (Not in Mondays), 2014/06/17 18:33:11]

Done.

[Randy Smith (Not in Mondays), 2014/06/17 18:33:11]

Done.

+}
+
 // static
 void SdchManager::SdchErrorRecovery(ProblemCodes problem) {
   UMA_HISTOGRAM_ENUMERATION("Sdch3.ProblemCodes_4", problem, MAX_PROBLEM_CODE);
 }
 
 void SdchManager::set_sdch_fetcher(SdchFetcher* fetcher) {
   DCHECK(CalledOnValidThread());
   fetcher_.reset(fetcher);
 }
 
 // static
 void SdchManager::EnableSdchSupport(bool enabled) {
   g_sdch_enabled_ = enabled;
 }
 
 // static
 void SdchManager::EnableSecureSchemeSupport(bool enabled) {
   g_secure_scheme_supported_ = enabled;
 }
 
 void SdchManager::BlacklistDomain(const GURL& url) {
   SetAllowLatencyExperiment(url, false);
 
   std::string domain(StringToLowerASCII(url.host()));
   int count = blacklisted_domains_[domain];
   if (count > 0)
     return;  // Domain is already blacklisted.
 
-  count = 1 + 2 * exponential_blacklist_count[domain];
+  count = 1 + 2 * exponential_blacklist_count_[domain];
   if (count > 0)
-    exponential_blacklist_count[domain] = count;
+    exponential_blacklist_count_[domain] = count;
   else
     count = INT_MAX;
 
   blacklisted_domains_[domain] = count;
 }
 
 void SdchManager::BlacklistDomainForever(const GURL& url) {
   SetAllowLatencyExperiment(url, false);
 
   std::string domain(StringToLowerASCII(url.host()));
-  exponential_blacklist_count[domain] = INT_MAX;
+  exponential_blacklist_count_[domain] = INT_MAX;
   blacklisted_domains_[domain] = INT_MAX;
 }
 
 void SdchManager::ClearBlacklistings() {
   blacklisted_domains_.clear();
-  exponential_blacklist_count.clear();
+  exponential_blacklist_count_.clear();
 }
 
 void SdchManager::ClearDomainBlacklisting(const std::string& domain) {
   blacklisted_domains_.erase(StringToLowerASCII(domain));
 }
 
 int SdchManager::BlackListDomainCount(const std::string& domain) {
   if (blacklisted_domains_.end() == blacklisted_domains_.find(domain))
     return 0;
   return blacklisted_domains_[StringToLowerASCII(domain)];
 }
 
 int SdchManager::BlacklistDomainExponential(const std::string& domain) {
-  if (exponential_blacklist_count.end() ==
-      exponential_blacklist_count.find(domain))
+  if (exponential_blacklist_count_.end() ==
+      exponential_blacklist_count_.find(domain))
     return 0;
-  return exponential_blacklist_count[StringToLowerASCII(domain)];
+  return exponential_blacklist_count_[StringToLowerASCII(domain)];
 }
 
 bool SdchManager::IsInSupportedDomain(const GURL& url) {
   DCHECK(CalledOnValidThread());
   if (!g_sdch_enabled_ )
     return false;
 
   if (!secure_scheme_supported() && url.SchemeIsSecure())
     return false;
 
@@ skipping 146 matching lines @@
     SdchErrorRecovery(DICTIONARY_COUNT_EXCEEDED);
     return false;
   }
 
   UMA_HISTOGRAM_COUNTS("Sdch3.Dictionary size loaded", dictionary_text.size());
   DVLOG(1) << "Loaded dictionary with client hash " << client_hash
            << " and server hash " << server_hash;
   Dictionary* dictionary =
       new Dictionary(dictionary_text, header_end + 2, client_hash,
                      dictionary_url, domain, path, expiration, ports);
-  dictionary->AddRef();
-  dictionaries_[server_hash] = dictionary;
+  dictionaries_[server_hash] = scoped_refptr<Dictionary>(dictionary);

[jar (doing other things), 2014/06/17 05:48:55]

This is actually a bad pattern. You typically can't put scoped_refptr<> into an
STL container.  See http://www.chromium.org/developers/smart-pointer-guidelines
for a longer explanation.

The existing code (manual AddRef and Release) would achieve what you wanted...
so this could not be the cause.

[jar (doing other things), 2014/06/17 17:03:28]

nit: If you're going to use scoped_refptr, there is no need for this cast, just
do the assignment.... or better yet... since you don't need to manually AddRef,
don't waste time with the temporary.

...but if you really like the temp... it is fine... just kill the cast.

[Randy Smith (Not in Mondays), 2014/06/17 18:33:11]

Done.

[Randy Smith (Not in Mondays), 2014/06/17 18:33:11]

Really done (to distinguish from the other "Done"s which are just dismissing
comments.)

   return true;
 }
 
 void SdchManager::GetVcdiffDictionary(const std::string& server_hash,
     const GURL& referring_url, Dictionary** dictionary) {
   DCHECK(CalledOnValidThread());
   *dictionary = NULL;
   DictionaryMap::iterator it = dictionaries_.find(server_hash);
   if (it == dictionaries_.end()) {
     return;
@@ skipping 80 matching lines @@
       case '/':
         (*output)[i] = '_';
         continue;
       default:
         continue;
     }
   }
 }
 
 }  // namespace net