Integrate Eyeo Adblock Plus in Bromite

[uazo]

Description

First working version related to eyeo adblock plus integration in Bromite

Removed what I think is unsuitable for bromite:

• Telemetry
• Parameters/Cookies/Headers from the download url
• Downloads in build phase (Preloaded Subscriptions)
• Acceptable Ads Headers ("x-adblock-key")
• deny the rules that modify the headers and the addition of snippets (AllowPrivilegedFilters)

Modified:

• Update check timeout (1 hour to 24 hours)
• Some LOGs

if you can, start to see it so we talk about it.

All submissions

• there are no other open Pull Requests for the same update/change
• Bromite can be built with these changes
• I have tested that the new change works as intended (AVD or physical device will do)

Format

• patch subject and filename match (e.g. Subject: Alternative cache (NIK-based) -> Alternative-cache-NIK-based.patch)
• patch description contains explanation of changes
• no unnecessary whitespace or unrelated changes

[uazo]

ah sorry, forgot. the patch is done on v106, so you can't merge but you can review

[ghost]

Is it easier to integrate than uBlock Origin?

[uazo]

Is it easier to integrate than uBlock Origin?

without a shadow of a doubt. it is already there, all ready and well done. I did very little, at the moment.
we need to understand if it is also easy to maintain, we'll see.

[csagan5]

What would be the strategy for the existing subresource filter? This patch does not seem to touch that part.

[uazo]

What would be the strategy for the existing subresource filter?

if we decide to keep adblockplus, we should delete the files in the filters subfolder at the startup. that's enough to disable them.

This patch does not seem to touch that part.

I wanted to test the new patch on my real device first.
I'll let you know if I detect any problems, including performance problems (not so much because of eyeo, but our other patches).

[uazo]

I verified that from a user point of view, the adblock works correctly and does not seem to have an impact on general performance.
it seems to me that the site setting is not connected, so that need should be developed.
in my opinion it is also useful to add the configurable timeout for subscription downloads, together with some infobars.

I'm starting a deeper read of the code, for what my skills enable me and in the most transparency as possible.
it is in no way meant to be a judgment on the eyeo code and have pity for my mistakes.
these are the points I would like to check better:

• the code allows modification of the response headers
  clarify which ones and if only blocking is possible
• the code allows the modification of the csp
  clarify which ones and if only blocking is possible
• the code allows the url rewrite (but seems only to internal resources)
  however to be verified
