1567268 - Support the complete range of block event types in ContentBlocking.onContentBlocked

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P2)

Description

[Eugen Sawin [:esawin]]

The current GV ContentBlocking.onContentBlocked implementation predates Gecko's nsIWebProgressListener.onContentBlockingEvent API and depends on the barely supported nsIClassifiedChannel.matchedList which doesn't give us the full range of today's content blocking categories.

We should refactor the GV implementation to use nsIWebProgressListener.onContentBlockingEvent which would enable the API to report a wider range of block event types, including cookie-based events as triggered by some cookie behavior settings (ETP).

For the refactoring, we will have to decouple BlockEvent.categories from the Category constants, since those only range from anti-tracking to safe browsing. The refactoring should allow for BlockEvent to report the complete range of content blocking events, including cookies.

Comment 1

[Eugen Sawin [:esawin]]

The issue with nsIWebProgressListener.STATE_BLOCKED_TRACKING_CONTENT is that it does not allow us to differentiate between ad, analytic, social and content trackers. We will either have to split up STATE_BLOCKED_TRACKING_CONTENT or continue mapping the GV categories based on nsIClassifiedChannel.matchedList.

Ehsan, are there any plans to provide more detailed (list-based) flags in nsIWebProgressListener.onContentBlockingEvent? Otherwise, GV will need to continue doing the mapping itself based the matched lists, which will not work at all for bug 1567611.

Alternatively, we could stop reporting at such detail, but that would the break the symmetry between the settings and the reporting, i.e., you can configure GV to block specific categories of trackers (ad, analytic, social, content) but the block reports would not give you this specific category information.

Comment 2

[Eugen Sawin [:esawin]]

Attachment: Bug 1567268 - [1.0] Extend ContentBlocking API to support the complete range of block event types.

Comment 3

[Eugen Sawin [:esawin]]

Attachment: Bug 1567268 - [2.0] Update GVE to report the extended content blocking events.

Depends on D39018

Comment 4

[Eugen Sawin [:esawin]]

Attachment: Bug 1567268 - [1.1] Extend ContentBlocking API to support the complete range of block event types.

Comment 5

[Eugen Sawin [:esawin]]

Attachment: Bug 1567268 - [2.0] Update GVE to report the extended content blocking events.

Depends on D39030

Comment 6

[(no longer active)]

There is a lot here and I'm a bit lost as to what this bug is aiming to do, so here is some info in the hopes of reaching a consensus on what needs to be done here.

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

The current GV ContentBlocking.onContentBlocked implementation predates Gecko's nsIWebProgressListener.onContentBlockingEvent API

FWIW onContentBlockingEvent is just a refactoring of the old onSecurityChange API which used to be used for both dispatching security change notifications and content blocking notifications. Nothing too relevant changed during this refactor, so the above (onContentBlocked "predating" onContentBlockingEvent isn't super interesting...).

and depends on the barely supported nsIClassifiedChannel.matchedList which doesn't give us the full range of today's content blocking categories.

Um, why do you think that? nsIClassifiedChannel.matchedList is as supported as it ever was before, it gives you the matched safebrowsing table that was used to block an element. It is currently supported for the following content blocking back-ends:

• Tracking protection
• Safe browsing
• Fingerprinting blocking
• Cryptocurrency mining blocking
• Social tracking protection

Note that this is only supported for back-ends which block the loading of resources.

We could in theory provide this data for more backends, e.g. our "channel annotation" backends (for example the one used for cookie blocking/ETP) where we don't block the loading of resources if that is useful. (GeckoView is the only consumer of this data so this data so far is only available where GV has asked for it or where it has been cargo-cult copied...)

We should refactor the GV implementation to use nsIWebProgressListener.onContentBlockingEvent which would enable the API to report a wider range of block event types, including cookie-based events as triggered by some cookie behavior settings (ETP).

Well, why do these two APIs need to be tied together? Currently GV manages to use STATE_BLOCKED_TRACKING_CONTENT together with the matchedList API without any issues, so why is using this API together with another content blocking code like STATE_COOKIES_BLOCKED_TRACKER an issue?

For the refactoring, we will have to decouple BlockEvent.categories from the Category constants, since those only range from anti-tracking to safe browsing. The refactoring should allow for BlockEvent to report the complete range of content blocking events, including cookies.

Not sure if I understand this part...

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

The issue with nsIWebProgressListener.STATE_BLOCKED_TRACKING_CONTENT is that it does not allow us to differentiate between ad, analytic, social and content trackers. We will either have to split up STATE_BLOCKED_TRACKING_CONTENT or continue mapping the GV categories based on nsIClassifiedChannel.matchedList.

Is there a problem with the second option?

The first option is in theory doable, however it's not really a preferable choice because:

a) Most code wants to treat a type of blocking in a special way, not blocking of specific categories. As a case in point, currently the entire code in Firefox/Gecko wants to treat types and not categories in a special way. So breaking up the content blocking notification types per category will make it more difficult to write code that handles these notifications, since instead of e.g. handling one case for cookie blocking, you'd need to handle e.g. 4 cases.
b) The impact of changing our categorization would ripple through the entire code base, since the categorization would be hardcoded into content blocking notification codes we use in various parts of the code base. For example with the recent rapid changes we have been making to the social tracking protection categorization, each one of those changes would have turned into big and hard to review/get right patches.
c) It is quite a lot of work to do the refactoring suggested. Searching for onContentBlockingEvent in the code shows that this will be a lot of code changes...

