Implement cosmetic filtering #3303

antonok-edm · 2019-08-29T23:18:59Z

Submitter Checklist:

Closes brave/brave-browser#5381
Depends on brave/adblock-rust-ffi#3

Test Plan:

Majority of tests, covering fine-grained API behavior, are implemented in unit tests within https://github.com/brave/adblock-rust.

FFI behavior is validated by tests in https://github.com/brave/adblock-rust-ffi.

End-to-end browser behavior is verified by preloading custom cosmetic rules into the adblock-rust engine, loading a test page, and verifying that DOM elements show as expected using getComputedStyle.

Reviewer Checklist:

New files have MPL-2.0 license header.
Request a security/privacy review as needed.
Adequate test coverage exists to prevent regressions
Verify test plan is specified in PR before merging to source

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on.
All relevant documentation has been updated.

petemill

This is great. On my first review pass, I was attempting to understand the code architecture via the method / API naming and calls. I couldn't do that so I had to read each function implementation to get a basic understanding. I recommend changing the naming to become more understandable. The API method descriptions in the JSON are clearer and could be a good starting point. Here's some suggestions:

chrome.braveShields.{get,set}CosmeticFilteredElementsAsync ⟶ {get,set}CosmeticFilteringControlTypeAsync
chrome.braveShields.hostnameCosmeticResources ⟶ getCosemeticStylesheetForHostname
Though perhaps ...StyleRules... is less confusing since it returns a string and not a javascript StyleSheet object. Also perhaps create / generate is clearer than get? Behind the scenes is it fetching something or doing some heavy generation work?
chrome.braveShields.classIdStylesheet ⟶ getCosmeticStyleSheetForElements
Same considerations as for hostnameCosmeticResources, perhaps ...ForElementSelectors is clearer as it doesn't accept actual DOM Elements, but that's quite long.

components/brave_extension/extension/brave_extension/content.ts

petemill · 2019-08-30T18:02:14Z

components/brave_extension/extension/brave_extension/background/events/cosmeticFilterEvents.ts