• understand if any error in "elemhide_for_selector.jst" not currently trapped with try / catch can arrive at the page as window.onerror
• check if we are actually blocking the snippets, which being injected js code decided by external filters may not be suitable for bromite
• understand why in the generation of the css string (in js emulation) the url is also injected.
• the code predicts that localhost is never blocked but it is to check if the calls to localhost are already blocked by bromite
• the code "installs" a proxy in the UrlLoaders, check the functionality better
• there is some sort of metadata in the subscription file (https://eyeo.gitlab.io/adblockplus/abc/core-spec/#appendix-filter-list-syntax), understand what it is for
• check better what the "sitekeys" and the "special filters" are
• css injection seems to block the BackForwardCache
• check converter.cc, if any rule is not suitable for bromite
• check for redirect_info.bypass_redirect_checks

[gothmog123]

we users thank you for your great work. this will make bromite the best android browser. any ETA on when this is coming? im stoked

[csagan5]

• check better what the "sitekeys" and the "special filters" are

sitekeys are a feature for whitelisted ads IIRC

• check if we are actually blocking the snippets, which being injected js code decided by external filters may not be suitable for bromite

I would be okay with this, trusted 3rd-party filters usually only inject JavaScript running locally (and not reaching to any network resource), although we probably cannot prevent this without some sort of sandbox.

I verified that from a user point of view, the adblock works correctly and does not seem to have an impact on general performance.
it seems to me that the site setting is not connected, so that need should be developed.

Which site setting? The one for allowing the ads?

[uazo]

so, even with all those doubts, you agree to proceed anyway?
That's fine with me, I'll proceed with the checks.

Please note that the production of the patch with the eyeo code base is also to be integrated into the life cycle of bromite.

check if we are actually blocking the snippets

I would be okay with this

me not yet.
how would I check them? I would prefer not to have the snippets active.

The one for allowing the ads?

Yes.

any ETA on when this is coming?

@gothmog123 absolutely not known

[PaperCutBad]

Is it easier to integrate than uBlock Origin?

without a shadow of a doubt. it is already there, all ready and well done. I did very little, at the moment. we need to understand if it is also easy to maintain, we'll see.

What about Brave's filtering method? I think Eyeo's AdBlock Plus doesn't support all syntax of uBlock Origin but brave's adblock seems to support them. However implementing any of them will be a huge plus for cosmetic filtering.

[csagan5]

so, even with all those doubts, you agree to proceed anyway?

I haven't tested it yet, will let you know.

check if we are actually blocking the snippets

I would be okay with this

me not yet. how would I check them? I would prefer not to have the snippets active.

AFAIK all these snippets do is declare local variables/functions that do not reach to any third-party server; to avoid that a compromise in filter lists translates in a browser compromise we should have sandboxing on those snippets to disallow reaching anything outside the first-party domain.

The one for allowing the ads?

Yes.

So are technically both filtering active at all times?

[uazo]

to disallow reaching anything outside the first-party domain.

we can try

So are technically both filtering active at all times?

can be, without any problems apparently.

I haven't tested it yet, will let you know.

I am experiencing performance problems due to this adblock when browsing at https://source.chromium.org, verified by entering that domain in the excluded list via the ui of AdBlock Plus. I do not know the reason, it could be trivially blocked urls slowing down navigation, I still have to check.

[uazo]

I confirm that there is a general performance issue caused by the rules inserted in the css not linked to any url (and therefore always injected for every navigation).
the 'basic' css is very huge and makes the paint phase slow: this is visible with as many (html) nodes as there are in codesearch while with lighter pages it does not appear.
let's see if I can find a way out.

[uazo]

I found a way. Basically, that huge css was injected several times, each time the url was changed, but also with the same document.

diff --git a/third_party/blink/renderer/core/css/style_engine.cc b/third_party/blink/renderer/core/css/style_engine.cc
index 1ede854d10..e97ab7b09d 100644
--- a/third_party/blink/renderer/core/css/style_engine.cc
+++ b/third_party/blink/renderer/core/css/style_engine.cc
@@ -499,6 +499,14 @@ void StyleEngine::UpdateActiveStyleSheetsInShadow(
   }
 }
 
+bool StyleEngine::ActiveUserStyleSheetsContainsKey(const StyleSheetKey& injection_key) {
+  for (auto& sheet : injected_user_style_sheets_)
+    if (sheet.first == injection_key)
+      return true;
+
+  return false;
+}
+
 void StyleEngine::UpdateActiveUserStyleSheets() {
   DCHECK(user_style_dirty_);
 
diff --git a/third_party/blink/renderer/core/css/style_engine.h b/third_party/blink/renderer/core/css/style_engine.h
index d7cc3f59b6..9e1693401c 100644
--- a/third_party/blink/renderer/core/css/style_engine.h
+++ b/third_party/blink/renderer/core/css/style_engine.h
@@ -236,6 +236,7 @@ class CORE_EXPORT StyleEngine final : public GarbageCollected<StyleEngine>,
   }
   void ViewportRulesChanged();
 
+  bool ActiveUserStyleSheetsContainsKey(const StyleSheetKey& injection_key);
   void InjectSheet(const StyleSheetKey&,
                    StyleSheetContents*,
                    WebCssOrigin = WebCssOrigin::kAuthor);
