Skip to content

Commit

Permalink
Revert 277160 "Make SdchManager per-profile."
Browse files Browse the repository at this point in the history
On LSan, SdchManagerTest.CanUseMultipleDictionaries fails (leaks):
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20LSan%20Tests%20%281%29/builds/3073/steps/net_unittests/logs/stdio

E.g.:
Indirect leak of 34 byte(s) in 1 object(s) allocated from:
#0 0x512b7b in operator new(unsigned long) /usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:55
chromium#1 0x7f4730122739 in __gnu_cxx::new_allocator<char>::allocate(unsigned long, void const*) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/ext/new_allocator.h:92
chromium#2 0x7f4730121d2c in std::string::_Rep::_S_create(unsigned long, unsigned long, std::allocator<char> const&) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:609
chromium#3 0x7f4730121f04 in std::string::_Rep::_M_clone(std::allocator<char> const&, unsigned long) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:631
chromium#4 0x7f473011ec48 in std::string::reserve(unsigned long) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:512
chromium#5 0x7f473011f391 in std::string::append(unsigned long, char) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:290
chromium#6 0x7f473011eaeb in std::string::resize(unsigned long, char) /build/buildd/gcc-4.6-4.6.3/build/x86_64-linux-gnu/libstdc++-v3/include/bits/basic_string.tcc:647
chromium#7 0x31d6642 in resize /usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/basic_string.h:749
chromium#8 0x31d6642 in base::Base64Encode(base::BasicStringPiece<std::string> const&, std::string*) base/base64.cc:13
chromium#9 0x34a1d05 in net::SdchManager::UrlSafeBase64Encode(std::string const&, std::string*) net/base/sdch_manager.cc:541
chromium#10 0x34a1124 in net::SdchManager::GenerateHash(std::string const&, std::string*, std::string*) net/base/sdch_manager.cc:508
chromium#11 0x349facb in net::SdchManager::AddSdchDictionary(std::string const&, GURL const&) net/base/sdch_manager.cc:363
chromium#12 0x803e06 in net::SdchManagerTest_CanUseMultipleDictionaries_Test::TestBody() net/base/sdch_manager_unittest.cc:403
[...]

> Make SdchManager per-profile.
> 
> This will both allow SDCH dictionaries to be cached (as they can use the
> cache associated with the profile) and will provide privacy protection
> between different profiles (the existing of a dictionary in one profile
> will not be leaked to another profile).
> 
> BUG=374914
> R=jar@chromium.org
> 
> Review URL: https://codereview.chromium.org/298063006

TBR=rdsmith@chromium.org

Review URL: https://codereview.chromium.org/331023002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@277185 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
viettrungluu@chromium.org committed Jun 14, 2014
1 parent b63c88b commit 13e4f34
Show file tree
Hide file tree
Showing 22 changed files with 311 additions and 381 deletions.
8 changes: 8 additions & 0 deletions chrome/browser/browser_process_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
#include "chrome/browser/metrics/thread_watcher.h"
#include "chrome/browser/net/chrome_net_log.h"
#include "chrome/browser/net/crl_set_fetcher.h"
#include "chrome/browser/net/sdch_dictionary_fetcher.h"
#include "chrome/browser/notifications/notification_ui_manager.h"
#include "chrome/browser/plugins/chrome_plugin_service_filter.h"
#include "chrome/browser/plugins/plugin_finder.h"
Expand Down Expand Up @@ -217,6 +218,13 @@ BrowserProcessImpl::~BrowserProcessImpl() {

void BrowserProcessImpl::StartTearDown() {
TRACE_EVENT0("shutdown", "BrowserProcessImpl::StartTearDown");
// We need to shutdown the SdchDictionaryFetcher as it regularly holds
// a pointer to a URLFetcher, and that URLFetcher (upon destruction) will do
// a PostDelayedTask onto the IO thread. This shutdown call will both discard
// any pending URLFetchers, and avoid creating any more.
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
base::Bind(&SdchDictionaryFetcher::Shutdown));

// We need to destroy the MetricsServicesManager, IntranetRedirectDetector,
// PromoResourceService, and SafeBrowsing ClientSideDetectionService (owned by
// the SafeBrowsingService) before the io_thread_ gets destroyed, since their
Expand Down
3 changes: 1 addition & 2 deletions chrome/browser/io_thread.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1052,8 +1052,7 @@ void IOThread::InitSystemRequestContextOnIOThread() {
ConstructSystemRequestContext(globals_, net_log_));

sdch_manager_->set_sdch_fetcher(
new SdchDictionaryFetcher(
sdch_manager_, system_url_request_context_getter_.get()));
new SdchDictionaryFetcher(system_url_request_context_getter_.get()));
}

