From ea218b8e12842aac4169b043d90108ed7232df75 Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Thu, 27 Jan 2022 15:26:10 -0800 Subject: [PATCH 1/2] miscellaneous fixes for cosmetic filtering --- .../ad_block_service_browsertest.cc | 34 ++++- .../renderer/cosmetic_filters_js_handler.cc | 134 +++++++++--------- .../renderer/cosmetic_filters_js_handler.h | 8 +- .../resources/data/content_cosmetic.ts | 28 +--- components/definitions/global.d.ts | 2 + test/data/cosmetic_filtering.html | 7 +- 6 files changed, 119 insertions(+), 94 deletions(-) diff --git a/browser/brave_shields/ad_block_service_browsertest.cc b/browser/brave_shields/ad_block_service_browsertest.cc index 88d03b404147..44aac67f3b27 100644 --- a/browser/brave_shields/ad_block_service_browsertest.cc +++ b/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() { diff --git a/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc b/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc index d3509ee8246b..4738f3aa25e5 100644 --- a/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc +++ b/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc @@ -77,87 +77,50 @@ 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]; - }; + window.content_cosmetic.scheduleQueuePump(false, false); })();)"; 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++; + window.cf_worker.injectStylesheet(rule, 0); } }); - 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); + let rule = selector + '{'; + selectors[selector].forEach(prop => { + if (!rule.endsWith('{')) { + rule += ';'; } - nextIndex++; - }; - }; - if (!document.adoptedStyleSheets.includes( - window.content_cosmetic.cosmeticStyleSheet)){ - document.adoptedStyleSheets = - [window.content_cosmetic.cosmeticStyleSheet, - ...document.adoptedStyleSheets]; + rule += prop; + }); + rule += '}'; + window.cf_worker.injectStylesheet(rule, 0); }; })();)"; @@ -237,6 +200,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::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 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 context) { @@ -266,6 +264,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 @@ -455,19 +461,17 @@ void CosmeticFiltersJSHandler::CSSRulesRoutine( 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); + 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); + } } } diff --git a/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.h b/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.h index 475c6bbb71d2..8a2d72c9e5ff 100644 --- a/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.h +++ b/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 #include #include #include @@ -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 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> inserted_stylesheet_ids; + raw_ptr render_frame_ = nullptr; mojo::Remote cosmetic_filters_resources_; diff --git a/components/cosmetic_filters/resources/data/content_cosmetic.ts b/components/cosmetic_filters/resources/data/content_cosmetic.ts index 4e4088e0c9b3..e85246a8d3fa 100644 --- a/components/cosmetic_filters/resources/data/content_cosmetic.ts +++ b/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() +// 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() @@ -331,32 +333,11 @@ const unhideSelectors = (selectors: Set) => { .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() diff --git a/components/definitions/global.d.ts b/components/definitions/global.d.ts index 3a9e23cbfe12..915f7bfddeb6 100644 --- a/components/definitions/global.d.ts +++ b/components/definitions/global.d.ts @@ -28,6 +28,7 @@ declare global { content_cosmetic: { cosmeticStyleSheet: CSSStyleSheet allSelectorsToRules: Map + 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) } } diff --git a/test/data/cosmetic_filtering.html b/test/data/cosmetic_filtering.html index 78218b5e0abf..9c4a10199306 100644 --- a/test/data/cosmetic_filtering.html +++ b/test/data/cosmetic_filtering.html @@ -51,10 +51,10 @@
-
+
-
-
+
+
@@ -77,5 +77,6 @@ A text of 30 chars and 5 words is needed here to consider element significant.
+
From 618538856a2189882ce7f7f1ef36f1c1df0ad71d Mon Sep 17 00:00:00 2001 From: Anton Lazarev Date: Thu, 27 Jan 2022 16:06:19 -0800 Subject: [PATCH 2/2] remove unecessary calls into JS --- .../renderer/cosmetic_filters_js_handler.cc | 79 +++++-------------- 1 file changed, 21 insertions(+), 58 deletions(-) diff --git a/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc b/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc index 4738f3aa25e5..8b90132f958e 100644 --- a/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc +++ b/components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc @@ -97,33 +97,6 @@ const char kHideSelectorsInjectScript[] = window.content_cosmetic.scheduleQueuePump(false, false); })();)"; -const char kForceHideSelectorsInjectScript[] = - R"((function() { - const selectors = %s; - selectors.forEach(selector => { - if (typeof selector === 'string') { - let rule = selector + '{display:none !important;}'; - window.cf_worker.injectStylesheet(rule, 0); - } - }); - })();)"; - -const char kStyleSelectorsInjectScript[] = - R"((function() { - const selectors = %s; - for (let selector in selectors) { - let rule = selector + '{'; - selectors[selector].forEach(prop => { - if (!rule.endsWith('{')) { - rule += ';'; - } - rule += prop; - }); - rule += '}'; - window.cf_worker.injectStylesheet(rule, 0); - }; - })();)"; - std::string LoadDataResource(const int id) { auto& resource_bundle = ui::ResourceBundle::GetSharedInstance(); if (resource_bundle.IsGzipped(id)) { @@ -418,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; @@ -440,39 +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_)