diff --git a/third_party/blink/renderer/core/exported/web_document.cc b/third_party/blink/renderer/core/exported/web_document.cc
index 0cdd3f6515..84e83e6694 100644
--- a/third_party/blink/renderer/core/exported/web_document.cc
+++ b/third_party/blink/renderer/core/exported/web_document.cc
@@ -268,6 +268,16 @@ WebStyleSheetKey WebDocument::InsertAbpElemhideStylesheet(
   Document* document = Unwrap<Document>();
   DCHECK(document);
 
+  const WebStyleSheetKey& injection_key =
+      key && !key->IsNull() ? *key : WebString::FromUTF8("abp");
+  DCHECK(!injection_key.IsEmpty());
+
+  // Check if the css in already present
+  if (document->GetStyleEngine().ActiveUserStyleSheetsContainsKey(injection_key)) {
+    LOG(WARNING) << "[eyeo] Skip add css as already in collection";
+    return injection_key;
+  }
+
   auto* parsed_sheet = MakeGarbageCollected<StyleSheetContents>(
       MakeGarbageCollected<CSSParserContext>(*document));
   parsed_sheet->ParseString(source_code);
@@ -289,9 +299,6 @@ WebStyleSheetKey WebDocument::InsertAbpElemhideStylesheet(
         {SchedulingPolicy::DisableBackForwardCache()});
   }
 
-  const WebStyleSheetKey& injection_key =
-      key && !key->IsNull() ? *key : GenerateStyleSheetKey();
-  DCHECK(!injection_key.IsEmpty());
   document->GetStyleEngine().InjectSheet(injection_key, parsed_sheet, origin);
   return injection_key;
 }

@mpawlowski-eyeo sorry to bother you, but do you think it might be right?

[mpawlowski-eyeo]

Hey, we fought against injecting that CSS multiple times but perhaps we still have a bug somewhere - I predict e.g. during back-forward navigations we could be re-injecting it. I like this idea of checking the injection key, I'll make a ticket to analyse that and include in our code if it works :)

Good analysis of our code so far, I can answer some questions:

the code allows modification of the response headers
clarify which ones and if only blocking is possible

This is for "header filters": https://help.eyeo.com/adblockplus/how-to-write-filters#header-filter
You can see examples of such filters here if you search for $header. They will block responses that contain the header defined by the filter. We don't modify the response headers via header filters.

the code allows the modification of the csp
clarify which ones and if only blocking is possible

This is to support rare CSP filters: https://help.eyeo.com/adblockplus/how-to-write-filters#content-security-policies
Some sites check if a response (particularly, script) arrives to detect situations when a request was blocked by an ad-blocker. Sometimes the site can be "tricked" by letting the resource arrive but with a response header that violates CSP, so the site is forced to discard it.

You can see examples of such filters on easylist if you search for $csp.
In this case we do modify the response headers.

the code allows the url rewrite (but seems only to internal resources)
however to be verified

Rewrites are indeed only to predefined internal resources. Some pages check for presence of a resource to determine if ad-blocking interfered with a fetch, in which case we can pretend the resource arrived but replace it with an empty image or one-pixel video instead of the ad.

These filters are also rare, examples on the anti-circumvention list.

understand if any error in "elemhide_for_selector.jst" not currently trapped with try / catch can arrive at the page as window.onerror

I'm actually unsure. We can try sanitizing this better if you discover problems, let me know.

check if we are actually blocking the snippets, which being injected js code decided by external filters may not be suitable for bromite

Something often misunderstood about snippets: the actual JS code that gets injected is static and built-in. There's a library of functions that are allowed to be executed. The filters may only select which of the predefined functions they want to be called on a site, and with what arguments. So we're not executing live code downloaded from the Internet.

At the same time, we know it can still feel controversial, and perhaps there could be ways to exploit this mechanism, so filter lists that are not "privileged" are not allowed to have snippet filters. AFAIR the only privileged filter lists are https://easylist-downloads.adblockplus.org/abp-filters-anti-cv.txt and another one we use for testing.

Setting every filter list to be non-privileged should effectively disable executing snippets.

understand why in the generation of the css string (in js emulation) the url is also injected.

