Unrevert cosmetic filtering #4269

antonok-edm · 2019-12-20T01:18:26Z

Closes brave/brave-browser#5381 (again...).

Original, now reverted, PR: #3303

This points to an updated adblock-rust-ffi version, which points to an updated adblock-rust version that makes CSS selector validation an optional feature. The GLIBC_2.29 error only occurred because of adblock-rust's cssparser library dependency calling f64::pow; this is no longer included in brave-core builds. It only really needs to be used by the CRX packager when building DAT files.

Apart from the original changes from #3303, there are a few additional commits from @snyderp and myself which are used to detect whether matching elements on a page are 1p content tor 3p whitespace before injecting a stylesheet to block them.

Submitter Checklist:

Test Plan:

CI on this PR should pass.
npm run test -- brave_browser_tests --filter="CosmeticFilteringEnabledTest.*"

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.

bsclifton · 2020-01-09T22:51:49Z

Just rebased; let's watch CI here. Most importantly, we'll want to watch Linux CI. It was having problems with GLIBC

bsclifton · 2020-01-09T22:52:29Z

@bridiver did you have any other concerns about this? You were an approver for the original work, IIRC. If things look good, go ahead and hit the approve button 😄

bridiver · 2020-01-09T23:00:59Z

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@pes-cosmetic-analysis",


can we change this to a commit hash?

petemill

This has some tests failing which seem related:

19:01:40  4 tests failed:
19:01:40      CosmeticFilteringEnabledTest.CosmeticFilteringCustomStyle (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:906)
19:01:40      CosmeticFilteringEnabledTest.CosmeticFilteringDynamic (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:876)
19:01:40      CosmeticFilteringEnabledTest.CosmeticFilteringSimple (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:839)
19:01:40      CosmeticFilteringEnabledTest.CosmeticFilteringUnhide (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:927)

pes10k · 2020-01-22T21:34:16Z

@petemill just double checking, do @antonok-edm 's new tests address the concern?

bsclifton · 2020-01-23T06:13:34Z

@snyderp looks like tests all pass with the latest push! 🎉 So I think we're good to go here

As there is a cherry-picked commit from here in one of @petemill 's PRs that we're trying to expedite for 1.3, I'll let him do the honors of approving / merging 😄

bsclifton · 2020-01-23T06:18:07Z

After merge, let's ping @snyderp / @ryanbr to address #4403 (comment) 😄

petemill · 2020-02-03T22:43:34Z

I rebased, fixed conflicts and pushed. Let's see what CI does (although tests passed locally). Giving the code another once-over.

petemill · 2020-02-03T22:45:47Z

components/brave_extension/extension/brave_extension/content_cosmetic.ts

