Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TrackingCategory.RECOMMENDED does not block ads as expected #4191

Closed
mcomella opened this issue Aug 22, 2019 · 12 comments

Comments

@mcomella
Copy link
Contributor

commented Aug 22, 2019

I tried applying TrackingCategory.RECOMMENDED in FFTV but ads still appeared:

trackingCategories = arrayOf(TrackingCategory.RECOMMENDED)

(This is how the default values for TrackingProtectionPolicy.select define it)

I switched to adding these categories manually and it worked fine:

trackingCategories = arrayOf(TrackingCategory.AD, TrackingCategory.ANALYTICS, TrackingCategory.SOCIAL)

Looking at the code, these values are being added together:

However, these values look like they're supposed to be bit-packed because other values use bit-shift operators: I think they should be using and instead of +.

@mcomella mcomella self-assigned this Aug 22, 2019

@mcomella

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Note: I tested on rottentomatoes.com which shows a banner ad at the top.

CC @pocmo This seems important because tracking protection may be broken in our apps.

@mcomella mcomella removed their assignment Aug 22, 2019

@mcomella

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

I set it up using and but that doesn't seem to work either: I'll leave this to the professionals.

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

@Amejia481

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

On it!

@Amejia481

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

I tried applying TrackingCategory.RECOMMENDED in FFTV but ads still appeared:

@mcomella I believe tracking protection does not block ads, it blocks scripts/cookies/content from trackers depending on which policy you choose. ( I have to confirm). For blocking ads we need an Adblocker.

Ads not showing maybe a side effect in some cases, as they may depend on the scripts/cookies/content that we are blocking.


If you want to confirm the categories, that you want are getting apply you can test verifying
the session.trackersBlocked property that reports a list of trackers blocked on a site. When a site finishes loading it should contain trackers that belong to the categories that you applied (As long as the site has these trackers categories on it)

If we take https://rottentomatoes.com/ as a guinea pig and apply TrackingProtectionPolicy.recommended() (AD,ANALYTICS and SOCIAL), when the page finishes loading we are going to get reported that:

session.trackersBlocked.trackingCategories
contains TrackingCategory.AD,TrackingCategory.ANALYTICS and TrackingCategory.ANALYTICS.

A tracker may contain multiple categories, you can use the snippet below to not show duplicates:

var categoriesBlocked = mutableSetOf<EngineSession.TrackingProtectionPolicy.TrackingCategory>()

session.trackersBlocked.forEach{
    categoriesBlocked.addAll(it.trackingCategories.toSet())
}

If we use the same site but just applying AD:

TrackingProtectionPolicy.select(arrayOf(TrackingProtectionPolicy.TrackingCategory.AD))

When the page finishes loading session.trackersBlocked.trackingCategories is going to contain
only TrackingProtectionPolicy.TrackingCategory.AD (I apply the snipped above to avoid duplicates)

The same could be seen if we apply just
TrackingProtectionPolicy.select(arrayOf(TrackingProtectionPolicy.TrackingCategory.ANALYTICS))

When the page finishes loading session.trackersBlocked.trackingCategories is going to contain
only TrackingProtectionPolicy.TrackingCategory.ANALYTICS (I apply the snipped above to avoid duplicates)

@pocmo

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

@Amejia481 Did you verify that this is still the case for WebView (browser-engine-system) too?

@Amejia481

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

Checking!

@Amejia481

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

Issue discovered, working on a patch!
Thanks @mcomella for reporting it!

@Amejia481 Amejia481 self-assigned this Aug 23, 2019

@Amejia481 Amejia481 added this to 🏃‍♀️ In Progress in A-C: Android Components Sprint Planning Aug 23, 2019

@Amejia481

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

trackingCategories = arrayOf(TrackingCategory.AD, TrackingCategory.ANALYTICS, TrackingCategory.SOCIAL)

@mcomella As you described above, until the fix lands you have to add the categories manually, sorry for that!

@mcomella

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

I believe tracking protection does not block ads, it blocks scripts/cookies/content from trackers depending on which policy you choose.

Sorry I was not explicit about SystemWebView (FFTV is still on it) and about ^: a side effect of TP is that it blocks ads and the ad I usually use for testing wasn't blocked. I probably should have been clearer. :)

@Amejia481

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2019

I really appreciate you were very diligent double checking after the API refactoring. I'm covering with more tests to avoid similar issues in the future :), Sorry for the inconvenience, that this may cause you!

Amejia481 added a commit to Amejia481/android-components that referenced this issue Aug 24, 2019
Amejia481 added a commit to Amejia481/android-components that referenced this issue Aug 24, 2019

@pocmo pocmo added this to the 10.0.0 🏊 milestone Aug 26, 2019

bors bot pushed a commit that referenced this issue Aug 26, 2019
Merge #4196
4196: Closes #4191: Fixed the recommended() tracking category on SystemEngine r=pocmo a=Amejia481



Co-authored-by: Arturo Mejia <arturomejiamarmol@gmail.com>

@bors bors bot closed this in 4a6912d Aug 26, 2019

A-C: Android Components Sprint Planning automation moved this from 🏃‍♀️ In Progress to 🏁 Done Aug 26, 2019

@mcomella

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

I really appreciate you were very diligent double checking after the API refactoring. I'm covering with more tests to avoid similar issues in the future :)

And thanks for writing those tests. :)

NotWoods added a commit to NotWoods/android-components that referenced this issue Aug 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.