That's actually a good question, this code predates me and is a carryover from the implementation in the extension. I'll try to ask around.

the code predicts that localhost is never blocked but it is to check if the calls to localhost are already blocked by bromite

I don't understand this one - does Bromite do something funky with localhost urls?
We assume localhost resources are "clean" mostly to make the various unit and browser tests in Chromium to pass without being interrupted by our code. In principle, we could filter localhost requests too.

the code "installs" a proxy in the UrlLoaders, check the functionality better

In the past we used blink::URLLoaderThrottle but they didn't allow us to implement rewrite and header filters. They only allowed us to block or allow requests but not to modify their content.

there is some sort of metadata in the subscription file (https://eyeo.gitlab.io/adblockplus/abc/core-spec/#appendix-filter-list-syntax), understand what it is for

Most of it is unused, as you probably noticed. Some of it was used for displaying mete info in the extension UI and is ignored by the native SDK. Expiration time is parsed to figure out how often we download updates.

check better what the "sitekeys" and the "special filters" are

Sitekeys are a constraint that can be placed on filters that basically mean "only consider this filter if the server has sent a matching sitekey in the x-adblock-key response header". These can be used to enable additional allowing filters for "friendly" servers. The servers need to be aware of the mechanism, I can send a pdf with a description of how it works if you're interested.

Pretty rare in practice, we actually find it hard to find sites that serve these even for our internal testing.

css injection seems to block the BackForwardCache

Sounds like a bug, if you can tell me how to reproduce it I'll report it to the team.

check for redirect_info.bypass_redirect_checks

This is a hack I don't like but we didn't find a better way to implement redirect filters. Chromium has a slew of internal checks that disallows redirecting in-flight requests for safety, but that's exactly what we need to do for rewrite filters.

[uazo]

thank you!

Hey, we fought against injecting that CSS multiple times but perhaps we still have a bug somewhere - I predict e.g. during back-forward navigations we could be re-injecting it. I like this idea of checking the injection key, I'll make a ticket to analyse that and include in our code if it works :)

Glad to be of help!

Good analysis of our code so far

thank you :)

I can answer some questions:

most kind, I will study your answers carefully.

css injection seems to block the BackForwardCache

Sounds like a bug, if you can tell me how to reproduce it I'll report it to the team.

I will only quickly respond to this one: is the chromium code that deactivates it if there is an injected css (or js), notoriously from extensions. but for me the back-forward cache should always be deactivated, at least until I understand how it works (ot: I am afraid that deleting site storage serves little purpose if the value of javascript variables is retained between cross site back/forward navigation), so (for now) it is not a bug :)
in any case I will check it, if I find a way to reproduce it I will let you know.

[intrnl]

check if we are actually blocking the snippets

I suppose this can be a toggle where JS scriptlets are blocked by default?

fwiw I see scriptlets as a necessity for defusing sites that tries to check if their ads are getting blocked, and can even aid in compatibility as well since these scriptlets can inject placeholder code so that the site's JS doesn't throw an exception.

