1557009 - GeckoView: Allow setting tracking protection exceptions

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

I just came across this code in Fenix:
https://github.com/mozilla-mobile/fenix/blob/94881f824190054d8c2395e2740c3a075dcce047/app/src/main/java/org/mozilla/fenix/AppRequestInterceptor.kt#L26-L42

For every load request it looks up the host of the URL in an exception list and then updates the tracking protection policy on the GeckoSession. This looks fragile and may be expensive for every request.

I wonder what the GeckoView team thinkgs about this and if we could do better? Are exceptions something Gecko(View) could or should deal with? I assume there's already some mechanism available since Firefox desktop allows exceptions too.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Flagging Eugen for input since he added the tracking protection policy API to GeckoView.

Comment 2

[Eugen Sawin [:esawin]]

I think content blocking exceptions are best handled in Gecko(View).
An exceptions API should reduce the app's onLoadRequest handler complexity and ensure consistent behavior in respect to private mode etc.

@Sebastian: can you help us prioritize this?

@Ehsan: Do you see a better way to implement the GV content blocking exceptions API than by exposing a sensible subset of nsIPermissionManager?

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

An exceptions API should reduce the app's onLoadRequest handler complexity and ensure consistent behavior in respect to private mode etc.

Sounds great 👍

@Sebastian: can you help us prioritize this?

At this time it's too late for Fenix MVP, I think. But it would be nice to replace the current implementation in one of the next updates.

Comment 4

[(no longer active)]

