Merge pull request #12038 from brave/cosmetic-filtering-fixes

Cosmetic filtering fixes

browser/brave_shields/ad_block_service_browsertest.cc

@@ -2008,7 +2008,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CosmeticFilteringHide1pContent) {
 }
 
 // Test cosmetic filtering on elements added dynamically
-IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, DISABLED_CosmeticFilteringDynamic) {
+IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CosmeticFilteringDynamic) {
   UpdateAdBlockInstanceWithRules("##.blockme");
 
   WaitForBraveExtensionShieldsDataReady();
@@ -2021,7 +2021,8 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, DISABLED_CosmeticFilteringDynamic) {
       browser()->tab_strip_model()->GetActiveWebContents();
 
   auto result_first = EvalJs(contents,
-                             R"(async function waitCSSSelector() {
+                             R"(addElementsDynamically();
+        async function waitCSSSelector() {
           if (await checkSelector('.blockme', 'display', 'none')) {
             window.domAutomationController.send(true);
           } else {
@@ -2221,6 +2222,35 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CosmeticFilteringIframeScriptlet) {
   ASSERT_EQ(true, EvalJs(contents, "show_ad"));
 }
 
+// Test cosmetic filtering on an element that already has an `!important`
+// marker on its `display` style.
+IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,
+                       CosmeticFilteringOverridesImportant) {
+  UpdateAdBlockInstanceWithRules("###inline-block-important");
+
+  WaitForBraveExtensionShieldsDataReady();
+
+  GURL tab_url =
+      embedded_test_server()->GetURL("b.com", "/cosmetic_filtering.html");
+  ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), tab_url));
+
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  auto result_first = EvalJs(contents,
+                             R"(async function waitCSSSelector() {
+          if (await checkSelector('#inline-block-important', 'display', 'none')) {
+            window.domAutomationController.send(true);
+          } else {
+            console.error('still waiting for css selector');
+            setTimeout(waitCSSSelector, 200);
+          }
+        } waitCSSSelector())",
+                             content::EXECUTE_SCRIPT_USE_MANUAL_REPLY);
+  ASSERT_TRUE(result_first.error.empty());
+  EXPECT_EQ(base::Value(true), result_first.value);
+}
+
 class DefaultCookieListFlagEnabledTest : public AdBlockServiceTest {
  public:
   DefaultCookieListFlagEnabledTest() {

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc

@@ -77,88 +77,24 @@ const char kCosmeticFilteringInitScript[] =
 
 const char kHideSelectorsInjectScript[] =
     R"((function() {
-          let nextIndex =
-              window.content_cosmetic.cosmeticStyleSheet.rules.length;
           const selectors = %s;
           selectors.forEach(selector => {
             if ((typeof selector === 'string') &&
                 (window.content_cosmetic.hide1pContent ||
                 !window.content_cosmetic.allSelectorsToRules.has(selector))) {
               let rule = selector + '{display:none !important;}';
-              window.content_cosmetic.cosmeticStyleSheet.insertRule(
-                `${rule}`, nextIndex);
+              let ruleIndex = 0;
               if (!window.content_cosmetic.hide1pContent) {
+                ruleIndex = window.content_cosmetic.nextRuleIndex;
+                window.content_cosmetic.nextRuleIndex++;
                 window.content_cosmetic.allSelectorsToRules.set(
-                  selector, nextIndex);
+                  selector, ruleIndex);
                 window.content_cosmetic.firstRunQueue.add(selector);
               }
-              nextIndex++;
+              window.cf_worker.injectStylesheet(rule, ruleIndex);
             }
           });
-          if (!document.adoptedStyleSheets.includes(
-              window.content_cosmetic.cosmeticStyleSheet)) {
-            document.adoptedStyleSheets =
-              [window.content_cosmetic.cosmeticStyleSheet,
-                ...document.adoptedStyleSheets];
-          };
-        })();)";
-
-const char kForceHideSelectorsInjectScript[] =
-    R"((function() {
-          let nextIndex =
-              window.content_cosmetic.cosmeticStyleSheet.rules.length;
-          const selectors = %s;
-          selectors.forEach(selector => {
-            if (typeof selector === 'string') {
-              let rule = selector + '{display:none !important;}';
-              window.content_cosmetic.cosmeticStyleSheet.insertRule(
-                `${rule}`, nextIndex);
-              if (!window.content_cosmetic.hide1pContent) {
-                window.content_cosmetic.allSelectorsToRules.set(
-                  selector, nextIndex);
-              }
-              nextIndex++;
-            }
-          });
-          if (!document.adoptedStyleSheets.includes(
-              window.content_cosmetic.cosmeticStyleSheet)) {
-            document.adoptedStyleSheets =
-              [window.content_cosmetic.cosmeticStyleSheet,
-                ...document.adoptedStyleSheets];
-          };
-        })();)";
-
-const char kStyleSelectorsInjectScript[] =
-    R"((function() {
-          let nextIndex =
-              window.content_cosmetic.cosmeticStyleSheet.rules.length;
-          const selectors = %s;
-          for (let selector in selectors) {
-            if (window.content_cosmetic.hide1pContent ||
-                !window.content_cosmetic.allSelectorsToRules.has(selector)) {
-              let rule = selector + '{';
-              selectors[selector].forEach(prop => {
-                if (!rule.endsWith('{')) {
-                  rule += ';';
-                }
-                rule += prop;
-              });
-              rule += '}';
-              window.content_cosmetic.cosmeticStyleSheet.insertRule(
-                `${rule}`, nextIndex);
-              if (!window.content_cosmetic.hide1pContent) {
-                window.content_cosmetic.allSelectorsToRules.set(
-                  selector, nextIndex);
-              }
-              nextIndex++;
-            };
-          };
-          if (!document.adoptedStyleSheets.includes(
-                window.content_cosmetic.cosmeticStyleSheet)){
-             document.adoptedStyleSheets =
-               [window.content_cosmetic.cosmeticStyleSheet,
-                 ...document.adoptedStyleSheets];
-          };
+          window.content_cosmetic.scheduleQueuePump(false, false);
         })();)";
 
 std::string LoadDataResource(const int id) {
@@ -237,6 +173,41 @@ void CosmeticFiltersJSHandler::AddJavaScriptObjectToFrame(
   bundle_injected_ = false;
 }
 
+// If `id` is nonzero, then the same number can be later passed to
+// `UninjectStylesheet` to remove the stylesheet from the page.
+void CosmeticFiltersJSHandler::InjectStylesheet(const std::string& stylesheet,
+                                                int id) {
+  blink::WebLocalFrame* web_frame = render_frame_->GetWebFrame();
+
+  blink::WebStyleSheetKey* style_sheet_key = nullptr;
+  if (id != 0) {
+    // Prepend a Brave-specific string to avoid collisions with stylesheets
+    // injected from other sources
+    std::string key = "BraveCFRule" + std::to_string(id);
+    inserted_stylesheet_ids.insert({id, std::make_unique<blink::WebString>(
+                                            blink::WebString::FromASCII(key))});
+    style_sheet_key = inserted_stylesheet_ids.at(id).get();
+  }
+  web_frame->GetDocument().InsertStyleSheet(
+      blink::WebString::FromUTF8(stylesheet), style_sheet_key,
+      blink::WebDocument::kUserOrigin);
+}
+
+void CosmeticFiltersJSHandler::UninjectStylesheet(int id) {
+  blink::WebLocalFrame* web_frame = render_frame_->GetWebFrame();
+
+  DCHECK_NE(id, 0);
+
+  auto i = inserted_stylesheet_ids.find(id);
+  if (i != inserted_stylesheet_ids.end()) {
+    std::unique_ptr<blink::WebStyleSheetKey> key = std::move(i->second);
+    inserted_stylesheet_ids.erase(i);
+
+    web_frame->GetDocument().RemoveInsertedStyleSheet(
+        *key, blink::WebDocument::kUserOrigin);
+  }
+}
+
 void CosmeticFiltersJSHandler::CreateWorkerObject(
     v8::Isolate* isolate,
     v8::Local<v8::Context> context) {
@@ -266,6 +237,14 @@ void CosmeticFiltersJSHandler::BindFunctionsToObject(
       isolate, javascript_object, "isFirstPartyUrl",
       base::BindRepeating(&CosmeticFiltersJSHandler::OnIsFirstParty,
                           base::Unretained(this)));
+  BindFunctionToObject(
+      isolate, javascript_object, "injectStylesheet",
+      base::BindRepeating(&CosmeticFiltersJSHandler::InjectStylesheet,
+                          base::Unretained(this)));
+  BindFunctionToObject(
+      isolate, javascript_object, "uninjectStylesheet",
+      base::BindRepeating(&CosmeticFiltersJSHandler::UninjectStylesheet,
+                          base::Unretained(this)));
 }
 
 template <typename Sig>
@@ -412,11 +391,6 @@ void CosmeticFiltersJSHandler::CSSRulesRoutine(
       (IsVettedSearchEngine(url_) && !enabled_1st_party_cf_)) {
     hide_selectors_list = nullptr;
   }
-  base::ListValue* force_hide_selectors_list;
-  if (!resources_dict->GetList("force_hide_selectors",
-                               &force_hide_selectors_list)) {
-    force_hide_selectors_list = nullptr;
-  }
 
   if (hide_selectors_list && hide_selectors_list->GetList().size() != 0) {
     std::string json_selectors;
@@ -434,41 +408,34 @@ void CosmeticFiltersJSHandler::CSSRulesRoutine(
         blink::BackForwardCacheAware::kAllow);
   }
 
+  base::Value* force_hide_selectors_list =
+      resources_dict->FindListKey("force_hide_selectors");
   if (force_hide_selectors_list &&
       force_hide_selectors_list->GetList().size() != 0) {
-    std::string json_selectors;
-    if (!base::JSONWriter::Write(*force_hide_selectors_list, &json_selectors) ||
-        json_selectors.empty()) {
-      json_selectors = "[]";
+    std::string stylesheet = "";
+    for (auto& selector : force_hide_selectors_list->GetList()) {
+      DCHECK(selector.is_string());
+      stylesheet += selector.GetString() + "{display:none !important}";
     }
-    // Building a script for stylesheet modifications
-    std::string new_selectors_script = base::StringPrintf(
-        kForceHideSelectorsInjectScript, json_selectors.c_str());
-    web_frame->ExecuteScriptInIsolatedWorld(
-        isolated_world_id_,
-        blink::WebScriptSource(
-            blink::WebString::FromUTF8(new_selectors_script)),
-        blink::BackForwardCacheAware::kAllow);
+    InjectStylesheet(stylesheet, 0);
   }
 
-  base::DictionaryValue* style_selectors_dictionary = nullptr;
-  if (resources_dict->GetDictionary("style_selectors",
-                                    &style_selectors_dictionary)) {
-    std::string json_selectors;
-    if (!base::JSONWriter::Write(*style_selectors_dictionary,
-                                 &json_selectors) ||
-        json_selectors.empty()) {
-      json_selectors = "[]";
-    }
-    std::string new_selectors_script =
-        base::StringPrintf(kStyleSelectorsInjectScript, json_selectors.c_str());
-    if (!json_selectors.empty()) {
-      web_frame->ExecuteScriptInIsolatedWorld(
-          isolated_world_id_,
-          blink::WebScriptSource(
-              blink::WebString::FromUTF8(new_selectors_script)),
-          blink::BackForwardCacheAware::kAllow);
+  base::Value* style_selectors_dictionary =
+      resources_dict->FindDictKey("style_selectors");
+  if (style_selectors_dictionary) {
+    std::string stylesheet = "";
+    for (const auto kv : style_selectors_dictionary->DictItems()) {
+      std::string selector = kv.first;
+      base::Value& styles = kv.second;
+      DCHECK(styles.is_list());
+      stylesheet += selector + '{';
+      for (auto& style : styles.GetList()) {
+        DCHECK(style.is_string());
+        stylesheet += style.GetString() + ';';
+      }
+      stylesheet += '}';
     }
+    InjectStylesheet(stylesheet, 0);
   }
 
   if (!enabled_1st_party_cf_)

components/cosmetic_filters/renderer/cosmetic_filters_js_handler.h

@@ -6,6 +6,7 @@
 #ifndef BRAVE_COMPONENTS_COSMETIC_FILTERS_RENDERER_COSMETIC_FILTERS_JS_HANDLER_H_
 #define BRAVE_COMPONENTS_COSMETIC_FILTERS_RENDERER_COSMETIC_FILTERS_JS_HANDLER_H_
 
+#include <map>
 #include <memory>
 #include <string>
 #include <vector>
@@ -32,7 +33,7 @@ class CosmeticFiltersJSHandler {
                            const int32_t isolated_world_id);
   ~CosmeticFiltersJSHandler();
 
-  // Adds the "cs_worker" JavaScript object and its functions to the current
+  // Adds the "cf_worker" JavaScript object and its functions to the current
   // render_frame_.
   void AddJavaScriptObjectToFrame(v8::Local<v8::Context> context);
   // Fetches an initial set of resources to inject into the page if cosmetic
@@ -68,6 +69,11 @@ class CosmeticFiltersJSHandler {
   void OnHiddenClassIdSelectors(base::Value result);
   bool OnIsFirstParty(const std::string& url_string);
 
+  void InjectStylesheet(const std::string& stylesheet, int id);
+  void UninjectStylesheet(int id);
+
+  std::map<int, std::unique_ptr<blink::WebString>> inserted_stylesheet_ids;
+
   raw_ptr<content::RenderFrame> render_frame_ = nullptr;
   mojo::Remote<cosmetic_filters::mojom::CosmeticFiltersResources>
       cosmetic_filters_resources_;

components/cosmetic_filters/resources/data/content_cosmetic.ts

@@ -33,6 +33,8 @@ const CC = window.content_cosmetic
 
 CC.cosmeticStyleSheet = CC.cosmeticStyleSheet || new CSSStyleSheet()
 CC.allSelectorsToRules = CC.allSelectorsToRules || new Map<string, number>()
+// Start from 1; valid indices must be nonzero.
+CC.nextRuleIndex = CC.nextRuleIndex || 1
 CC.observingHasStarted = CC.observingHasStarted || false
 // All new selectors go in `firstRunQueue`
 CC.firstRunQueue = CC.firstRunQueue || new Set<string>()
@@ -331,32 +333,11 @@ const unhideSelectors = (selectors: Set<string>) => {
     .sort()
     .reverse()
   // Delete the rules
-  let lastIdx: number = CC.allSelectorsToRules.size - 1
   for (const ruleIdx of rulesToRemove) {
     // Safe to asset ruleIdx is a number because we've already filtered out
     // any `undefined` instances with the filter call above.
-    CC.cosmeticStyleSheet.deleteRule(ruleIdx as number)
-  }
-  // Re-sync the indexes
-  // TODO: Sync is hard, just re-build by iterating through the StyleSheet rules.
-  const ruleLookup = Array.from(CC.allSelectorsToRules.entries())
-  let countAtLastHighest = rulesToRemove.length
-  for (let i = lastIdx; i > 0; i--) {
-    const [selector, oldIdx] = ruleLookup[i]
-    // Is this one we removed?
-    if (rulesToRemove.includes(i)) {
-      CC.allSelectorsToRules.delete(selector)
-      countAtLastHighest--
-      if (countAtLastHighest === 0) {
-        break
-      }
-      continue
-    }
-    if (oldIdx !== i) {
-      // Probably out of sync
-      console.error('Cosmetic Filters: old index did not match lookup index', { selector, oldIdx, i })
-    }
-    CC.allSelectorsToRules.set(selector, oldIdx - countAtLastHighest)
+    // @ts-expect-error
+    cf_worker.uninjectStylesheet(ruleIdx as number)
   }
 }
 
@@ -539,5 +520,6 @@ const tryScheduleQueuePump = () => {
 }
 
 CC.tryScheduleQueuePump = CC.tryScheduleQueuePump || tryScheduleQueuePump
+CC.scheduleQueuePump = CC.scheduleQueuePump || scheduleQueuePump
 
 tryScheduleQueuePump()

components/definitions/global.d.ts

@@ -28,6 +28,7 @@ declare global {
     content_cosmetic: {
       cosmeticStyleSheet: CSSStyleSheet
       allSelectorsToRules: Map<string, number>
+      nextRuleIndex: number
       observingHasStarted: boolean
       hide1pContent: boolean
       generichide: boolean
@@ -40,6 +41,7 @@ declare global {
       alreadyKnownFirstPartySubtrees: WeakSet
       _hasDelayOcurred: boolean
       _startCheckingId: number | undefined
+      scheduleQueuePump: ((hide1pContent: boolean, genericHide: boolean) => void)
       tryScheduleQueuePump: (() => void)
     }
   }

test/data/cosmetic_filtering.html

@@ -51,10 +51,10 @@
 <body>
     <div id="ad-banner"><img src="https://example.com/logo.png" alt=""></div>
     <div class="ad-banner">
-        <div class="ad" style="background: url(example.com)"><img src="https://example.com/logo.png" alt=""></div>
+        <div class="ad" style="background: url(https://example.com)"><img src="https://example.com/logo.png" alt=""></div>
     </div>
-    <div class="ad" style="background: url(example.com)"><img src="https://example.com/logo.png" alt=""></div>
-    <div class="ad" style="background: url(example.com)"><img src="https://example.com/logo.png" alt=""></div>
+    <div class="ad" style="background: url(https://example.com)"><img src="https://example.com/logo.png" alt=""></div>
+    <div class="ad" style="background: url(https://example.com)"><img src="https://example.com/logo.png" alt=""></div>
 
     <!-- Assume that the document host is test.lion.appspot.com (see AdBlockServiceTest.*) -->
     <div id="relative-url-div" class="fpsponsored">
@@ -77,5 +77,6 @@
       A text of 30 chars and 5 words is needed here to consider element significant.
       <img src="http://chrome.appspot.com/sponsored/640x820.png">
     </div>
+    <div id="inline-block-important" style="display: block !important"></div>
 </body>
 </html>