Closed Bug 1557009 Opened 1 year ago Closed 5 months ago

GeckoView: Allow setting tracking protection exceptions

Categories

(GeckoView :: General, enhancement, P1)

Unspecified
Android
enhancement

Tracking

(firefox68 wontfix, firefox69 wontfix, firefox70 fixed, firefox71 fixed)

RESOLVED FIXED
mozilla71
Tracking Status
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed
firefox71 --- fixed

People

(Reporter: sebastian, Assigned: droeh)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [geckoview:m1909])

Attachments

(2 files)

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.

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

Flags: needinfo?(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?

Flags: needinfo?(esawin) → needinfo?(ehsan)
Assignee: nobody → esawin
Type: defect → enhancement

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.

(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?

Flags: needinfo?(ehsan)

(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.

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!

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.

Depends on: 1557872

(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.

OS: All → Android
Priority: -- → P2
Whiteboard: [geckoview:fenix:p2]

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!

Flags: needinfo?(esawin)
Flags: needinfo?(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?

Flags: needinfo?(cpeterson)
Whiteboard: [geckoview:fenix:p2] → [geckoview:fenix:m8]

(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.

Flags: needinfo?(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!

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

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. :-)

(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 :)

See Also: → 1566836

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

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

Attachment #9088711 - Attachment description: Bug 1557009 - Add ContentBlockingController to GeckoSession to allow managing exceptions list for content blocking. r=snorp! → Bug 1557009 - Add ContentBlockingController to GeckoSession to allow managing exceptions list for content blocking. r=snorp!,ehsan!,#geckoview-reviewers!
Attachment #9090038 - Attachment description: Bug 1557009 - Add some basic tests for ContentBlockingController. r=snorp! → Bug 1557009 - Add some basic tests for ContentBlockingController. r=#geckoview-reviewers!

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.

Whiteboard: [geckoview:fenix:m8] → [geckoview:m1909]

(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.

Assignee: esawin → droeh
Priority: P2 → P1
Pushed by droeh@mozilla.com:
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
Pushed by droeh@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

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)?

(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!

Flags: needinfo?(droeh)

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:
Attachment #9088711 - Flags: approval-mozilla-beta?
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee1132fdac57
Add some basic tests for ContentBlockingController. r=geckoview-reviewers,snorp

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.

Attachment #9088711 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Flags: needinfo?(droeh)

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')

(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.

Flags: needinfo?(droeh)
You need to log in before you can comment on or make changes to this bug.