From a5762c39fb23ce4aacaa01b4f053c5f4f8f70b79 Mon Sep 17 00:00:00 2001 From: Ivan Efremov Date: Wed, 29 Jul 2020 22:12:45 +0700 Subject: [PATCH] 10907: Fix threading for certain adblock service methods. Fix https://github.com/brave/brave-browser/issues/10907 Some functionality of adblock service must be used only on certain TaskRunner, so this fixes a couple of methods that were previously called from UI thread. It also required making the corresponding extension shields API functions async. I also did some small code style improvements. --- browser/extensions/api/brave_shields_api.cc | 92 +++++++++++++------ browser/extensions/api/brave_shields_api.h | 13 +++ .../browser/ad_block_base_service.cc | 9 +- .../ad_block_regional_service_manager.cc | 22 ++--- .../ad_block_regional_service_manager.h | 1 - .../browser/ad_block_service_helper.cc | 15 ++- .../browser/ad_block_service_helper.h | 2 +- 7 files changed, 95 insertions(+), 59 deletions(-) diff --git a/browser/extensions/api/brave_shields_api.cc b/browser/extensions/api/brave_shields_api.cc index f86ac10de024..050a63aeee79 100644 --- a/browser/extensions/api/brave_shields_api.cc +++ b/browser/extensions/api/brave_shields_api.cc @@ -54,42 +54,55 @@ BraveShieldsUrlCosmeticResourcesFunction::Run() { std::unique_ptr params( brave_shields::UrlCosmeticResources::Params::Create(*args_)); EXTENSION_FUNCTION_VALIDATE(params.get()); + g_brave_browser_process->ad_block_service()->GetTaskRunner() + ->PostTaskAndReplyWithResult( + FROM_HERE, + base::BindOnce(&BraveShieldsUrlCosmeticResourcesFunction:: + GetUrlCosmeticResourcesOnTaskRunner, + this, params->url), + base::BindOnce(&BraveShieldsUrlCosmeticResourcesFunction:: + GetUrlCosmeticResourcesOnUI, + this)); + return RespondLater(); +} +std::unique_ptr BraveShieldsUrlCosmeticResourcesFunction:: + GetUrlCosmeticResourcesOnTaskRunner(const std::string& url) { base::Optional resources = g_brave_browser_process-> - ad_block_service()->UrlCosmeticResources(params->url); + ad_block_service()->UrlCosmeticResources(url); if (!resources || !resources->is_dict()) { - return RespondNow(Error( - "Url-specific cosmetic resources could not be returned")); + return std::unique_ptr(); } base::Optional regional_resources = g_brave_browser_process-> - ad_block_regional_service_manager()-> - UrlCosmeticResources(params->url); + ad_block_regional_service_manager()->UrlCosmeticResources(url); if (regional_resources && regional_resources->is_dict()) { ::brave_shields::MergeResourcesInto( - &*resources, - &*regional_resources, - false); + std::move(*regional_resources), &*resources, /*force_hide=*/false); } base::Optional custom_resources = g_brave_browser_process-> - ad_block_custom_filters_service()-> - UrlCosmeticResources(params->url); + ad_block_custom_filters_service()->UrlCosmeticResources(url); if (custom_resources && custom_resources->is_dict()) { ::brave_shields::MergeResourcesInto( - &*resources, - &*custom_resources, - true); + std::move(*custom_resources), &*resources, /*force_hide=*/true); } auto result_list = std::make_unique(); - result_list->Append(std::move(*resources)); + return result_list; +} - return RespondNow(ArgumentList(std::move(result_list))); +void BraveShieldsUrlCosmeticResourcesFunction::GetUrlCosmeticResourcesOnUI( + std::unique_ptr resources) { + if (!resources) { + Respond(Error("Url-specific cosmetic resources could not be returned")); + return; + } + Respond(ArgumentList(std::move(resources))); } ExtensionFunction::ResponseAction @@ -97,17 +110,34 @@ BraveShieldsHiddenClassIdSelectorsFunction::Run() { std::unique_ptr params( brave_shields::HiddenClassIdSelectors::Params::Create(*args_)); EXTENSION_FUNCTION_VALIDATE(params.get()); + g_brave_browser_process->ad_block_service()->GetTaskRunner() + ->PostTaskAndReplyWithResult( + FROM_HERE, + base::BindOnce(&BraveShieldsHiddenClassIdSelectorsFunction:: + GetHiddenClassIdSelectorsOnTaskRunner, + this, params->classes, params->ids, + params->exceptions), + base::BindOnce(&BraveShieldsHiddenClassIdSelectorsFunction:: + GetHiddenClassIdSelectorsOnUI, + this)); + return RespondLater(); +} +std::unique_ptr BraveShieldsHiddenClassIdSelectorsFunction:: + GetHiddenClassIdSelectorsOnTaskRunner( + const std::vector& classes, + const std::vector& ids, + const std::vector& exceptions) { base::Optional hide_selectors = g_brave_browser_process-> - ad_block_service()->HiddenClassIdSelectors(params->classes, - params->ids, - params->exceptions); + ad_block_service()->HiddenClassIdSelectors(classes, ids, exceptions); base::Optional regional_selectors = g_brave_browser_process-> ad_block_regional_service_manager()-> - HiddenClassIdSelectors(params->classes, - params->ids, - params->exceptions); + HiddenClassIdSelectors(classes, ids, exceptions); + + base::Optional custom_selectors = g_brave_browser_process-> + ad_block_custom_filters_service()-> + HiddenClassIdSelectors(classes, ids, exceptions); if (hide_selectors && hide_selectors->is_list()) { if (regional_selectors && regional_selectors->is_list()) { @@ -121,18 +151,20 @@ BraveShieldsHiddenClassIdSelectorsFunction::Run() { hide_selectors = std::move(regional_selectors); } - base::Optional custom_selectors = g_brave_browser_process-> - ad_block_custom_filters_service()-> - HiddenClassIdSelectors(params->classes, - params->ids, - params->exceptions); - auto result_list = std::make_unique(); + if (hide_selectors && hide_selectors->is_list()) { + result_list->Append(std::move(*hide_selectors)); + } + if (custom_selectors && custom_selectors->is_list()) { + result_list->Append(std::move(*custom_selectors)); + } - result_list->Append(std::move(*hide_selectors)); - result_list->Append(std::move(*custom_selectors)); + return result_list; +} - return RespondNow(ArgumentList(std::move(result_list))); +void BraveShieldsHiddenClassIdSelectorsFunction:: + GetHiddenClassIdSelectorsOnUI(std::unique_ptr selectors) { + Respond(ArgumentList(std::move(selectors))); } diff --git a/browser/extensions/api/brave_shields_api.h b/browser/extensions/api/brave_shields_api.h index b3532ede276c..b4e52e00fc1e 100644 --- a/browser/extensions/api/brave_shields_api.h +++ b/browser/extensions/api/brave_shields_api.h @@ -19,6 +19,11 @@ class BraveShieldsUrlCosmeticResourcesFunction : public ExtensionFunction { ~BraveShieldsUrlCosmeticResourcesFunction() override {} ResponseAction Run() override; + + private: + std::unique_ptr GetUrlCosmeticResourcesOnTaskRunner( + const std::string& url); + void GetUrlCosmeticResourcesOnUI(std::unique_ptr resources); }; class BraveShieldsHiddenClassIdSelectorsFunction : public ExtensionFunction { @@ -29,6 +34,14 @@ class BraveShieldsHiddenClassIdSelectorsFunction : public ExtensionFunction { ~BraveShieldsHiddenClassIdSelectorsFunction() override {} ResponseAction Run() override; + + private: + std::unique_ptr GetHiddenClassIdSelectorsOnTaskRunner( + const std::vector& classes, + const std::vector& ids, + const std::vector& exceptions); + void GetHiddenClassIdSelectorsOnUI( + std::unique_ptr selectors); }; class BraveShieldsAllowScriptsOnceFunction : public ExtensionFunction { diff --git a/components/brave_shields/browser/ad_block_base_service.cc b/components/brave_shields/browser/ad_block_base_service.cc index a7cf99c7e280..ff59558fd123 100644 --- a/components/brave_shields/browser/ad_block_base_service.cc +++ b/components/brave_shields/browser/ad_block_base_service.cc @@ -195,18 +195,17 @@ bool AdBlockBaseService::TagExists(const std::string& tag) { base::Optional AdBlockBaseService::UrlCosmeticResources( const std::string& url) { - return base::JSONReader::Read( - this->ad_block_client_->urlCosmeticResources(url)); + DCHECK(GetTaskRunner()->RunsTasksInCurrentSequence()); + return base::JSONReader::Read(ad_block_client_->urlCosmeticResources(url)); } base::Optional AdBlockBaseService::HiddenClassIdSelectors( const std::vector& classes, const std::vector& ids, const std::vector& exceptions) { + DCHECK(GetTaskRunner()->RunsTasksInCurrentSequence()); return base::JSONReader::Read( - this->ad_block_client_->hiddenClassIdSelectors(classes, - ids, - exceptions)); + ad_block_client_->hiddenClassIdSelectors(classes, ids, exceptions)); } void AdBlockBaseService::GetDATFileData(const base::FilePath& dat_file_path) { diff --git a/components/brave_shields/browser/ad_block_regional_service_manager.cc b/components/brave_shields/browser/ad_block_regional_service_manager.cc index b0ee0116ea9b..64d6c004600b 100644 --- a/components/brave_shields/browser/ad_block_regional_service_manager.cc +++ b/components/brave_shields/browser/ad_block_regional_service_manager.cc @@ -185,19 +185,20 @@ void AdBlockRegionalServiceManager::EnableFilterList(const std::string& uuid, base::Optional AdBlockRegionalServiceManager::UrlCosmeticResources( const std::string& url) { - auto it = this->regional_services_.begin(); - if (it == this->regional_services_.end()) { + base::AutoLock lock(regional_services_lock_); + auto it = regional_services_.begin(); + if (it == regional_services_.end()) { return base::Optional(); } base::Optional first_value = it->second->UrlCosmeticResources(url); - for ( ; it != this->regional_services_.end(); it++) { + for ( ; it != regional_services_.end(); it++) { base::Optional next_value = it->second->UrlCosmeticResources(url); if (first_value) { if (next_value) { - MergeResourcesInto(&*first_value, &*next_value, false); + MergeResourcesInto(std::move(*next_value), &*first_value, false); } } else { first_value = std::move(next_value); @@ -212,14 +213,15 @@ AdBlockRegionalServiceManager::HiddenClassIdSelectors( const std::vector& classes, const std::vector& ids, const std::vector& exceptions) { - auto it = this->regional_services_.begin(); - if (it == this->regional_services_.end()) { + base::AutoLock lock(regional_services_lock_); + auto it = regional_services_.begin(); + if (it == regional_services_.end()) { return base::Optional(); } base::Optional first_value = it->second->HiddenClassIdSelectors(classes, ids, exceptions); - for ( ; it != this->regional_services_.end(); it++) { + for ( ; it != regional_services_.end(); it++) { base::Optional next_value = it->second->HiddenClassIdSelectors(classes, ids, exceptions); if (first_value && first_value->is_list()) { @@ -238,12 +240,6 @@ AdBlockRegionalServiceManager::HiddenClassIdSelectors( return first_value; } -bool AdBlockRegionalServiceManager::IsSupportedLocale( - const std::string& locale) { - return (brave_shields::FindAdBlockFilterListByLocale( - regional_catalog_, locale) != regional_catalog_.end()); -} - void AdBlockRegionalServiceManager::SetRegionalCatalog( std::vector catalog) { regional_catalog_ = std::move(catalog); diff --git a/components/brave_shields/browser/ad_block_regional_service_manager.h b/components/brave_shields/browser/ad_block_regional_service_manager.h index eb56acccf051..42dd84c73779 100644 --- a/components/brave_shields/browser/ad_block_regional_service_manager.h +++ b/components/brave_shields/browser/ad_block_regional_service_manager.h @@ -40,7 +40,6 @@ class AdBlockRegionalServiceManager { explicit AdBlockRegionalServiceManager(BraveComponent::Delegate* delegate); ~AdBlockRegionalServiceManager(); - bool IsSupportedLocale(const std::string& locale); std::unique_ptr GetRegionalLists(); void SetRegionalCatalog(std::vector catalog); diff --git a/components/brave_shields/browser/ad_block_service_helper.cc b/components/brave_shields/browser/ad_block_service_helper.cc index 9acfca9e6f0d..4ba0044d6c17 100644 --- a/components/brave_shields/browser/ad_block_service_helper.cc +++ b/components/brave_shields/browser/ad_block_service_helper.cc @@ -120,10 +120,7 @@ std::vector RegionalCatalogFromJSON( // If `force_hide` is true, the contents of `from`'s `hide_selectors` field // will be moved into a possibly new field of `into` called // `force_hide_selectors`. -void MergeResourcesInto( - base::Value* into, - base::Value* from, - bool force_hide) { +void MergeResourcesInto(base::Value from, base::Value* into, bool force_hide) { base::Value* resources_hide_selectors = nullptr; if (force_hide) { resources_hide_selectors = into->FindKey("force_hide_selectors"); @@ -135,7 +132,7 @@ void MergeResourcesInto( resources_hide_selectors = into->FindKey("hide_selectors"); } base::Value* from_resources_hide_selectors = - from->FindKey("hide_selectors"); + from.FindKey("hide_selectors"); if (resources_hide_selectors && from_resources_hide_selectors) { for (auto i = from_resources_hide_selectors->GetList().begin(); i < from_resources_hide_selectors->GetList().end(); @@ -146,7 +143,7 @@ void MergeResourcesInto( base::Value* resources_style_selectors = into->FindKey("style_selectors"); base::Value* from_resources_style_selectors = - from->FindKey("style_selectors"); + from.FindKey("style_selectors"); if (resources_style_selectors && from_resources_style_selectors) { for (auto i : from_resources_style_selectors->DictItems()) { base::Value* resources_entry = @@ -164,7 +161,7 @@ void MergeResourcesInto( } base::Value* resources_exceptions = into->FindKey("exceptions"); - base::Value* from_resources_exceptions = from->FindKey("exceptions"); + base::Value* from_resources_exceptions = from.FindKey("exceptions"); if (resources_exceptions && from_resources_exceptions) { for (auto i = from_resources_exceptions->GetList().begin(); i < from_resources_exceptions->GetList().end(); @@ -175,7 +172,7 @@ void MergeResourcesInto( base::Value* resources_injected_script = into->FindKey("injected_script"); base::Value* from_resources_injected_script = - from->FindKey("injected_script"); + from.FindKey("injected_script"); if (resources_injected_script && from_resources_injected_script) { *resources_injected_script = base::Value( resources_injected_script->GetString() @@ -185,7 +182,7 @@ void MergeResourcesInto( base::Value* resources_generichide = into->FindKey("generichide"); base::Value* from_resources_generichide = - from->FindKey("generichide"); + from.FindKey("generichide"); if (from_resources_generichide) { if (from_resources_generichide->GetBool()) { *resources_generichide = base::Value(true); diff --git a/components/brave_shields/browser/ad_block_service_helper.h b/components/brave_shields/browser/ad_block_service_helper.h index 06236f6f2e8e..93a741b75a33 100644 --- a/components/brave_shields/browser/ad_block_service_helper.h +++ b/components/brave_shields/browser/ad_block_service_helper.h @@ -24,7 +24,7 @@ std::vector::const_iterator FindAdBlockFilterListByLocale( std::vector RegionalCatalogFromJSON( const std::string& catalog_json); -void MergeResourcesInto(base::Value* into, base::Value* from, bool force_hide); +void MergeResourcesInto(base::Value from, base::Value* into, bool force_hide); } // namespace brave_shields