(the concerns are well and valid, I'm just not sure that not providing the ability to have scriptlet injections at all is a good idea)

[mpawlowski-eyeo]

Snippets are also what lets us block video ads on youtube and some other sites that use more advanced methods of delivering ads, so disabling them comes at a cost for user experience

[uazo]

Snippets are also what lets us block video ads on youtube

currently it seems to me that youtube advertising videos are blocked even without snippets, but if you tell so I probably failed to block snippets, I will check, thanks

[snakelizzard]

Hello @uazo. For case of upgrading adblock patch, you can use tools/adblock/interdiff.py.
Given you provided commit ranges for eyeo SDK in different Chromium branches, it will generate the diff of what was changed between version A and version B.
Occasionally, it will skip some files for manual merge, but in general, it is good even for cases when there were many changes.

[uazo]

@snakelizzard thank you, I had seen it, but for now I prefer my solution that does not force me to checkout locally.
I can then interdiff on patch revisions

[csagan5]

Something often misunderstood about snippets: the actual JS code that gets injected is static and built-in. There's a library of functions that are allowed to be executed. The filters may only select which of the predefined functions they want to be called on a site, and with what arguments. So we're not executing live code downloaded from the Internet.

At the same time, we know it can still feel controversial, and perhaps there could be ways to exploit this mechanism, so filter lists that are not "privileged" are not allowed to have snippet filters. AFAIR the only privileged filter lists are https://easylist-downloads.adblockplus.org/abp-filters-anti-cv.txt and another one we use for testing.

Setting every filter list to be non-privileged should effectively disable executing snippets.

We could vet this part and allow it, and then review snippets each time they change.

[csagan5]

@uazo related: ungoogled-software/ungoogled-chromium#662 (comment)

[uazo]

related: ungoogled-software/ungoogled-chromium#662 (comment)

What exactly did you want me to look at?
I, theoretically, as far as I understand how it works, do not completely disagree with manifest v3, it seems to me to be a good solution with regard to the privacy problems that extensions might have.
Of course, I am only saying this lightly because I have been using this patch for months :), otherwise it would be a big problem.

[uazo]

@mpawlowski-eyeo very very tiny bug:

diff --git a/components/adblock/core/subscription/ongoing_subscription_request_impl.cc b/components/adblock/core/subscription/ongoing_subscription_request_impl.cc
index 79df9f3a04..25d321c2e2 100644
--- a/components/adblock/core/subscription/ongoing_subscription_request_impl.cc
+++ b/components/adblock/core/subscription/ongoing_subscription_request_impl.cc
@@ -52,7 +52,6 @@ const net::NetworkTrafficAnnotationTag kTrafficAnnotation =
           setting:
             "You enable or disable this feature via 'Adblock Enable' pref."
           policy_exception_justification: "Not implemented."
-          }
         })");

[mpawlowski-eyeo]

Thanks, fixing :)

[mpawlowski-eyeo]

Hey, I took a look at your changes and found some issues:

• You disable snippets by adding

+  // Always forbid snippets and header filters
+  if ((true)) return false;

but you also bring in the snippets library from a submodule into the top-level repo:

 create mode 100755 components/resources/adblocking/snippets/dist/isolated-first.jst
 create mode 100755 components/resources/adblocking/snippets/dist/isolated-first.source.jst

If you don't intend to use snippets, you can get rid of those .jst files and also of IDR_ADBLOCK_SNIPPETS_JS.
You will also then need to remove copy_snippets_lib target from components/resources/adblocking/BUILD.gn and perhaps #ifdef out some code in components/adblock/content/browser/element_hider_impl.cc that uses IDR_ADBLOCK_SNIPPETS_JS (GenerateSnippetScript())

• You disable preloaded subscriptions by doing

-      {{"*easylist.txt", IDR_ADBLOCK_FLATBUFFER_EASYLIST},
-       {"*exceptionrules.txt", IDR_ADBLOCK_FLATBUFFER_EXCEPTIONRULES},
-       {"*abp-filters-anti-cv.txt", IDR_ADBLOCK_FLATBUFFER_ANTICV}};
+      {};

and

-    "//components/resources/adblocking:make_all_preloaded_subscriptions",

You can follow this up with removing the .txt files from components/resources/adblocking

These two changes should result in a smaller patch, with fewer big resource/data files.

FWIW we are planning to switch to a patch-based delivery method and one of the ideas is to make snippets and preloaded subscriptions optional patches. This would mean in the future perhaps you would no longer need to delete the code on your end, you'd just pick a different set of patches from our repo. What goes into a "base" patch and what goes into "optional" patches is not decided yet, we'll be using your diff as a data point tho ;)

[mpawlowski-eyeo]

Ah one more thing, you wrote that you disabled "Downloads in build phase (Preloaded Subscriptions)"

Preloaded subscriptions are not downloaded during a build. There are .txt files with filter list content committed into the repo and static - during the build they are converted to .fb binary files and then embedded in the ResourceBundle. This only happens when needed, e.g. when a flatbuffer schema changes, that's why it's a dynamic build step - we used to have the .fb files committed into the repo but then we had to remember to update them when we changed code that influences conversion output.

