Merge pull request #6330 from brave/ie_cosmetic_crash_1.12

10907: Fix threading for certain adblock service methods (1.12)

browser/extensions/api/brave_shields_api.cc

@@ -5,8 +5,6 @@
 
 #include "brave/browser/extensions/api/brave_shields_api.h"
 
-#include <memory>
-#include <string>
 #include <utility>
 
 #include "base/strings/string_number_conversions.h"
@@ -54,60 +52,90 @@ BraveShieldsUrlCosmeticResourcesFunction::Run() {
   std::unique_ptr<brave_shields::UrlCosmeticResources::Params> 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<base::ListValue> BraveShieldsUrlCosmeticResourcesFunction::
+    GetUrlCosmeticResourcesOnTaskRunner(const std::string& url) {
   base::Optional<base::Value> 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::ListValue>();
   }
 
   base::Optional<base::Value> 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<base::Value> 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<base::ListValue>();
-
   result_list->Append(std::move(*resources));
+  return result_list;
+}
 
-  return RespondNow(ArgumentList(std::move(result_list)));
+void BraveShieldsUrlCosmeticResourcesFunction::GetUrlCosmeticResourcesOnUI(
+    std::unique_ptr<base::ListValue> resources) {
+  if (!resources) {
+    Respond(Error("Url-specific cosmetic resources could not be returned"));
+    return;
+  }
+  Respond(ArgumentList(std::move(resources)));
 }
 
 ExtensionFunction::ResponseAction
 BraveShieldsHiddenClassIdSelectorsFunction::Run() {
   std::unique_ptr<brave_shields::HiddenClassIdSelectors::Params> 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<base::ListValue> BraveShieldsHiddenClassIdSelectorsFunction::
+    GetHiddenClassIdSelectorsOnTaskRunner(
+        const std::vector<std::string>& classes,
+        const std::vector<std::string>& ids,
+        const std::vector<std::string>& exceptions) {
   base::Optional<base::Value> 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<base::Value> regional_selectors = g_brave_browser_process->
       ad_block_regional_service_manager()->
-        HiddenClassIdSelectors(params->classes,
-                               params->ids,
-                               params->exceptions);
+          HiddenClassIdSelectors(classes, ids, exceptions);
+
+  base::Optional<base::Value> 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 +149,20 @@ BraveShieldsHiddenClassIdSelectorsFunction::Run() {
     hide_selectors = std::move(regional_selectors);
   }
 
-  base::Optional<base::Value> custom_selectors = g_brave_browser_process->
-      ad_block_custom_filters_service()->
-          HiddenClassIdSelectors(params->classes,
-                                 params->ids,
-                                 params->exceptions);
-
   auto result_list = std::make_unique<base::ListValue>();
+  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<base::ListValue> selectors) {
+  Respond(ArgumentList(std::move(selectors)));
 }
 
 

browser/extensions/api/brave_shields_api.h

@@ -6,6 +6,10 @@
 #ifndef BRAVE_BROWSER_EXTENSIONS_API_BRAVE_SHIELDS_API_H_
 #define BRAVE_BROWSER_EXTENSIONS_API_BRAVE_SHIELDS_API_H_
 
+#include <memory>
+#include <string>
+#include <vector>
+
 #include "extensions/browser/extension_function.h"
 
 namespace extensions {
@@ -19,6 +23,11 @@ class BraveShieldsUrlCosmeticResourcesFunction : public ExtensionFunction {
   ~BraveShieldsUrlCosmeticResourcesFunction() override {}
 
   ResponseAction Run() override;
+
+ private:
+  std::unique_ptr<base::ListValue> GetUrlCosmeticResourcesOnTaskRunner(
+      const std::string& url);
+  void GetUrlCosmeticResourcesOnUI(std::unique_ptr<base::ListValue> resources);
 };
 
 class BraveShieldsHiddenClassIdSelectorsFunction : public ExtensionFunction {
@@ -29,6 +38,14 @@ class BraveShieldsHiddenClassIdSelectorsFunction : public ExtensionFunction {
   ~BraveShieldsHiddenClassIdSelectorsFunction() override {}
 
   ResponseAction Run() override;
+
+ private:
+  std::unique_ptr<base::ListValue> GetHiddenClassIdSelectorsOnTaskRunner(
+      const std::vector<std::string>& classes,
+      const std::vector<std::string>& ids,
+      const std::vector<std::string>& exceptions);
+  void GetHiddenClassIdSelectorsOnUI(
+      std::unique_ptr<base::ListValue> selectors);
 };
 
 class BraveShieldsAllowScriptsOnceFunction : public ExtensionFunction {

components/brave_shields/browser/ad_block_base_service.cc

@@ -199,18 +199,17 @@ bool AdBlockBaseService::TagExists(const std::string& tag) {
 
 base::Optional<base::Value> 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<base::Value> AdBlockBaseService::HiddenClassIdSelectors(
         const std::vector<std::string>& classes,
         const std::vector<std::string>& ids,
         const std::vector<std::string>& 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) {

components/brave_shields/browser/ad_block_regional_service_manager.cc

@@ -194,19 +194,20 @@ void AdBlockRegionalServiceManager::EnableFilterList(const std::string& uuid,
 base::Optional<base::Value>
 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::Value>();
   }
   base::Optional<base::Value> first_value =
       it->second->UrlCosmeticResources(url);
 
-  for ( ; it != this->regional_services_.end(); it++) {
+  for ( ; it != regional_services_.end(); it++) {
     base::Optional<base::Value> 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);
@@ -221,14 +222,15 @@ AdBlockRegionalServiceManager::HiddenClassIdSelectors(
         const std::vector<std::string>& classes,
         const std::vector<std::string>& ids,
         const std::vector<std::string>& 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::Value>();
   }
   base::Optional<base::Value> first_value =
       it->second->HiddenClassIdSelectors(classes, ids, exceptions);
 
-  for ( ; it != this->regional_services_.end(); it++) {
+  for ( ; it != regional_services_.end(); it++) {
     base::Optional<base::Value> next_value =
         it->second->HiddenClassIdSelectors(classes, ids, exceptions);
     if (first_value && first_value->is_list()) {

components/brave_shields/browser/ad_block_service_helper.cc

@@ -52,10 +52,7 @@ std::vector<FilterList>::const_iterator FindAdBlockFilterListByLocale(
 // 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");
@@ -67,7 +64,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();
@@ -78,7 +75,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 =
@@ -96,7 +93,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();
@@ -107,7 +104,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()
@@ -117,7 +114,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);

components/brave_shields/browser/ad_block_service_helper.h

@@ -21,7 +21,7 @@ std::vector<adblock::FilterList>::const_iterator FindAdBlockFilterListByLocale(
     const std::vector<adblock::FilterList>& region_lists,
     const std::string& locale);
 
-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
 

components/brave_shields/browser/cosmetic_merge_unittest.cc

@@ -32,7 +32,7 @@ class CosmeticResourceMergeTest : public testing::Test {
         base::JSONReader::Read(expected);
     ASSERT_TRUE(expected_val);
 
-    MergeResourcesInto(&*a_val, &*b_val, force_hide);
+    MergeResourcesInto(std::move(b_val.value()), &*a_val, force_hide);
 
     ASSERT_EQ(*a_val, *expected_val);
   }