Opened 4 years ago

Closed 4 years ago

Last modified 6 weeks ago

#3687 closed change (fixed)

Add experimental support for content blockers on Safari 9

Reported by: sebastian Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-1.12-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: kzar, greiner, fhd, athornburgh, Ross, scheer Blocked By: #3671, #3788, #3870, #3910, #3956, #4141
Blocking: Platform: Safari
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29338027/
https://codereview.adblockplus.org/29340571/

Description (last modified by kzar)

Background

With #3671, abp2blocklist can no longer only be used from the command line, but also provides an API that could be bundled and used in Adblock Plus, in order to convert Adblock Plus filters to content blocker rules suported in Safari 9.

Moreover, there is a chance that the old, now deprecated, mechanism, we currently rely on, will be removed in Safari 10. Besides that, the new content blocker mechanism is also more efficient.

So we should start to support the new content blocker mechanism, based on abp2blocklist, as optional feature.

What to change

  • Integrate abp2blocklist as depdency and process/bundle it's API using jsHydra.
  • Adapt existing code for compatibility with abp2blocklist:
    • Split the basedomain related logic out of the url module, into a module called tldjs and make the API compatible with the respective node.js library.
    • Turn punycode into a module rather than a global object.
  • Add a preference to switch between the classic blocking mechanism (default) and the new content blocker mechanism. This preference should be exposed on the options page in the general tab, if safari.extension.setContentBlocker is available. When the preference is enabled and then disabled the content blocking API should continue to be used until Safari is restarted. (The old APIs will be disabled!) The user should also be informed that they should restart Safari.
  • If the new content blocker mechanism is enabled use abp2blocklist to convert the filters and register the resulting rules with safari.extension.setContentBlocker. Don't register a listener for the beforeload DOM event, neither apply the traditional element hiding code in that case. Also make sure to disable safari/include.youtube.js. Make sure any block counters being displayed by the ABP icon are removed. Hide the stats section of the popup.

Hints for testers

  • Try enabling the "Use Safari's native Content Blocking (experimental)" feature in the options page on a supported (>= 9) version of Safari.
  • Ensure adblocking still works. (Check the console for pages to see that some "Content blocker prevented ..." message show up in the console too.)
  • Try disabling the feature, make sure a red "Please restart Safari" message is shown to the user. Restart Safari and check that content blocking really was disabled. (Also check content blocking still works before you restart.)
  • Try whitelisting a page that would otherwise have adverts blocked, after a small delay try refreshing the page and ensure that the adverts are now displayed.
  • Try adding several subscriptions and then enabling content blocking for the first time. Ensure that content blocking is disabled automatically after the error is displayed. Make sure the "Please restart safari" is NOT displayed in that case.
  • Try enabling content blocking, then adding lots of subscriptions, then restarting Safari after the error is dispalyed. Ensure that content blocking is automatically disabled when Safari restarts and that the error message is not displayed in that case.
  • Try adding lots of subscriptions and then enabling content blocking for the first time. Ensure an error message is displayed and that content blocking is automatically disabled. Then try enabling content blocking several more times without modifying your subscriptions or filters. Ensure that the error is always displayed and that content blocking is always disabled.

Change History (35)

comment:1 Changed 4 years ago by kzar

  • Owner set to kzar

comment:2 Changed 4 years ago by kzar

  • Cc greiner added

(Adding you as CC Thomas as this involves a small change to the options page.)

comment:3 Changed 4 years ago by sebastian

  • Description modified (diff)

Yeah, we would need to add that option to the new options page as well. As for the text label of that option, I intentionally omitted it in the issue description, as it doesn't matter much, as we will revisit all texts with tech writing anyway, once the new options page is ready.

comment:4 Changed 4 years ago by fhd

  • Cc fhd added

comment:5 Changed 4 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/adblockpluschrome/rev/b357888a249a

comment:6 Changed 4 years ago by kzar

  • Review URL(s) modified (diff)

comment:7 Changed 4 years ago by kzar

  • Blocked By 3788 added

comment:8 Changed 4 years ago by kzar

  • Blocked By 3816 added

comment:9 Changed 4 years ago by sebastian

If there really is no way to make content blockers work while still using synchronous messaging (for the old options page), I guess we could simply replace the "Options" button in the toolbar icon popup with a button to disable content blockers, which will bring back the options page.

While we aim to have the new options page before Safari 10 will be released, I don't think that we should defer this feature until then. We need people to test it, that we have enough time to fix any issues, before content blockers will be mandatory.

