Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cosmetic filtering fixes #12038

Merged
merged 2 commits into from
Jan 28, 2022
Merged

Cosmetic filtering fixes #12038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea218b8
miscellaneous fixes for cosmetic filtering
antonok-edm Jan 27, 2022
6185388
remove unecessary calls into JS
antonok-edm Jan 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions browser/brave_shields/ad_block_service_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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() {
Comment on lines +2024 to +2025
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/brave/brave-core/pull/6735/files#r794075446

if (await checkSelector('.blockme', 'display', 'none')) {
window.domAutomationController.send(true);
} else {
Expand Down Expand Up @@ -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() {
Expand Down
175 changes: 71 additions & 104 deletions components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[] =
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed kForceHideSelectorsInjectScript and kStyleSelectorsInjectScript entirely. They were previously adding rules to window.content_cosmetic.cosmeticStyleSheet in order to keep track of the indices of rules, but this is no longer necessary as a result of using injectStylesheet with an id of 0. At that point, there is no special behavior in JS and we can construct the corresponding stylesheets directly in C++ instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool refactor!

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) {
Expand Down Expand Up @@ -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);
Comment on lines +191 to +193
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InsertStyleSheet with kUserOrigin provides the desired "injected stylesheet" behavior as if it were added by an extension, rather than "constructed stylesheet" from document.adoptedStyleSheets.

It also has the added bonus of being able to pass in a custom blink::WebString as an identifier for each stylesheet, allowing those sheets to be removed later by RemoveInsertedStyleSheet. There was previously a lot of JS code to manage this around the document.adoptedStyleSheets API.

InsertStyleSheet and RemoveInsertedStyleSheet are exposed directly to the JS cosmetic filtering script through injectStylesheet and uninjectStylesheet.

}

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) {
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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;
Expand All @@ -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_)
Expand Down
8 changes: 7 additions & 1 deletion components/cosmetic_filters/renderer/cosmetic_filters_js_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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
Expand Down Expand Up @@ -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_;
Expand Down
28 changes: 5 additions & 23 deletions components/cosmetic_filters/resources/data/content_cosmetic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>()
Expand Down Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -539,5 +520,6 @@ const tryScheduleQueuePump = () => {
}

CC.tryScheduleQueuePump = CC.tryScheduleQueuePump || tryScheduleQueuePump
CC.scheduleQueuePump = CC.scheduleQueuePump || scheduleQueuePump

tryScheduleQueuePump()
2 changes: 2 additions & 0 deletions components/definitions/global.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ declare global {
content_cosmetic: {
cosmeticStyleSheet: CSSStyleSheet
allSelectorsToRules: Map<string, number>
nextRuleIndex: number
observingHasStarted: boolean
hide1pContent: boolean
generichide: boolean
Expand All @@ -40,6 +41,7 @@ declare global {
alreadyKnownFirstPartySubtrees: WeakSet
_hasDelayOcurred: boolean
_startCheckingId: number | undefined
scheduleQueuePump: ((hide1pContent: boolean, genericHide: boolean) => void)
tryScheduleQueuePump: (() => void)
}
}
Expand Down
7 changes: 4 additions & 3 deletions test/data/cosmetic_filtering.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Expand All @@ -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>