(In reply to Eugen Sawin [:esawin] from comment #2)

@Ehsan: Do you see a better way to implement the GV content blocking exceptions API than by exposing a sensible subset of nsIPermissionManager?

OMG, please don't do comment 0! Not only the answer to your question is yes but also the anti-tracking backend expects to perform these checks itself and the impacts of something higher level trying to imitate those checks is actually unknown to me, all I can tell you is that it will cause very hard to diagnose bugs and it will surely regress performance since the anti-tracking backend code for this has been well optimized since as comment 0 notices it is on the critical path of page load performance.

The anti-tracking API for checking whether something is on the allow list is here: https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/toolkit/components/antitracking/AntiTrackingCommon.h#137. This API is used automatically by all of our url classifiers without you needing to do anything to manage it. It is rather low-level and may or may not be directly suitable for you. Generally I don't think GV or Fenix should even need to check whether the page is on the block list in the first place. If there is such a need perhaps our abstractions are leaking some info somewhere, let's discuss how we can fix that?

For putting a page on the allow list on desktop we use enableForCurrentPage() and disableForCurrentPage() does the opposite. This is what needs to be exposed through GV to consumers so that they can manipulate the allow list. And this is how we check to see if a site is on the allow list.

Perhaps this logic should be abstracted into a JSM and shared between desktop and GV?

Comment 5

[Eugen Sawin [:esawin]]

(In reply to :Ehsan Akhgari from comment #4)

Perhaps this logic should be abstracted into a JSM and shared between desktop and GV?

That would be great. My proposal to expose a subset of nsIPermissionManager was based on the idea of duplicating that logic in GV.
Do you think we can find an owner for the reusable JSM refactoring? Otherwise, I can look into it as part of this bug.

Comment 6

[Johann Hofmann [:johannh]]

Yeah, I agree with Ehsan, it would be great if you could just set the permission and let Gecko handle everything else. A few things to note about this permission:

• The permission manager only stores origins (which include the scheme), but the url-classifier only deals with hostnames (meaning no scheme), so we need to do a bit of ugly conversion and simply always save the permission with the https:// scheme as a convention.

• There are actually two different permissions for content blocking, one is called "trackingprotection" and the other "trackingprotection-pb". The former is accessed in normal browsing while the latter is for private browsing mode and needs to be set with EXPIRE_SESSION scope.

• In browser-contentblocking we still use URI-based permission manager APIs but we would like to deprecate those in the near future (bug 1402957), in favor of always using principal-based APIs. So if you design a new API for GeckoView I would really appreciate if it would be principal-only (or at least based on origins) from the start :)

(In reply to Eugen Sawin [:esawin] from comment #5)

(In reply to :Ehsan Akhgari from comment #4)

Perhaps this logic should be abstracted into a JSM and shared between desktop and GV?

That would be great. My proposal to expose a subset of nsIPermissionManager was based on the idea of duplicating that logic in GV.
Do you think we can find an owner for the reusable JSM refactoring? Otherwise, I can look into it as part of this bug.

I think this is a good idea. Note that we already have the SitePermissions.jsm module on desktop, which solves a bunch of things such as temporary permissions (that only apply for the current tab). I don't think it's a good idea to use that as a basis since it's very much geared towards desktop. It might be interesting for inspiration, though.

It would be awesome if your or someone from your team would give this a shot. I think both Ehsan and I are very happy to review and advise.

Thanks!

Comment 7

[(no longer active)]

If you file a bug and needinfo me on it I'd be happy to even put together a patch next week. :-) Should be very simple.

Comment 8

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #7)

If you file a bug and needinfo me on it I'd be happy to even put together a patch next week. :-) Should be very simple.

I filed bug 1557872 for it.

Comment 9

[Chenxia Liu [:liuche]]

We're looking at putting this into the next sprint or two on Fenix - I haven't seen any status updates on this bug, so let me know what work remains to be done and what version of GV we could expect this to be in!

Comment 10

[Chris Peterson [:cpeterson]]

(In reply to Chenxia Liu [:liuche] from comment #9)

We're looking at putting this into the next sprint or two on Fenix - I haven't seen any status updates on this bug, so let me know what work remains to be done and what version of GV we could expect this to be in!

Eugen, Ehsan already wrote a JS module for manipulating the Content Blocking allow list (bug 1557872 fixed in Gecko 69 Beta). What do GV (and A-C?) need to do to give Fenix access to read and write allow list entries?

Comment 11

[Eugen Sawin [:esawin]]

(In reply to Chenxia Liu [:liuche] from comment #9)

We're looking at putting this into the next sprint or two on Fenix - I haven't seen any status updates on this bug, so let me know what work remains to be done and what version of GV we could expect this to be in!

There has been a concern with the initial API design since it breaks with GV's philosophy of not managing state for the app, in this case the exception origins.

There are two API proposals now, both with their advantages and drawbacks. Let's move the discussion to the proposal doc.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Chenxia Liu [:liuche] from comment #9)

We're looking at putting this into the next sprint or two on Fenix - I haven't seen any status updates on this bug, so let me know what work remains to be done and what version of GV we could expect this to be in!

Is this urgent? Note that on top of that GV API we need AC work to expose those new APIs (once we know what they are going to be):
https://github.com/mozilla-mobile/android-components/issues/3264

Comment 13

[(no longer active)]

FWIW if we need to change ContentBlockingAllowList.jsm in a way to make it more useful for GeckoView needs, please file the needed bugs and needinfo me. After the refactoring done in bug 1557872 it should be quite easy to support other requirements at the API boundary. The current boundary is a simple abstraction of what desktop Firefox needed, I decided to not guess what GV needed in advance. :-)

Comment 14

[Johann Hofmann [:johannh]]

(In reply to Eugen Sawin [:esawin] from comment #11)

(In reply to Chenxia Liu [:liuche] from comment #9)

We're looking at putting this into the next sprint or two on Fenix - I haven't seen any status updates on this bug, so let me know what work remains to be done and what version of GV we could expect this to be in!

There has been a concern with the initial API design since it breaks with GV's philosophy of not managing state for the app, in this case the exception origins.

There are two API proposals now, both with their advantages and drawbacks. Let's move the discussion to the proposal doc.

Sorry for further hijacking this bug on a more general discussion, but GV's philosophy of not managing state for the app, in this case the exception origins feels a little weird to me at first sight. A lot of app state is managed by Gecko. Web permissions, certificate exceptions, push notifications, basic auth, and arguably also things like cookies/quota storage, cache and the ton of other persistent non-user triggered state we store in Gecko.

Moving these out of the platform into client code sounds like a bunch of work and I'm assuming that it's not part of your MVP work (??), so why get hung up with content blocking exceptions? There's probably a good explanation of the rationale that I don't understand yet.

Again, happy to move that discussion elsewhere, sorry :)

Comment 15

[Dylan Roeh (:droeh) (he/him)]

Attachment: Bug 1557009 - Add ContentBlockingController to GeckoSession to allow managing exceptions list for content blocking. r=snorp!,ehsan!,#geckoview-reviewers!

(First draft -- still working on tests etc, will add additional reviewers for final patch.)

Comment 16

[Arturo Mejia [:amejia]]

Great, I will start look at it, and working locally!

Comment 17

[Dylan Roeh (:droeh) (he/him)]

Attachment: Bug 1557009 - Add some basic tests for ContentBlockingController. r=#geckoview-reviewers!

Depends on D43758

Comment 18

[Chris Peterson [:cpeterson]]

The Fenix team would like to finish Tracking Protection in their next Fenix sprint (September 16-27) using GV 70 Beta. In that case, we would need to uplift Tracking Protection fixes to GV 70 Beta.

Moving this bug to GV's September sprint ([geckoview:m1909]) since Dylan is working on it.

Comment 19

[Chris Peterson [:cpeterson]]

(In reply to Chris Peterson [:cpeterson] from comment #18)

Moving this bug to GV's September sprint ([geckoview:m1909]) since Dylan is working on it.

Setting priority P1 since we're working on this bug for September.

Reassigning to Dylan since he's working on the bug while Eugen is out.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dce44ccda93f
Add ContentBlockingController to GeckoSession to allow managing exceptions list for content blocking. r=snorp,geckoview-reviewers,Ehsan,agi

Comment 21

[Arthur Iakab [arthur_iakab]]

Backed out changeset dce44ccda93f (bug 1557009) for causing android lints failure on ContentBlockingController.java CLOSED TREE

Backout revision https://hg.mozilla.org/integration/autoland/rev/01c8b743537fa3b051a4ff77acffa1cd929e76f6

Failed push https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=265353779&resultStatus=testfailed%2Cbusted%2Cexception&revision=dce44ccda93fe01626d019ee0eb3db935dc50d8c&searchStr=linting%2Copt%2Candroid%2Cgradle%2Ctests%2Csource-test-mozlint-android-lints%2Ca%28lints%29

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=265353779&repo=autoland

Dylan can you please take a look?

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc7ac1d69679
Add ContentBlockingController to GeckoSession to allow managing exceptions list for content blocking. r=snorp,geckoview-reviewers,Ehsan,agi

Comment 23

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/dc7ac1d69679

Comment 24

[Chris Peterson [:cpeterson]]

Dylan, do we need to uplift the tracking protection exceptions to GV 70 Beta for Fenix? Or can Fenix wait for it to ride to Beta with GV 71 Beta (October 21)?

Comment 25

[Dylan Roeh (:droeh) (he/him)]

(In reply to Chris Peterson [:cpeterson] from comment #24)

Dylan, do we need to uplift the tracking protection exceptions to GV 70 Beta for Fenix? Or can Fenix wait for it to ride to Beta with GV 71 Beta (October 21)?

We should uplift to 70, thanks for the reminder!

Comment 26

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 9088711 [details]
Bug 1557009 - Add ContentBlockingController to GeckoSession to allow managing exceptions list for content blocking. r=snorp!,ehsan!,#geckoview-reviewers!

Beta/Release Uplift Approval Request

• User impact if declined: Fenix will not be able to update to new content blocking exception API as planned.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively low risk as it does not change any existing API, just adds some new functionality.
• String changes made/needed:

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee1132fdac57
Add some basic tests for ContentBlockingController. r=geckoview-reviewers,snorp

Comment 28

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/ee1132fdac57

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9088711 [details]
Bug 1557009 - Add ContentBlockingController to GeckoSession to allow managing exceptions list for content blocking. r=snorp!,ehsan!,#geckoview-reviewers!

Mostly geckoview-only changes, OK for uplift for beta 6.

Comment 30

[Cristian Brindusan [:cbrindusan]]

Dylan, can you please provide a patch for beta which applies cleanly? tThank you.

Comment 31

[Bogdan Tara[:bogdan_tara | bogdant]]

tried to uplift both patches dc7ac1d69679+ee1132fdac57
grafting 565034:dc7ac1d69679 "Bug 1557009 - Add ContentBlockingController to GeckoSession to allow managing exceptions list for content blocking. r=snorp,geckoview-reviewers,Ehsan,agi"
merging mobile/android/components/geckoview/GeckoViewStartup.js
merging mobile/android/geckoview/api.txt
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md
merging mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java
merging mobile/android/geckoview_example/src/main/res/menu/actions.xml
merging mobile/android/geckoview_example/src/main/res/values/strings.xml
merging mobile/android/modules/geckoview/moz.build
merging toolkit/components/antitracking/ContentBlockingAllowList.jsm
warning: conflicts while merging mobile/android/components/geckoview/GeckoViewStartup.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md! (edit, then use 'hg resolve --mark')

Comment 32

[Dylan Roeh (:droeh) (he/him)]

(In reply to Bogdan Tara[:bogdan_tara] from comment #31)

tried to uplift both patches dc7ac1d69679+ee1132fdac57
grafting 565034:dc7ac1d69679 "Bug 1557009 - Add ContentBlockingController to GeckoSession to allow managing exceptions list for content blocking. r=snorp,geckoview-reviewers,Ehsan,agi"
merging mobile/android/components/geckoview/GeckoViewStartup.js
merging mobile/android/geckoview/api.txt
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md
merging mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java
merging mobile/android/geckoview_example/src/main/res/menu/actions.xml
merging mobile/android/geckoview_example/src/main/res/values/strings.xml
merging mobile/android/modules/geckoview/moz.build
merging toolkit/components/antitracking/ContentBlockingAllowList.jsm
warning: conflicts while merging mobile/android/components/geckoview/GeckoViewStartup.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md! (edit, then use 'hg resolve --mark')

Sorry, thought I'd replied here already. I'll take care of the uplift.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6c496f7af7076b770955b70548ce35944192bfdd