comment:10 Changed 4 years ago by kzar

Well I'm guessing we can find a work around like you suggested, I'm working on figuring this stuff out. (At the moment I'm looking at #3788.)

In the mean time, according to Apple's API docs, this is a blocker. As soon as we use the content blocking API the legacy options page will break as use of canLoad will be disabled.

comment:11 follow-up: Changed 4 years ago by sebastian

Did you try it out? Thing is, according to the documentation you can only use canLoad inside beforeload listeners anyway. So, if beforeload events aren't fired anymore when a content blocker is registered you would theoretically also not be able to use canLoad anymore.

However, it's possible to create a fake beforeload event to use synchrnous messaging via canLoad wherever you want. And if they don't explicitly remove that API when content blockers are registered that should still keep working.

comment:12 in reply to: ↑ 11 Changed 4 years ago by kzar

Replying to sebastian:

Did you try it out?

Not yet, I'm going to but like I say I'm working on #3788 first. (If I find the documentation there was wrong I'll remove #3816 as a blocker from this issue, if not I'll try and figure out a work around like the one you suggested.)

comment:13 Changed 4 years ago by kzar

So I just tested it out...

  • As soon as you call safari.extension.setContentBlocker from the background page, with a non-undefined value (even null) canLoad sometimes stops working. (Yes it's not even 100% reliable, but it usually does!)
  • The first time you call canLoad after it stops working you get an exception "The canLoad message is not supported while using a content filter.".
  • After the first time canLoad simple returns true no matter what.
  • Once canLoad stops working I found no way to make it work again without reloading the extension! (Even calling safari.extension.setContentBlocker(undefined).)

Of course as soon as the canLoad function stops working the existing options page completely breaks.

comment:14 Changed 4 years ago by kzar

  • Blocked By 3816 removed
  • Cc athornburgh added
  • Platform changed from Unknown / Cross platform to Safari

So I've got a very rough proof of concept working. As Safari really does disable the old APIs when we use the new ones it provided some challenges, not least because the legacy options page relies on these disabled APIs!

Instead of waiting for the new options page to land we've decided to hide the button for the options page from the popup when the new API is being used. We'll also provide a toggle in the popup that allows the user to switch the new API back off again, which they wouldn't otherwise be able to do without use of the options page.

(When the user clicks to turn the new API off again we explain that they must first restart Safari for their changes to take effect. We also provide them a way to toggle it back on again, in case they clicked the button accidentally and don't want to restart Safari.)

So far the UI I have made is very ugly, I've basically copied the "Enabled for this site" toggle and called it "Use Safari Content Blocking API" or similar. I might need some help making it look pretty, Thomas / Aaron if you have any suggestions I'm all ears!

Last edited 4 years ago by kzar (previous) (diff)

comment:15 Changed 4 years ago by kzar

  • Description modified (diff)

comment:16 follow-up: Changed 4 years ago by greiner

So the only way for disabling Acceptable Ads would be to disable the extension? That might be an issue so what about creating a minimalist options page with at least that one option? In there would could also have that toggle to switch the new API off again.

comment:17 follow-up: Changed 4 years ago by sebastian

With the current approach you would have to disable content blockers from the toolbar icon popup, restart Safari, go to the options page, change the configuration, and if you'd like turn on content blockers again.

Of course this workflow isn't great, thank Apple for that! But also it's just temporary while we are stuck with the old options page. The new options page (as it doesn't rely on synchrnous messaging) will work also when content blockers are enabled, disabling content blockers will require a restart though.

Implementing yet another version of the options page for a temporary purpose goes way too far IMO. If the Acceptable Ads case is important, we could rather add an option to toggle Aceptable Ads to the icon popup when content blockers are enabled, and therefore the options page isn't available.

Or perhaps it wouldn't be too much effort to make the old options page work without asynchronous messaging. We wouldn't even have to implement everything from the scratch, but could use the message responder from adblockplusui which already supports all features we need.

comment:18 in reply to: ↑ 16 Changed 4 years ago by kzar

Replying to greiner:

So the only way for disabling Acceptable Ads would be to disable the extension? That might be an issue so what about creating a minimalist options page with at least that one option? In there would could also have that toggle to switch the new API off again.

That's true, in order to disable Acceptable Ads the user would have to disable the new API from the popup, restart Safari and then open and then old options page first.

(I suppose the other question with all this is how close is the new options page? If it's just around the corner maybe we should wait after all.)

comment:19 in reply to: ↑ 17 ; follow-up: Changed 4 years ago by kzar

Replying to sebastian:

Or perhaps it wouldn't be too much effort to make the old options page work without asynchronous messaging. We wouldn't even have to implement everything from the scratch, but could use the message responder from adblockplusui which already supports all features we need.

I'm leaning towards this option, if you guys think it makes sense I even volunteer to file an issue and give the implementation a shot. (We'll see how practical it is though, perhaps it turns out to be a complete disaster and we'll have to reconsider.)

comment:20 in reply to: ↑ 19 Changed 4 years ago by sebastian

Replying to kzar:

(I suppose the other question with all this is how close is the new options page? If it's just around the corner maybe we should wait after all.)

There is still quite some stuff to be done, before we can ship the new options page. We are kinda under pressure there, because if Safari 10 will completely remove synchronous messaging we would need the new options page by this summer. But even if we make it in time, I wouldn't want to delay experimental content blocker support until then. On the other hand, if we could make the old options page work with asynchronous messaging, this would remove the pressure from the new options page.

Replying to kzar:

Replying to sebastian:

Or perhaps it wouldn't be too much effort to make the old options page work without asynchronous messaging. We wouldn't even have to implement everything from the scratch, but could use the message responder from adblockplusui which already supports all features we need.

I'm leaning towards this option, if you guys think it makes sense I even volunteer to file an issue and give the implementation a shot. (We'll see how practical it is though, perhaps it turns out to be a complete disaster and we'll have to reconsider.)

Sounds good to me. I just hope that it doesn't turn out to be too complicated though.

comment:21 Changed 4 years ago by kzar

  • Blocked By 3870 added

comment:22 Changed 4 years ago by kzar

  • Description modified (diff)

comment:23 Changed 4 years ago by sebastian

  • Priority changed from P3 to P2

(I forgot to make this issue P2, when we put it on the roadmap)

comment:24 Changed 4 years ago by kzar

  • Blocked By 3910 added

comment:25 Changed 4 years ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:26 Changed 4 years ago by kzar

  • Blocked By 3956 added

comment:27 Changed 4 years ago by kzar

The content blocking API logs messages to the console (see below) when it blocks requests. Perhaps we could somehow use that fact to track the number of blocked requests?

Content blocker prevented frame displaying http://example.com/ from loading a resource from http://ads.example.com

comment:28 Changed 4 years ago by abpbot

A commit referencing this issue has landed:
https://hg.adblockplus.org/adblockpluschrome/rev/df6a3e0ce76a

comment:29 Changed 4 years ago by kzar

  • Cc Ross scheer added
  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:30 Changed 4 years ago by kzar

  • Description modified (diff)

comment:31 Changed 3 years ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

The only issue I've found with this so far is #4066 and that itself might be caused by the machine setup. The actions in the test notes all work as described (Thanks for the comprehensive notes!) and I haven't been able to massively break it so far (except for the one mentioned above). The feature is hidden on Safari < 9 as expected.

ABP 1.11.0.1606
Safari 6.1 / OS X 10.8
Safari 7.1.8 / OS X 10.9
Safari 8.6 / OS X 10.10.3
Safari 9.1 / OS X 10.11

Last edited 3 years ago by Ross (previous) (diff)

comment:32 Changed 3 years ago by kzar

Yes, I have witnessed Safari crashing like that occasionally when using content blocking lists. (When loading a JSON file myself using the extension builder tools and when dynamically generating and loading one inside of Adblock Plus.) I didn't notice a pattern to the crashing though and it was very rare for me. Ultimately I'm not sure what we can do here other than to wait for Apple to fix their software :(

comment:33 Changed 3 years ago by sebastian

In the past, I observed Safari crashing when running JavaScript code that blocks the thread for more like a second or so. So given that converting filters is a quite CPU heavy operation, this might also be the issue here, also considering that it seems to happen more often on the (slow) test machines than it does in your (presumely faster) development environment.

Perhaps, we can add some zero timeouts to the filter conversion code. That could not only help with those crashes, but also gives other code a chance to run while converting filters, e.g. to avoid our UI from freezing.

comment:34 Changed 3 years ago by trev

  • Blocked By 4141 added

comment:35 Changed 6 months ago by Arvind098

spam

Last edited 6 weeks ago by kzar (previous) (diff)
Note: See TracTickets for help on using tickets.