void IOThread::UpdateDnsClientEnabled() {
Expand Down
14 changes: 7 additions & 7 deletions chrome/browser/net/sdch_dictionary_fetcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,22 @@
#include "net/url_request/url_request_status.h"

SdchDictionaryFetcher::SdchDictionaryFetcher(
net::SdchManager* manager,
net::URLRequestContextGetter* context)
: manager_(manager),
weak_factory_(this),
: weak_factory_(this),
task_is_pending_(false),
context_(context) {
DCHECK(CalledOnValidThread());
DCHECK(manager);
}

SdchDictionaryFetcher::~SdchDictionaryFetcher() {
DCHECK(CalledOnValidThread());
}

// static
void SdchDictionaryFetcher::Shutdown() {
net::SdchManager::Shutdown();
}

void SdchDictionaryFetcher::Schedule(const GURL& dictionary_url) {
DCHECK(CalledOnValidThread());

Expand Down Expand Up @@ -60,7 +62,6 @@ void SdchDictionaryFetcher::ScheduleDelayedRun() {
}

void SdchDictionaryFetcher::StartFetching() {
DCHECK(CalledOnValidThread());
DCHECK(task_is_pending_);
task_is_pending_ = false;

Expand All @@ -76,12 +77,11 @@ void SdchDictionaryFetcher::StartFetching() {

void SdchDictionaryFetcher::OnURLFetchComplete(
const net::URLFetcher* source) {
DCHECK(CalledOnValidThread());
if ((200 == source->GetResponseCode()) &&
(source->GetStatus().status() == net::URLRequestStatus::SUCCESS)) {
std::string data;
source->GetResponseAsString(&data);
manager_->AddSdchDictionary(data, source->GetURL());
net::SdchManager::Global()->AddSdchDictionary(data, source->GetURL());
}
current_fetch_.reset(NULL);
ScheduleDelayedRun();
Expand Down
12 changes: 5 additions & 7 deletions chrome/browser/net/sdch_dictionary_fetcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@ class SdchDictionaryFetcher
public net::SdchFetcher,
public base::NonThreadSafe {
public:
// Consumer must guarantee that the SdchManager pointer outlives
// this object. The current implementation guarantees this by
// the SdchManager owning this object.
SdchDictionaryFetcher(net::SdchManager* manager,
net::URLRequestContextGetter* context);
explicit SdchDictionaryFetcher(net::URLRequestContextGetter* context);
virtual ~SdchDictionaryFetcher();

// Stop fetching dictionaries, and abandon any current URLFetcheer operations
// so that the IO thread can be stopped.
static void Shutdown();

// Implementation of SdchFetcher class.
// This method gets the requested dictionary, and then calls back into the
// SdchManager class with the dictionary's text.
Expand All @@ -59,8 +59,6 @@ class SdchDictionaryFetcher
// completes (either successfully or with failure).
virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;

net::SdchManager* const manager_;

// A queue of URLs that are being used to download dictionaries.
std::queue<GURL> fetch_queue_;
// The currently outstanding URL fetch of a dicitonary.
Expand Down
15 changes: 0 additions & 15 deletions chrome/browser/profiles/off_the_record_profile_io_data.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#include "chrome/browser/net/chrome_net_log.h"
#include "chrome/browser/net/chrome_network_delegate.h"
#include "chrome/browser/net/chrome_url_request_context.h"
#include "chrome/browser/net/sdch_dictionary_fetcher.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/common/chrome_switches.h"
#include "chrome/common/pref_names.h"
Expand All @@ -29,7 +28,6 @@
#include "content/public/browser/resource_context.h"
#include "extensions/common/constants.h"
#include "extensions/common/extension.h"
#include "net/base/sdch_manager.h"
#include "net/ftp/ftp_network_layer.h"
#include "net/http/http_cache.h"
#include "net/http/http_network_session.h"
Expand Down Expand Up @@ -246,19 +244,6 @@ void OffTheRecordProfileIOData::InitializeInternal(
ftp_factory_.get());
main_context->set_job_factory(main_job_factory_.get());

// Setup the SDCHManager for this profile.
sdch_manager_.reset(new net::SdchManager);
sdch_manager_->set_sdch_fetcher(
new SdchDictionaryFetcher(
sdch_manager_.get(),
// SdchDictionaryFetcher takes a reference to the Getter, and
// hence implicitly takes ownership.
new net::TrivialURLRequestContextGetter(
main_context,
content::BrowserThread::GetMessageLoopProxyForThread(
content::BrowserThread::IO))));
main_context->set_sdch_manager(sdch_manager_.get());

#if defined(ENABLE_EXTENSIONS)
InitializeExtensionsRequestContext(profile_params);
#endif
Expand Down
3 changes: 0 additions & 3 deletions chrome/browser/profiles/off_the_record_profile_io_data.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ class Profile;
namespace net {
class FtpTransactionFactory;
class HttpTransactionFactory;
class SdchManager;
} // namespace net

// OffTheRecordProfile owns a OffTheRecordProfileIOData::Handle, which holds a
Expand Down Expand Up @@ -148,8 +147,6 @@ class OffTheRecordProfileIOData : public ProfileIOData {
mutable scoped_ptr<net::URLRequestJobFactory> main_job_factory_;
mutable scoped_ptr<net::URLRequestJobFactory> extensions_job_factory_;

mutable scoped_ptr<net::SdchManager> sdch_manager_;

DISALLOW_COPY_AND_ASSIGN(OffTheRecordProfileIOData);
};

Expand Down
15 changes: 0 additions & 15 deletions chrome/browser/profiles/profile_impl_io_data.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
#include "chrome/browser/net/cookie_store_util.h"
#include "chrome/browser/net/http_server_properties_manager.h"
#include "chrome/browser/net/predictor.h"
#include "chrome/browser/net/sdch_dictionary_fetcher.h"
#include "chrome/browser/net/sqlite_server_bound_cert_store.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/common/chrome_constants.h"
Expand All @@ -42,7 +41,6 @@
#include "extensions/browser/extension_protocols.h"
#include "extensions/common/constants.h"
#include "net/base/cache_type.h"
#include "net/base/sdch_manager.h"
#include "net/ftp/ftp_network_layer.h"
#include "net/http/http_cache.h"
#include "net/ssl/server_bound_cert_service.h"
Expand Down Expand Up @@ -524,19 +522,6 @@ void ProfileImplIOData::InitializeInternal(
InitializeExtensionsRequestContext(profile_params);
#endif

// Setup the SDCHManager for this profile.
sdch_manager_.reset(new net::SdchManager);
sdch_manager_->set_sdch_fetcher(
new SdchDictionaryFetcher(
sdch_manager_.get(),
// SdchDictionaryFetcher takes a reference to the Getter, and
// hence implicitly takes ownership.
new net::TrivialURLRequestContextGetter(
main_context,
content::BrowserThread::GetMessageLoopProxyForThread(
content::BrowserThread::IO))));
main_context->set_sdch_manager(sdch_manager_.get());

// Create a media request context based on the main context, but using a
// media cache. It shares the same job factory as the main context.
StoragePartitionDescriptor details(profile_path_, false);
Expand Down
3 changes: 0 additions & 3 deletions chrome/browser/profiles/profile_impl_io_data.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ namespace net {
class FtpTransactionFactory;
class HttpServerProperties;
class HttpTransactionFactory;
class SDCHManager;
} // namespace net

namespace quota {
Expand Down Expand Up @@ -235,8 +234,6 @@ class ProfileImplIOData : public ProfileIOData {
mutable scoped_ptr<domain_reliability::DomainReliabilityMonitor>
domain_reliability_monitor_;

mutable scoped_ptr<net::SdchManager> sdch_manager_;

// Parameters needed for isolated apps.
base::FilePath profile_path_;
int app_cache_max_size_;
Expand Down
Loading

0 comments on commit 13e4f34

Please sign in to comment.