From 58f99af3f31077fe0f3ed116969a3f63e09a1a7b Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Thu, 30 Aug 2018 22:24:58 -0400 Subject: [PATCH 1/3] Add sequence checker for HTTPSE --- .../brave_shields/browser/https_everywhere_service.cc | 7 +++++-- .../brave_shields/browser/https_everywhere_service.h | 2 ++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/components/brave_shields/browser/https_everywhere_service.cc b/components/brave_shields/browser/https_everywhere_service.cc index f23373c5cbdb..094776edb800 100644 --- a/components/brave_shields/browser/https_everywhere_service.cc +++ b/components/brave_shields/browser/https_everywhere_service.cc @@ -83,6 +83,7 @@ std::string HTTPSEverywhereService::g_https_everywhere_component_base64_public_k kHTTPSEverywhereComponentBase64PublicKey); HTTPSEverywhereService::HTTPSEverywhereService() : level_db_(nullptr) { + DETACH_FROM_SEQUENCE(sequence_checker_); } HTTPSEverywhereService::~HTTPSEverywhereService() { @@ -103,6 +104,7 @@ bool HTTPSEverywhereService::Init() { } void HTTPSEverywhereService::InitDB(const base::FilePath& install_dir) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); base::FilePath zip_db_file_path = install_dir.AppendASCII(DAT_FILE_VERSION).AppendASCII(DAT_FILE); base::FilePath unzipped_level_db_path = zip_db_file_path.RemoveExtension(); @@ -142,6 +144,7 @@ void HTTPSEverywhereService::OnComponentReady( bool HTTPSEverywhereService::GetHTTPSURL( const GURL* url, const uint64_t& request_identifier, std::string& new_url) { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); base::AssertBlockingAllowed(); if (!IsInitialized() || url->scheme() == url::kHttpsScheme) { return false; @@ -358,8 +361,8 @@ std::string HTTPSEverywhereService::CorrecttoRuleToRE2Engine( return correctedto; } -void HTTPSEverywhereService::CloseDatabase() -{ +void HTTPSEverywhereService::CloseDatabase() { + DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); if (level_db_) { delete level_db_; level_db_ = nullptr; diff --git a/components/brave_shields/browser/https_everywhere_service.h b/components/brave_shields/browser/https_everywhere_service.h index 529fbdbb8770..48418f4bc2a5 100644 --- a/components/brave_shields/browser/https_everywhere_service.h +++ b/components/brave_shields/browser/https_everywhere_service.h @@ -13,6 +13,7 @@ #include #include "base/files/file_path.h" +#include "base/sequence_checker.h" #include "brave/components/brave_shields/browser/base_brave_shields_service.h" #include "brave/components/brave_shields/browser/https_everywhere_recently_used_cache.h" #include "content/public/common/resource_type.h" @@ -89,6 +90,7 @@ class HTTPSEverywhereService : public BaseBraveShieldsService { HTTPSERecentlyUsedCache recently_used_cache_; leveldb::DB* level_db_; + SEQUENCE_CHECKER(sequence_checker_); DISALLOW_COPY_AND_ASSIGN(HTTPSEverywhereService); }; From 0532a5789d1bc1defc81c0a6a11a3d551bff9bea Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Thu, 30 Aug 2018 22:35:35 -0400 Subject: [PATCH 2/3] Use a single sequenced task runner for HTTPSEverywehre Otherwise we're creating lots of different threads and accessing things for the DB on different possibly overlapping threads I think this also fixes: https://github.com/brave/brave-browser/issues/707 https://github.com/brave/brave-browser/issues/708 https://github.com/brave/brave-browser/issues/739 https://github.com/brave/brave-browser/issues/797 https://github.com/brave/brave-browser/issues/761 https://github.com/brave/brave-browser/issues/762 https://github.com/brave/brave-browser/issues/796 https://github.com/brave/brave-browser/issues/797 https://github.com/brave/brave-browser/issues/798 https://github.com/brave/brave-browser/issues/861 --- .../brave_httpse_network_delegate_helper.cc | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/browser/net/brave_httpse_network_delegate_helper.cc b/browser/net/brave_httpse_network_delegate_helper.cc index 5c0d53ea16f2..355a67773009 100644 --- a/browser/net/brave_httpse_network_delegate_helper.cc +++ b/browser/net/brave_httpse_network_delegate_helper.cc @@ -81,18 +81,14 @@ int OnBeforeURLRequest_HttpsePreFileWork( GetHTTPSURLFromCacheOnly(&request->url(), request->identifier(), ctx->new_url_spec)) { ctx->request_url = request->url(); - - scoped_refptr task_runner = - base::CreateSequencedTaskRunnerWithTraits({base::MayBlock(), - base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN}); - task_runner->PostTaskAndReply(FROM_HERE, - base::Bind(OnBeforeURLRequest_HttpseFileWork, - base::Unretained(request), new_url, ctx), - base::Bind(base::IgnoreResult( - &OnBeforeURLRequest_HttpsePostFileWork), - base::Unretained(request), - new_url, next_callback, ctx) - ); + g_brave_browser_process->https_everywhere_service()-> + GetTaskRunner()->PostTaskAndReply(FROM_HERE, + base::Bind(OnBeforeURLRequest_HttpseFileWork, + base::Unretained(request), new_url, ctx), + base::Bind(base::IgnoreResult( + &OnBeforeURLRequest_HttpsePostFileWork), + base::Unretained(request), + new_url, next_callback, ctx)); return net::ERR_IO_PENDING; } else { if (!ctx->new_url_spec.empty()) { From 259af5b6da7cd0f40172148a3b1e3f8eebea54a7 Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Thu, 30 Aug 2018 23:43:25 -0400 Subject: [PATCH 3/3] Ensure GetHTTPSURL only called once level_db_ is initialized --- components/brave_shields/browser/https_everywhere_service.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/components/brave_shields/browser/https_everywhere_service.cc b/components/brave_shields/browser/https_everywhere_service.cc index 094776edb800..5a503e0d3d70 100644 --- a/components/brave_shields/browser/https_everywhere_service.cc +++ b/components/brave_shields/browser/https_everywhere_service.cc @@ -123,6 +123,7 @@ void HTTPSEverywhereService::InitDB(const base::FilePath& install_dir) { unzipped_level_db_path.AsUTF8Unsafe(), &level_db_); if (!status.ok() || !level_db_) { + level_db_ = nullptr; LOG(ERROR) << "Level db open error " << unzipped_level_db_path.value().c_str() << ", error: " << status.ToString(); @@ -146,7 +147,7 @@ bool HTTPSEverywhereService::GetHTTPSURL( std::string& new_url) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); base::AssertBlockingAllowed(); - if (!IsInitialized() || url->scheme() == url::kHttpsScheme) { + if (!IsInitialized() || !level_db_ || url->scheme() == url::kHttpsScheme) { return false; } if (!ShouldHTTPSERedirect(request_identifier)) {