Before every release, we run components/resources/adblocking/update.sh to download new versions of those .txt lists and then commit them into the repo. The process is manual and doesn't happen during the build. So there's no network dependency during compilation.

[uazo]

@mpawlowski-eyeo always nice thanks.
I am sorry I have not yet fully considered your reports, I promise to find some time as soon as possible.

If you don't intend to use snippets

I've been using this patch for months, in both android and windows versions, and I don't miss the snippets, because most sites don't detect any adblock! great result, congratulations! i still don't understand why ublock (which i have among the extensions anyway) always detects something extra, maybe some difference between the rules, i don't know yet. but as I am using 3 adblocks at the same time (the standard one from bromite, yours and ublock) I have to decide to put some logs to see which one is working better.

These two changes should result in a smaller patch, with fewer big resource/data files.

fully agree, I will do so.

we are planning to switch to a patch-based delivery method

great, that would be very useful for us, I still have some difficulty in understanding which branch is best to use. so I would have solved it, I think.

you would no longer need to delete the code on your end

even better! would speed up the introduction, by us and perhaps also in other chromium forks.
I do not understand how you want to eliminate telemetry, but we shall see.

you'd just pick a different set of patches from our repo.

you probably have some technical reason: why don't you consider simpler gn switches with good build documentation instead of generating different patches? wouldn't it be easier to maintain? and especially, thinking like a company, although I have not yet understood your business model, you could indicate the optimal configuration and those possible but not officially managed.

we'll be using your diff as a data point tho ;)

always happy to be of help

Preloaded subscriptions are not downloaded during a build.
So there's no network dependency during compilation.

thank you, I was wrong...

[uazo]

@mpawlowski-eyeo sorry to keep writing, but I think I found a bug in 109 sdk.

Basically the order of initialization of the services seems incorrect, for which the subscription_service_ of AdblockWebContentObserver did not have initialised the value of configuration_.
For now I've fixed it with 00109-Fix-Adblock.patch, so I'm sure that SubscriptionServiceImpl::InstallFilteringConfiguration is called before being used in the observer and then everything works perfectly.

[mpawlowski-eyeo]

Good catch! Wonder why our tests didn't find that...

I think a more elegant fix would be something like this:

diff --git a/chrome/browser/adblock/adblock_controller_factory.cc b/chrome/browser/adblock/adblock_controller_factory.cc
index 94379bca6bbea..a02dfe2e7c9e4 100644
--- a/chrome/browser/adblock/adblock_controller_factory.cc
+++ b/chrome/browser/adblock/adblock_controller_factory.cc
@@ -112,4 +112,9 @@ void AdblockControllerFactory::RegisterProfilePrefs(
   adblock::prefs::RegisterProfilePrefs(registry);
 }
 
+bool AdblockControllerFactory::ServiceIsCreatedWithBrowserContext()
+    const {
+  return true;
+}
+
 }  // namespace adblock
diff --git a/chrome/browser/adblock/adblock_controller_factory.h b/chrome/browser/adblock/adblock_controller_factory.h
index 6832cb2228d64..944201c315502 100644
--- a/chrome/browser/adblock/adblock_controller_factory.h
+++ b/chrome/browser/adblock/adblock_controller_factory.h
@@ -43,6 +43,7 @@ class AdblockControllerFactory : public BrowserContextKeyedServiceFactory {
       content::BrowserContext* context) const override;
   void RegisterProfilePrefs(
       user_prefs::PrefRegistrySyncable* registry) override;
+  bool ServiceIsCreatedWithBrowserContext() const override;
 };
 
 }  // namespace adblock

We probably want to create AdblockController (and have it register its configuration with SubscriptionService) ASAP and automatically, without an explicit instantiation somewhere "early" in the code.

I haven't tested this fix yet, I'll make an internal ticket to do it :)