Ehsan, are there any plans to provide more detailed (list-based) flags in nsIWebProgressListener.onContentBlockingEvent? Otherwise, GV will need to continue doing the mapping itself based the matched lists, which will not work at all for bug 1567611.

See above, I'd really like to avoid expanding the content blocking notifications this way.

If that problem is what we're trying to solve here, why not just provide the matchedList data there?

Alternatively, we could stop reporting at such detail, but that would the break the symmetry between the settings and the reporting, i.e., you can configure GV to block specific categories of trackers (ad, analytic, social, content) but the block reports would not give you this specific category information.

I think we can have our cake and eat it too. :-)

Comment 7

[(no longer active)]

So bug 1545033 added nsIClassifiedChannel.matchedTrackingLists which is similar to matchedList except that it matches all of the lists that a channel matched against when being classified (e.g. when being classified for cookie blocking/ETP).

Can you please check to see if this API is sufficient for your needs alongside with matchedList? Thanks!

Comment 8

[Eugen Sawin [:esawin]]

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

and depends on the barely supported nsIClassifiedChannel.matchedList which doesn't give us the full range of today's content blocking categories.

Um, why do you think that? nsIClassifiedChannel.matchedList is as supported as it ever was before, it gives you the matched safebrowsing table that was used to block an element. It is currently supported for the following content blocking back-ends:

• Tracking protection
• Safe browsing
• Fingerprinting blocking
• Cryptocurrency mining blocking
• Social tracking protection

Note that this is only supported for back-ends which block the loading of resources.

That's just inaccurately worded from my side. There have been bugs that affected GV specifically around matchedList before, but we haven't had any issue for a long time. I'm happy to hear that matchedList will continue to be supported.

On the other hand, it is only useful for blocked elements, not for potential blocks (e.g., STATE_LOADED_TRACKING_CONTENT) for bug 1567611.

We should refactor the GV implementation to use nsIWebProgressListener.onContentBlockingEvent which would enable the API to report a wider range of block event types, including cookie-based events as triggered by some cookie behavior settings (ETP).

Well, why do these two APIs need to be tied together? Currently GV manages to use STATE_BLOCKED_TRACKING_CONTENT together with the matchedList API without any issues, so why is using this API together with another content blocking code like STATE_COOKIES_BLOCKED_TRACKER an issue?

Combining STATE_* and matchedList works for us, but makes block reporting more complicated than it needs to be. This is definitely not a high priority item, just a nice-to-have.

For the refactoring, we will have to decouple BlockEvent.categories from the Category constants, since those only range from anti-tracking to safe browsing. The refactoring should allow for BlockEvent to report the complete range of content blocking events, including cookies.

Not sure if I understand this part...

GV API specifics, feel free to ignore this comment.

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

The issue with nsIWebProgressListener.STATE_BLOCKED_TRACKING_CONTENT is that it does not allow us to differentiate between ad, analytic, social and content trackers. We will either have to split up STATE_BLOCKED_TRACKING_CONTENT or continue mapping the GV categories based on nsIClassifiedChannel.matchedList.

Is there a problem with the second option?

It's not an actual problem, but would make for a cleaner mapping between Gecko and the GV API.

If that problem is what we're trying to solve here, why not just provide the matchedList data there?

Knowing that GV getting category info out of matchedList is not considered a "barely supported" hack, I think we're OK with continuing using matchedList data for GV block type mapping.

I think we can have our cake and eat it too. :-)

And we can share it with our users! :)

Comment 9

[Eugen Sawin [:esawin]]

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

So bug 1545033 added nsIClassifiedChannel.matchedTrackingLists which is similar to matchedList except that it matches all of the lists that a channel matched against when being classified (e.g. when being classified for cookie blocking/ETP).

Can you please check to see if this API is sufficient for your needs alongside with matchedList? Thanks!

That API seems to fill in the missing link for bug 1567611. I think we are actually good-to-go, thanks a lot!

Comment 10

[Chris Peterson [:cpeterson]]

