Update adblockpluschrome's safari bookmark dependencies to include WebRTC support, content blocker rule merging, and other recent changes to abp2blocklist

[mjethani]

Background

There have been recent improvements to abp2blocklist, including WebRTC support, rule merging, and a few others (#5248, #4329, #4327, #5332, #5345, #5325, #5283, #3673). At some point we need to update the dependencies in adblockpluschrome's safari bookmark to take advantage of these improvements.

What to change

Update the dependencies file to point to the latest version of abp2blocklist.

Since the safari version of adblockpluschrome's adblockpluscore dependency does not support WebRTC yet, patch RegExpFilter.typeMap in lib/requestBlocker.js by adding a new property called WEBRTC with a value of 256.

Make appropriate changes to safari/contentBlocking.js to use the new asynchronous version of the generateRules function.

Hints for testers

This update should affect the behavior of the extension only when Safari's native content blocking is enabled.

Please see the issues #5248, #4329, #4327, #5332, #5345, #5325, #5283, and #3673 for details on all the changes that have gone into this update. Ad blocking as well as whitelisting should be a lot more accurate now.

In some cases it is not possible to translate an EasyList filter exactly into a native content blocking rule. In such cases, we err on the side of not blocking rather than blocking. There will still be some false negatives (ads not being blocked), but there should be few or no false positives. Any false positive, especially where the content has been explicitly whitelisted but is still being blocked, should be considered a bug.

After a new filter subscription is added, it can take up to 30 seconds for it to take effect, and this delay is expected where the filter list is too large (e.g. currently EasyList+EasyList Germany+AA). During this delay the browser may slow down as the extension compiles the rule set in the background, but it should not freeze completely.

In some cases the generated rule set may be too large despite #3673 and the extension will throw an error as it did before this update.

[mjethani]

Let's discuss what to do about WebRTC.

[mjethani]

A simple solution for WebRTC for now is to change this line in lib/abp2blocklist.js:

const typeMap = filterClasses.RegExpFilter.typeMap;

To this:

const typeMap = Object.assign({}, filterClasses.RegExpFilter.typeMap);

// Since the Safari version of adblockpluscore does not support WebRTC yet,
// we simply hardcode it in here. This should be removed once the Safari extension
// has upgraded to the latest adblockpluscore.
if (!typeMap.WEBRTC)
  typeMap.WEBRTC = 256;

[kzar]

IMO that comment should mention _why_ the adblockpluscore dependency is old for the safari branch of adblockpluschrome, otherwise the reader will probably think we should just update the adblockpluscore dependency instead.

Also I wonder if that workaround works as we expect, since when the filters are parsed the core code wont know about the WEBRTC request type, so that bit could be set wrongly. (I'm not saying this is a problem, just that we need to figure out if it is.)

While your suggestion is preferable IMO another option could be to create a safari branch of the adblockpluscore repository which is pointed to the latest revision of adblockpluscore that old versions of Safari + JS Hydra can handle, then backporting the WEBRTC commit on to that.

I suppose a third option could be to simply modify filterClasses.RegExpFilter.typeMap when the extension is first loaded, before any filters are processed. We would do this somewhere in the adblockpluschrome code, maybe safari/ext/common.js.

Do you have an opinion Wladimir? For reference ​the discussion started in this review. The problem is that users with a recent Safari version which supports WebRTC could have those connections blocked wrongly when in content blocking mode unless we do something. The alternative is to make non-trivial changes to abp2blocklist so that the rules generated don't match WebRTC connections if WEBRTC isn't present in the typeMap.

[trev]

Let's wait for Sebastian, but IMHO we shouldn't touch the Safari extension unless there are critical issues. The significant changes proposed here would be an unreasonable use of development and QA resources, with the Safari extension ecosystem being de facto dead.

[mjethani]

Replying to kzar:

Also I wonder if that workaround works as we expect, since when the filters are parsed the core code wont know about the WEBRTC request type, so that bit could be set wrongly. (I'm not saying this is a problem, just that we need to figure out if it is.)

If core doesn't support WebRTC but we add typeMap.WEBRTC locally in lib/abp2blocklist.js, then this is what happens:

For the filter foo, it generates the following rule:

{
  "trigger": {
    "url-filter": "^[^:]+:(//)?.*foo",
    "resource-type": [
      "image",
      "style-sheet",
      "script",
      "font",
      "media",
      "raw"
    ]
  },
  "action": {
    "type": "block"
  }
}

i.e. abp2blocklist still thinks the filter covers WebRTC, because the content type has all bits set (i.e. literally 1111111111111111111110111111). All is good from abp2blocklist's point of view. This is the use case we're really concerned about. The only additional thing we would have to do here is add two exceptions for ^stuns?: and ^turns?: respectively.

For the filter foo$xmlhttprequest,websocket,webrtc, it should still generate the same URL pattern but with only the raw resource type, but the filter is considered invalid by core, therefore dropped. I haven't seen a filter like this in practice; even if it did occur, it would be relatively OK to drop a filter like that.

Without adding typeMap.WEBRTC in lib/abp2blocklist.js (which is the case right now), for the filter foo, it generates the following rule set:

[
{
  "trigger": {
    "url-filter": "^wss?://.*foo",
    "resource-type": [
      "raw"
    ]
  },
  "action": {
    "type": "block"
  }
},
{
  "trigger": {
    "url-filter": "^https?://.*foo",
    "resource-type": [
      "image",
      "style-sheet",
      "script",
      "font",
      "media",
      "raw"
    ]
  },
  "action": {
    "type": "block"
  }
}
]

This is the problem.

Too many filters have no options, therefore this really bloats the size of the rule set taking it well beyond the 50,000-rule limit.

[mjethani]

We could also just leave the Safari extension as it is and focus on iOS for now.

[mjethani]

I should add that there is also a usability issue with the new asynchronous version of generateRules. If the user adds filter lists in quick succession, we need to make sure we always have a rule set based on the user's last selection. The rule set passed to Safari finally should be based on the filter lists selected by the user. The new generateRules can take up to 15 seconds to do its job (because of rule merging), which means we have to either cancel the last asynchronous rule generation operation when a new selection is made or just wait for it to complete before generating a new rule set again based on the latest selection. The UI finally must reflect the rule set actually being used by Safari (which should in turn be based on the user's selection). Right now it's all likely to go out of sync.

Plus, if the promise returned by generateRules gets rejected because of an error somewhere in the chain, this needs to bubble up as well.

In short, there are also a few more changes required to safari/contentBlocking.js in adblockpluschrome if we want to upgrade to the latest version of abp2blocklist.

[sebastian]

Replying to trev:

Let's wait for Sebastian, but IMHO we shouldn't touch the Safari extension unless there are critical issues. The significant changes proposed here would be an unreasonable use of development and QA resources, with the Safari extension ecosystem being de facto dead.

We need the changes in abp2blocklist, anyway, for Adblock Plus for iOS/macOS. And updating the dependency and integrating the changes in Adblock Plus for Safari, allows us to test these changes sooner and with a broader scope.

Also note that currently users of Adblock Plus for Safari have to opt-in in order to use Content Blockers, plus that features is labelled experimental in Adblock Plus for Safari.

However, if with the next major update of Safari, Content Blockers will become the only way to block ads, these changes might get much more important for Adblock Plus for Safari.

[trev]

It's your call of course. I'm not really convinced that the development and testing effort is justified, even if we can reuse some of that effort for abp2blocklist. Adblock Plus for Safari is a considerably more complicated product to test, nor is QA going to skip testing abp2blocklist merely because same changes have been tested in Adblock Plus for Safari.

[sebastian]

I consider the development resources we'd save by rejecting this issue, now, neglectable. As far as testing goes, you are right, this will be some effort, but I think it is worth it as we might find bugs already which otherwise will become an issue later on, with our other products that rely on abp2blocklist. In particular since Adblock Plus for Safari, is currently our only product for desktop, supporting Content Blockers. And this is unlikely to change before Safari 11 is released. However, if Safari 11 will drop support of event based blocking (which is already deprecated for quite a while), we have to address the improvements to Content Blocker support this dependency update will introduce, anyway, as (among others) it mitigates false positives, hence improves support of Acceptable Ads.

[kzar]

My opinion is to go for it this time, adding WEBRTC to the RegExpFilter.typeMap from safari/ext/common.js and updating the abp2blocklist dependency. That way we don't have to mess with adblockpluscore nor updating that dependency, but we'll get the benefit of Manish's abp2blocklist improvements (and that old sitekey fix).

[sebastian]

Sounds good to me.

[abpbot]

A commit referencing this issue has landed:
​Issue 5464 - Upgrade to new asynchronous version of abp2blocklist

[kzar]

Please could you add a "Hints for testers" section to the description with testing instructions?

[kzar]

Thanks, looks good now.

Heads up Ross, this one will need quite a bit of testing before we do the next Safari release.