+
+const handleMutations = function (mutations: any[]) {
+  for (const aMutation of mutations) {
+    if (aMutation.type === 'attribute') {


I don't think this will work - shouldn't it be checking for attributes not attribute? We need to use proper TS types and not pass in any as the params, which would have caught this. I'm happy to fix this now.

@petemill yes from checking https://developer.mozilla.org/en-US/docs/Web/API/MutationObserver that looks right to me too (i.e. changing to attributes is correct).

… block component. Reverts the revert of the oringal implementation: "Merge pull request #4255 from brave/bsc-revert-cosmetic-filtering" This reverts commit fcfe38a, reversing changes made to 950778a. Fixes some conflicts as a result of master getting flag features and uses updated adblock-rust-ffi version which provides a slightly different API than for the original implementation.

petemill

CI passed. @snyderp @antonok-edm please can you review my additions before merge: d2675a1

petemill · 2020-02-04T05:38:33Z

Awaiting final code owner review from @bridiver

…ification and add stronger types

antonok-edm · 2020-02-04T15:41:43Z

CI passed. @snyderp @antonok-edm please can you review my additions before merge: d2675a1

@petemill those changes look good to me!

petemill · 2020-02-04T17:08:06Z

tsconfig.json

    "allowJs": true,
    // convert jsx to react components
    "jsx": "react",
    "lib": [
      "es6",
      "dom",
+      "DOM.Iterable",


Prevents Typescript from complaining when we iterate over DomList and provides values() in DOMTokenList

petemill · 2020-02-04T17:10:07Z

@antonok-edm @snyderp It's being asked what exactly fixes the issue that made this previously get reverted because of #4255 ?

iefremov · 2020-07-29T15:21:07Z

components/brave_shields/browser/ad_block_base_service.cc

+base::Optional<base::Value> AdBlockBaseService::HostnameCosmeticResources(
+        const std::string& hostname) {
+  return base::JSONReader::Read(
+          this->ad_block_client_->hostnameCosmeticResources(hostname));


Callling this from UI thread was causing this quite nasty family of crashes brave/brave-browser#10907 - just wanted to emphasize that we should super carefully review multithreaded code

@iefremov yikes! Are they sorted out now? What would be helpful for @antonok-edm or I do from here to sort things out?

@pes10k fixing it here #6250

Gotcha, thanks a million @iefremov !

antonok-edm requested a review from bridiver as a code owner December 20, 2019 01:18

antonok-edm self-assigned this Dec 20, 2019

antonok-edm force-pushed the unrevert-cosmetic-filtering branch from 4d72014 to f1171e9 Compare December 20, 2019 04:37

antonok-edm force-pushed the unrevert-cosmetic-filtering branch 2 times, most recently from f423a6d to 4a2ee6b Compare January 9, 2020 14:53

bsclifton added this to the 1.5.x - Nightly milestone Jan 9, 2020

bsclifton self-requested a review January 9, 2020 22:50

bsclifton force-pushed the unrevert-cosmetic-filtering branch from 4a2ee6b to 444d441 Compare January 9, 2020 22:51

bridiver reviewed Jan 9, 2020

View reviewed changes

antonok-edm force-pushed the unrevert-cosmetic-filtering branch from 444d441 to 5e71153 Compare January 10, 2020 20:57

antonok-edm mentioned this pull request Jan 10, 2020

Match newer selectors API of adblock-rust brave/adblock-rust-ffi#12

Merged

antonok-edm force-pushed the unrevert-cosmetic-filtering branch 4 times, most recently from fdfbb6f to e243cfb Compare January 14, 2020 02:12

petemill requested changes Jan 14, 2020

View reviewed changes

bsclifton mentioned this pull request Jan 16, 2020

skepticalscience.com causes 100% CPU usage leading to browser becoming unresponsive brave/brave-browser#5406

Closed

simonhong added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Jan 20, 2020

pes10k mentioned this pull request Jan 22, 2020

Partially revert https://github.com/brave/brave-core/pull/4094 #4403

Merged

petemill force-pushed the unrevert-cosmetic-filtering branch from a9e9213 to 805e25b Compare February 3, 2020 22:43

petemill requested changes Feb 3, 2020

View reviewed changes

antonok-edm and others added 3 commits February 3, 2020 16:40

Refactor client-side cosmetic filtering for more iteration

a598670

Cosmetic filtering ignores content determined to be 1st party

14ff077

petemill force-pushed the unrevert-cosmetic-filtering branch from 805e25b to 538e12e Compare February 4, 2020 00:40

petemill approved these changes Feb 4, 2020

View reviewed changes

petemill requested a review from pes10k February 4, 2020 05:37

Cosmetic filtering content script: fix mutation handler attribute mod…

d2675a1

…ification and add stronger types

petemill force-pushed the unrevert-cosmetic-filtering branch from 538e12e to d2675a1 Compare February 4, 2020 05:43

petemill reviewed Feb 4, 2020

View reviewed changes

bridiver approved these changes Feb 4, 2020

View reviewed changes

petemill merged commit 1a2fe76 into master Feb 4, 2020

petemill deleted the unrevert-cosmetic-filtering branch February 4, 2020 17:51

This was referenced Feb 4, 2020

[Android] master build error brave/brave-browser#8056

Closed

Missing deps for brave_shields/common #4499

Merged

Uplift fixing the build to 1.5.x (#4499 and #4505) #4504

Merged

Missing dep in brave_shields/common #4505

Merged

antonok-edm mentioned this pull request Feb 5, 2020

Enable cosmetic filtering by default #4511

Merged

32 tasks

petemill mentioned this pull request Feb 11, 2020

Cosmetic filtering causes a layout re-flow during page-load brave/brave-browser#8197

Closed

iefremov reviewed Jul 29, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unrevert cosmetic filtering #4269

Unrevert cosmetic filtering #4269

antonok-edm commented Dec 20, 2019 •

edited by petemill

Loading

bsclifton commented Jan 9, 2020

bsclifton commented Jan 9, 2020

bridiver Jan 9, 2020

petemill left a comment

pes10k commented Jan 22, 2020

bsclifton commented Jan 23, 2020 •

edited

Loading

bsclifton commented Jan 23, 2020

petemill commented Feb 3, 2020 •

edited

Loading

petemill Feb 3, 2020

pes10k Feb 3, 2020

petemill left a comment •

edited

Loading

petemill commented Feb 4, 2020

antonok-edm commented Feb 4, 2020

petemill Feb 4, 2020

petemill commented Feb 4, 2020

iefremov Jul 29, 2020

pes10k Jul 29, 2020

iefremov Jul 30, 2020

pes10k Jul 30, 2020

Unrevert cosmetic filtering #4269

Unrevert cosmetic filtering #4269

Conversation

antonok-edm commented Dec 20, 2019 • edited by petemill Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

bsclifton commented Jan 9, 2020

bsclifton commented Jan 9, 2020

Choose a reason for hiding this comment

petemill left a comment

Choose a reason for hiding this comment

pes10k commented Jan 22, 2020

bsclifton commented Jan 23, 2020 • edited Loading

bsclifton commented Jan 23, 2020

petemill commented Feb 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petemill left a comment • edited Loading

Choose a reason for hiding this comment

petemill commented Feb 4, 2020

antonok-edm commented Feb 4, 2020

Choose a reason for hiding this comment

petemill commented Feb 4, 2020

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

antonok-edm commented Dec 20, 2019 •

edited by petemill

Loading

bsclifton commented Jan 23, 2020 •

edited

Loading

petemill commented Feb 3, 2020 •

edited

Loading

petemill left a comment •

edited

Loading