Assigning to Dylan because he is picking up ETP work while Eugen is out. Fenix needs this API for their ETP UI.

Comment 11

[Chris Peterson [:cpeterson]]

I'm editing a bunch of GeckoView bugs. If you'd like to filter all this bugmail, search and destroy emails containing this UUID:

e88a5094-0fc0-4b7c-b7c5-aef00a11dbc9

Comment 12

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

Attachment: Bug 1567268 - [1.0] Report all ETP-related blocking and non-blocking. r=snorp!,#geckoview-reviewers!

Comment 13

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

Attachment: Bug 1567268 - [2.0] Update GVE and tests to reflect better ETP reporting. r=#geckoview-reviewers!

Depends on D40694

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed05c5d39154
[1.0] Report all ETP-related blocking and non-blocking. r=geckoview-reviewers,snorp,rbarker
https://hg.mozilla.org/integration/autoland/rev/5dea65c19c50
[2.0] Update GVE and tests to reflect better ETP reporting. r=geckoview-reviewers,agi

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ed05c5d39154
https://hg.mozilla.org/mozilla-central/rev/5dea65c19c50

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Is this something we need to consider uplifting for GV69 or can this ride with 70?

Comment 17

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)

Is this something we need to consider uplifting for GV69 or can this ride with 70?

Bouncing the NI to Chris Peterson, as he's probably got a better view of Fenix's requirements than I do.

Chris: We could probably uplift this if necessary, but be aware that other tracking protection related patches (STP support, exception list) will likely be very difficult to uplift (will probably require other teams to uplift their patches, etc...). If it's an all-or-nothing package I would strongly suggest letting this ride with 70.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Yes, if it's feasible and you think we can take the risk then please uplift to 69. We have AC and Fenix functionality waiting for this. :)

Comment 19

[Emily Kager [:ekager]]

Could we please try to uplift this one? We are okay with STP and exceptions management not being uplifted, but could ship initial ETP features with just this bug. Thanks!

Comment 20

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

Comment on attachment 9083112 [details]
Bug 1567268 - [1.0] Report all ETP-related blocking and non-blocking. r=snorp!,#geckoview-reviewers!

Beta/Release Uplift Approval Request

• User impact if declined: Fenix will need to wait for GV 70 for better ETP reporting.
• 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): Changes only affect GV. Additionally, underlying behavior is not different, just how it is reported.
• String changes made/needed:

Comment 21

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

Unflagging Chris.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9083112 [details]
Bug 1567268 - [1.0] Report all ETP-related blocking and non-blocking. r=snorp!,#geckoview-reviewers!

GeckoView-only ETP reporting patch wanted by the mobile team. Approved for GV69.

Comment 23

[Bogdan Tara[:bogdan_tara | bogdant]]

Tried to uplift to beta, but got conflicts:
grafting 559502:ed05c5d39154 "Bug 1567268 - [1.0] Report all ETP-related blocking and non-blocking. r=geckoview-reviewers,snorp,rbarker"
merging mobile/android/chrome/geckoview/geckoview.js
merging mobile/android/geckoview/api.txt
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/ContentBlocking.java
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md
warning: conflicts while merging mobile/android/geckoview/api.txt! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md!

Comment 24

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

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

Tried to uplift to beta, but got conflicts:
grafting 559502:ed05c5d39154 "Bug 1567268 - [1.0] Report all ETP-related blocking and non-blocking. r=geckoview-reviewers,snorp,rbarker"
merging mobile/android/chrome/geckoview/geckoview.js
merging mobile/android/geckoview/api.txt
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/ContentBlocking.java
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md
warning: conflicts while merging mobile/android/geckoview/api.txt! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md!

Thanks, I'll handle the uplift.

Comment 25

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

(In reply to Dylan Roeh (:droeh) (he/him) from comment #24)

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

Tried to uplift to beta, but got conflicts:
grafting 559502:ed05c5d39154 "Bug 1567268 - [1.0] Report all ETP-related blocking and non-blocking. r=geckoview-reviewers,snorp,rbarker"
merging mobile/android/chrome/geckoview/geckoview.js
merging mobile/android/geckoview/api.txt
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/ContentBlocking.java
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md
warning: conflicts while merging mobile/android/geckoview/api.txt! (edit, then use 'hg resolve --mark')
warning: conflicts while merging mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md!

Thanks, I'll handle the uplift.

I'm holding off on the uplift; it looks like Fenix's 9.2 sprint will be using GV 70 beta per email discussion, and uplifting probably won't be necessary here.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9083112 [details]
Bug 1567268 - [1.0] Report all ETP-related blocking and non-blocking. r=snorp!,#geckoview-reviewers!

Confirmed with Dylan that we don't need this for 69 after all.