+          cssOrigin: 'user',
+          runAt: 'document_start'
+        })
+      })


) should be on separate line since { is separate line to (

Hmm, not sure what you mean? The first }) matches to ...insertCSS({, the second matches to ...Stylesheet(... => {.

antonok-edm · 2019-09-03T16:00:59Z

chrome.braveShields.hostnameCosmeticResources ⟶ getCosemeticStylesheetForHostname
Though perhaps ...StyleRules... is less confusing since it returns a string and not a javascript StyleSheet object. Also perhaps create / generate is clearer than get? Behind the scenes is it fetching something or doing some heavy generation work?

This actually returns 3 distinct things dependent on the hostname: the stylesheet in string format, a list of exceptions to generic cosmetic rules, and a script to be injected in the page. I figured just calling it a stylesheet might be confusing. I'd lean towards ...StyleRules..., I guess. It is querying saved rules from hashmaps according to the domain and assembling them behind the scenes.

By the way, do you know if there is a convenient way to build a Javascript StyleSheet object as part of the API? That sounds like it would make more sense, although it seems like it would be hard to do on the C++ side / without another layer of wrapping functions.

chrome.braveShields.classIdStylesheet ⟶ getCosmeticStyleSheetForElements
Same considerations as for hostnameCosmeticResources, perhaps ...ForElementSelectors is clearer as it doesn't accept actual DOM Elements, but that's quite long.

Perhaps getCosmeticStylesheetForSelectors? Although, again this is "generated/created" in a similar way.

antonok-edm · 2019-09-03T19:03:35Z

Also just added a toggle in settings corresponding to the new shields panel option.

szaimen · 2019-10-22T21:33:20Z

Any update here?

antonok-edm · 2019-10-24T16:05:18Z

@szaimen Still blocked on code reviews for brave/adblock-rust#50 unfortunately

szaimen · 2019-11-03T18:47:31Z

@antonok-edm any update here now that brave/adblock-rust#50 is merged?

antonok-edm · 2019-11-06T20:33:02Z

@szaimen #3419 and brave/brave-core-crx-packager#74 are required to fully support brave/adblock-rust#50 in the browser. Cosmetic filtering will be unblocked once that's sorted.

app/brave_generated_resources.grd

bbondy · 2019-11-18T14:30:39Z

components/brave_extension/extension/brave_extension/background/api/cosmeticFilterAPI.ts

+      exceptions: genericExceptions
+    })
+
+    chrome.tabs.executeScript(tabId, {


Can we call this only when injectedScript is empty.
Also checking with @diracdeltas about if we can do this and trust the different lists we use. Are these generic scripts or only in a certain guaranteed format to hide things? Could you give an example of an ABP filter syntax rule that uses it?

The scripts come from the same set of resources as for network redirects.
There are a few extra parameterized templated scriptlets, which were included in the same security review.

Here's an example of a scriptlet injection rule with 1 argument, directly from uBlock's filters:

computerbild.de##+js(abort-on-property-read.js, Date.prototype.toUTCString)

The argument (Date.prototype.toUTCString) is interpolated into this function at the location of the first occurrence of {{1}}, and the resulting function would be used in this executeScript call.

bbondy · 2019-11-18T14:45:00Z

components/brave_extension/extension/brave_extension/content.ts


 function getCurrentURL () {
  return window.location.hostname
 }

+const getClassesAndIds = function (addedNodes: Element[]) {


cc @bridiver is there an observer you know of that we can use in c++ to get the list of IDs and class names on all elements instead of getting them here? It seems a little expensive to do on each page load.

How the API works, is you pass in any class names or IDs that might be used, and then the adblock engine determines the stylesheet to use for blocking.

This is what uBlock Origin does and I haven't found the performance hit to be noticeable in practice, but it would be nice to take advantage of native APIs if possible.

components/brave_extension/extension/brave_extension/background/reducers/shieldsPanelReducer.ts

components/brave_shields/browser/ad_block_base_service.h

bbondy · 2019-11-18T15:16:32Z

components/brave_extension/extension/brave_extension/content.ts

+    classIdBuffer.classes.push(...classes)
+    classIdBuffer.ids.push(...ids)
+  } else {
+    chrome.runtime.sendMessage({


In practice, do you know how often classIdStylesheet gets hit for pages? Just making sure we're not inserting like 1000 stylesheets. I think maybe we should be debouncing these calls.

It depends entirely on how often the site makes mutations to the page. I can try on a few different ones if you'd like specific examples.
Note that this is already batched on a per-mutation basis - i.e. if a node tree is added, it's only called once for all the classes and ids in the entire tree.

components/brave_shields/browser/ad_block_service_browsertest.cc

components/brave_extension/extension/brave_extension/_locales/en_US/messages.json

components/brave_shields/browser/ad_block_service_browsertest.cc

browser/resources/settings/default_brave_shields_page/default_brave_shields_page.html

petemill

LGTM apart from the small redux point I raised. Let's get CI passing or investigate the issue. Great work!

bbondy · 2019-12-12T16:09:37Z

DEPS

@@ -1,7 +1,7 @@
 use_relative_paths = True

 deps = {
-  "vendor/adblock_rust_ffi": "https://github.com/brave/adblock-rust-ffi.git@89127a30655eaf54cf73794309846084ea8b91b9",
+  "vendor/adblock_rust_ffi": "https://github.com/brave/adblock-rust-ffi.git@6dad8c1b47c1b866fa1b1c19e36da6eb5797a524",


Before this gets merged please merge this:
brave/adblock-rust-ffi#7

And then update this changeset ref. As I only want to commit the ref here for things in master on the ffi repo.

bbondy

Approved, just one blocking note about the changeset 👍

antonok-edm · 2019-12-12T16:23:33Z

@petemill

As an aside, is there anywhere documented as to what the various cosmetic filter properties are? In other words, what 'exceptions' are, as opposed to either selectors for hiding, custom CSS rules, custom JS script and whatever else is contained.

Most of how this works is explained in doc comments from adblock-rust. I just double checked and there are a few important data structures that could use some additional explanation, so I'll follow up there as well.

edit: brave/adblock-rust#61

return a single script for cosmetic scriptlet injection

mutation observer applied only if rule exists notes change order mutation observer WIP browser tests isIdempotent isIdempotent mutation observer mutationObserver mutation observer mutationObserver mutation observer commented mutation observer for anton cleanup comments

Occasionally, chrome.tabs.insertCSS fails when triggered from chrome.webNavigation events. It cannot work unless some page content is already loaded. See https://bugs.chromium.org/p/chromium/issues/detail?id=331654#c15

bridiver · 2019-12-14T17:16:30Z

Jenkins is failing because of some glibc error that does not appear to be related to this PR

bridiver · 2019-12-14T17:18:26Z

I'm approving this as ok to merge because @antonok-edm has verified the build on linux

This reverts commit fe9f891, reversing changes made to 4e6ba95.

This reverts commit fe9f891, reversing changes made to 4e6ba95. Was causing problems specifically on Linux because of the Rust dependency. This should fix brave/brave-browser#7055 This will un-fix brave/brave-browser#5381

bsclifton · 2019-12-18T16:51:46Z

Reverted with #4255

Note: when this was merged, milestone should have been set @antonok-edm (ex: set milestone to 1.4, set milestone for issue also). I re-opened the issue and left it without a milestone

antonok-edm requested a review from bbondy August 29, 2019 23:19

antonok-edm self-assigned this Aug 29, 2019

antonok-edm added feature/shields dependencies Pull requests that update a dependency file enhancement labels Aug 29, 2019

antonok-edm requested a review from petemill August 29, 2019 23:37

antonok-edm force-pushed the cosmetic-filtering-frontend branch from f87078a to de99c72 Compare August 29, 2019 23:45

antonok-edm mentioned this pull request Aug 29, 2019

WIP: mutation observer #2727

Closed

32 tasks

antonok-edm force-pushed the cosmetic-filtering-frontend branch 2 times, most recently from 8421e0d to 923c2d4 Compare August 30, 2019 04:16

petemill reviewed Aug 30, 2019

View reviewed changes

components/brave_extension/extension/brave_extension/content.ts Outdated Show resolved Hide resolved

petemill reviewed Aug 30, 2019

View reviewed changes

antonok-edm force-pushed the cosmetic-filtering-frontend branch 2 times, most recently from 93877bb to 25d6c39 Compare September 3, 2019 18:17

antonok-edm requested a review from bridiver as a code owner September 3, 2019 19:02

antonok-edm requested a review from petemill September 6, 2019 17:39

antonok-edm force-pushed the cosmetic-filtering-frontend branch from b7d74d3 to 6aa0f4c Compare November 12, 2019 22:09

bbondy requested changes Nov 18, 2019

View reviewed changes

antonok-edm force-pushed the cosmetic-filtering-frontend branch from 6aa0f4c to 6d321f1 Compare November 20, 2019 01:17

antonok-edm requested review from bbondy and tildelowengrimm November 20, 2019 18:16

antonok-edm force-pushed the cosmetic-filtering-frontend branch 2 times, most recently from d3a2595 to cab4209 Compare November 20, 2019 18:58

petemill reviewed Dec 12, 2019

View reviewed changes

bbondy reviewed Dec 12, 2019

View reviewed changes

bbondy approved these changes Dec 12, 2019

View reviewed changes

antonok-edm force-pushed the cosmetic-filtering-frontend branch 2 times, most recently from e3b6f91 to 165eb3f Compare December 12, 2019 17:12

antonok-edm mentioned this pull request Dec 12, 2019

Add doc comments for cosmetic filter structs/methods brave/adblock-rust#61

Merged

antonok-edm force-pushed the cosmetic-filtering-frontend branch from 165eb3f to 5ea7413 Compare December 13, 2019 01:27

antonok-edm and others added 9 commits December 13, 2019 13:57

expose cosmetic filtering javascript API

1715046

return a single script for cosmetic scriptlet injection

use cosmetic filtering api

7512da5

work around browser-internal race condition

c45be3c

Occasionally, chrome.tabs.insertCSS fails when triggered from chrome.webNavigation events. It cannot work unless some page content is already loaded. See https://bugs.chromium.org/p/chromium/issues/detail?id=331654#c15

add browser tests for cosmetic filtering

3aca80f

add brave://flags feature to enable cosmetic filtering

5640c42

connect feature flag to cosmetic filtering mechanism

6a9fa30

only use MutationObserver when cosmetic filtering is enabled

e7af497

store generic rule exceptions in the Shields Panel state

ec80ce7

antonok-edm force-pushed the cosmetic-filtering-frontend branch from d8fa2fe to ec80ce7 Compare December 13, 2019 21:58

antonok-edm merged commit fe9f891 into master Dec 14, 2019

antonok-edm deleted the cosmetic-filtering-frontend branch December 14, 2019 18:14

antonok-edm mentioned this pull request Dec 17, 2019

Update generateCosmeticBlockingStylesheet implementation w/ tests #4246

Closed

32 tasks

bridiver added a commit that referenced this pull request Dec 18, 2019

Revert "Merge pull request #3303 from brave/cosmetic-filtering-frontend"

6d59ebf

This reverts commit fe9f891, reversing changes made to 4e6ba95.

bsclifton added this to the 1.4.x - Nightly milestone Dec 18, 2019

antonok-edm mentioned this pull request Jan 9, 2020

Unrevert cosmetic filtering #4269

Merged

32 tasks

pes10k mentioned this pull request Mar 3, 2020

Implement cosmetic blocking [tracking] brave/brave-browser#5381

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement cosmetic filtering #3303

Implement cosmetic filtering #3303

antonok-edm commented Aug 29, 2019 •

edited

Loading

petemill left a comment •

edited

Loading

petemill Aug 30, 2019

antonok-edm Sep 3, 2019

antonok-edm commented Sep 3, 2019

antonok-edm commented Sep 3, 2019

szaimen commented Oct 22, 2019

antonok-edm commented Oct 24, 2019

szaimen commented Nov 3, 2019

antonok-edm commented Nov 6, 2019

bbondy Nov 18, 2019

antonok-edm Nov 19, 2019

bbondy Nov 18, 2019

antonok-edm Nov 19, 2019

bbondy Nov 18, 2019

antonok-edm Nov 19, 2019

petemill left a comment

bbondy Dec 12, 2019

bbondy left a comment

antonok-edm commented Dec 12, 2019 •

edited

Loading

bridiver commented Dec 14, 2019

bridiver commented Dec 14, 2019

bsclifton commented Dec 18, 2019

Implement cosmetic filtering #3303

Implement cosmetic filtering #3303

Conversation

antonok-edm commented Aug 29, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

petemill left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonok-edm commented Sep 3, 2019

antonok-edm commented Sep 3, 2019

szaimen commented Oct 22, 2019

antonok-edm commented Oct 24, 2019

szaimen commented Nov 3, 2019

antonok-edm commented Nov 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy left a comment

Choose a reason for hiding this comment

antonok-edm commented Dec 12, 2019 • edited Loading

bridiver commented Dec 14, 2019

bridiver commented Dec 14, 2019

bsclifton commented Dec 18, 2019

antonok-edm commented Aug 29, 2019 •

edited

Loading

petemill left a comment •

edited

Loading

antonok-edm commented Dec 12, 2019 •

edited

Loading