1258041 - Update the tracking protection list selection UI to account for the DNT-related changes

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[François Marier [:francois]]

The tracking protection lists will change in order to differentiate between trackers that honor DNT and those that don't.

We'll need to ensure that the "standard" and "strict" lists still work the same way after that change.

Comment 1

[François Marier [:francois]]

Currently, we set the prefs like this:

- basic: urlclassifier.trackingTable = "test-track-simple,mozstd-track-digest256"
- strict: urlclassifier.trackingTable = "test-track-simple,mozfull-track-digest256"

to use the new DNT-aware lists, we'll need this instead:

- basic: urlclassifier.trackingTable = "test-track-simple,base-track-digest256"
- strict: urlclassifier.trackingTable = "test-track-simple,base-track-digest256,content-track-digest256"

Of course, instead of forcefully setting these strings, it would be better to split on commas and add/remove the content-track-digest256 list.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

What is supposed to happen to urlclassifier.trackingWhitelistTable? I assume that I should change the default values (in all.js) of both urlclassifier.trackingWhitelistTable and urlclassifier.trackingTable in this patch, right?

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to François Marier [:francois] from comment #1)
> - basic: urlclassifier.trackingTable =
> "test-track-simple,base-track-digest256"
> - strict: urlclassifier.trackingTable =
> "test-track-simple,base-track-digest256,content-track-digest256"
> 
> Of course, instead of forcefully setting these strings, it would be better
> to split on commas and add/remove the content-track-digest256 list.

Currently the frontend code does not hardcode any list names (e.g. mozstd), but in order to implement this new scheme it has to treat something as special, like maybe the "base" name, or the first name in browser.safebrowsing.provider.mozilla.lists. Since the existing flexibility didn't help us much so far, I am leaning towards the first option in order to simplify things. Thoughts?

Comment 4

[François Marier [:francois]]

(In reply to Panos Astithas [:past] from comment #2)
> What is supposed to happen to urlclassifier.trackingWhitelistTable? I assume
> that I should change the default values (in all.js) of both
> urlclassifier.trackingWhitelistTable and urlclassifier.trackingTable in this
> patch, right?

The trackingWhitelistTable pref stays the same in both cases and we're not renaming it as part of these changes.

Comment 5

[François Marier [:francois]]

(In reply to Panos Astithas [:past] from comment #3)
> Currently the frontend code does not hardcode any list names (e.g. mozstd),
> but in order to implement this new scheme it has to treat something as
> special, like maybe the "base" name, or the first name in
> browser.safebrowsing.provider.mozilla.lists. Since the existing flexibility
> didn't help us much so far, I am leaning towards the first option in order
> to simplify things. Thoughts?

You could perhaps treat all of the "*-track-*" lists as special since that part of the listname is fixed and is restricted to tracking protection lists.

Maybe it wasn't immediately obvious in my first comment, but the change I should have called out explicitly is that lists are additive and no longer "either/or".

So if you want a more flexible UI, it would need to be possible to check all of the lists you want (e.g. base + content), i.e. checkboxes not radio buttons.

Another reasonable approach if you want to get this done quicker IMHO would also be to just hardcode the prefs that "strict" and "basic" map to.

Comment 6

[François Marier [:francois]]

Also maybe we should roll bug 1274671 into this one since it's pretty much just updating the default values of the prefs that this one is toggling. They are both blocked on the lists being available on the server.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

Checkboxes instead of radio buttons make sense for the new lists, but that would require string changes for the list names, so we want to punt on that for the time being, right? If so, I think I'll just go with the simplest solution I can think of and we can rework it once it's time for the checkboxes work.

(In reply to François Marier [:francois] from comment #6)
> Also maybe we should roll bug 1274671 into this one since it's pretty much
> just updating the default values of the prefs that this one is toggling.
> They are both blocked on the lists being available on the server.

Indeed, I have already changed these in my WIP.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: MozReview Request: Bug 1258041 - Update the tracking protection list selection UI to account for the DNT-related changes. r?jaws r?francois

Review commit: https://reviewboard.mozilla.org/r/54892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54892/

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Parking my (completely untested) WIP for the day.

Comment 10

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

Tracking for 49 since a new feature aimed at 49 needs it.

Comment 11

[François Marier [:francois]]

https://reviewboard.mozilla.org/r/54892/#review51546

Instead of overwriting the urlclassifier.trackingTable pref entirely, why not replace `BASE_LIST + TRACK_SUFFIX` with `BASE_LIST + TRACK_SUFFIX + "," + CONTENT_LIST + TRACK_SUFFIX` ?

That way, if the default value of that pref changes in a non-user visible way (for example to add another test list) then the UI code doesn't need to be updated.

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

_getActiveList() already has to remove the test list because it needs to get to the active list. In the strict list case, removing the base list will result in the test list and the content list. If we don't hardcode the name of the test list, we will have to hardcode the name of the content list to make this work.

Comment 14

[François Marier [:francois]]

(In reply to Panos Astithas [:past] from comment #13)
> _getActiveList() already has to remove the test list because it needs to get
> to the active list. In the strict list case, removing the base list will
> result in the test list and the content list. If we don't hardcode the name
> of the test list, we will have to hardcode the name of the content list to
> make this work.

Hardcoding the name of the content and base lists sounds like a better option to me since we're not going to change that (for backward-compatibility reasons), but we could add new test lists at any time.

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8755936 [details]
MozReview Request: Bug 1258041 - Update the tracking protection list selection UI to account for the DNT-related changes. r?jaws r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54892/diff/1-2/

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

I've modified the patch as agreed. Tested that the list name change works as expected, but this still needs the server prod changes to be in place before landing.

Comment 17

[François Marier [:francois]]

Comment on attachment 8755936 [details]
MozReview Request: Bug 1258041 - Update the tracking protection list selection UI to account for the DNT-related changes. r?jaws r?francois

https://reviewboard.mozilla.org/r/54892/#review52556

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8755936 [details]
MozReview Request: Bug 1258041 - Update the tracking protection list selection UI to account for the DNT-related changes. r?jaws r?francois

https://reviewboard.mozilla.org/r/54892/#review53370

::: browser/components/preferences/blocklists.js:138
(Diff revision 2)
> +            trackingTable = trackingTable.replace("," + CONTENT_LIST_ID + TRACK_SUFFIX, "");
> +          } else {
> +            trackingTable += "," + CONTENT_LIST_ID + TRACK_SUFFIX;

If the content list is not selected, then we remove the content list from the pref value. But if it selected, then we add it to the pref.

What if in the case that the content list is selected, the value was already stored in the pref? Is that possible?

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

https://reviewboard.mozilla.org/r/54892/#review53370

> If the content list is not selected, then we remove the content list from the pref value. But if it selected, then we add it to the pref.
> 
> What if in the case that the content list is selected, the value was already stored in the pref? Is that possible?

If the content value was in the pref, then the dialog would have come up with the strict list checkbox ticked and the user wouldn't be able to make execution follow the path that adds the value to the pref again. So that case isn't possible.

It isn't even a problem if the user opens the dialog, makes a change, then goes to about:config to mess with the pref again and later gets back to the dialog to save the changes there, because we check for changes on submit before persisting. It is possible to fool the code by putting garbage in the pref manually, but that's not something that we care to guard against in many other places and it would also break the platform code, so I don't thik it matters here.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8755936 [details]
MozReview Request: Bug 1258041 - Update the tracking protection list selection UI to account for the DNT-related changes. r?jaws r?francois

https://reviewboard.mozilla.org/r/54892/#review53610

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

This patch is now waiting for the server-side list changes to make it into production.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ba7134dbdaa

Comment 23

[François Marier [:francois]]

(In reply to Panos Astithas [:past] from comment #21)
> This patch is now waiting for the server-side list changes to make it into
> production.

It also needs to wait for bug 1285428 to land since Panos' patch will trigger this bug.

Similarly, no uplifting unless that fix is uplifted first.

Comment 24

[Erin Lancaster [:elan]]

Panos, Shavar is on prod. Francois confirmed you are now unblocked thanks to bug 1289518.

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

Confirmed, I'm going to land the patch shortly.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/fff9d624dca2
Update the tracking protection list selection UI to account for the DNT-related changes. r=jaws r=francois

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 28

[Gerry Chang [:gchang]]

Hi :past,
Since this patch also affects 49/50, are you also considering to uplift this patch to 49/50?

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

The plan is to uplift to 50 once bug 1292728 is fixed.

Comment 30

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8755936 [details]
MozReview Request: Bug 1258041 - Update the tracking protection list selection UI to account for the DNT-related changes. r?jaws r?francois

Approval Request Comment
[Feature/regressing bug #]: this is an updated feature that we want to ship in 50, where the rest of the platform changes have already landed.
[User impact if declined]: the DNT loophole problem will remain unfixed
[Describe test coverage new/current, TreeHerder]: manual testing in Nightly
[Risks and why]: small risk, the meat of the change is contained in the preferences subsystem, which is not visited very often
[String/UUID change made/needed]: none

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

Bug 1292728 should be uplifted too, but it doesn't need approval as it is a test-only fix.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Panos, was this planned all along for Fx50? Did we miss the merge date as I think changes of this nature are best stabilized rather than jumping trains. Is this something we can ship with 51 so it gets tested and stabilized some more? Please let me know.

Comment 33

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Ritu Kothari (:ritu) from comment #32)
> Hello Panos, was this planned all along for Fx50? Did we miss the merge date
> as I think changes of this nature are best stabilized rather than jumping
> trains. Is this something we can ship with 51 so it gets tested and
> stabilized some more? Please let me know.

Yes, I believe this was planned for 49, but we missed the train for that one. Erin can confirm.

Comment 34

[Erin Lancaster [:elan]]

Confirmed. We are not targeting 50 as there just isn't enough time to test. We have consensus about uplifting to 51 and having that be our release vehicle, instead.

Comment 35

[Gerry Chang [:gchang]]

As Erin mentioned in commment #34, let it ride the train on 51 to get the extra bake time. Mark won't fix for 49/50.

Comment 36

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Gerry Chang [:gchang] from comment #35)
> As Erin mentioned in commment #34, let it ride the train on 51 to get the
> extra bake time. Mark won't fix for 49/50.

I think there is a misunderstanding here. Erin wrote that "we are not targeting 50 as there just isn't enough time to test. We have consensus about uplifting to 51", which is obviously a mistake as 51 is the current nightly and an uplift is not necessary. I believe she meant to say that "we are not targeting *49*" (as I wrote in my comment 33 to which she was responding), but "we have consensus about uplifting to *50*". Erin, can you clarify if we want to uplift this to aurora (50) or just let it ride the trains in 51?

Comment 37

[Erin Lancaster [:elan]]

I' sorry, Panos. You are correct. We need to uplift to *50* Not ride the trains.

Comment 38

[Panos Astithas (he/him) [:past] (please ni?)]

Thanks Erin. I don't seem to be able to request for approval again, could you please reconsider based on comment 37?

Comment 39

[Gerry Chang [:gchang]]

Comment on attachment 8755936 [details]
MozReview Request: Bug 1258041 - Update the tracking protection list selection UI to account for the DNT-related changes. r?jaws r?francois

We want to ship the updated feature in 50. Let it ride the train on 50.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5203cfbd484a

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We got this patch backported to aurora without pushing the fix for the firefox-ui-tests which are now perma-orange again. Can we get this done sooner than later please?

Comment 42

[Henrik Skupin [:whimboo][⌚️UTC+2]]

To add this is bug 1292728.

Comment 43

[Erin Lancaster [:elan]]

Matt, can you verify this is fixed and working as expected? As you can see it's been uplifted to 50. Many thanks!

Comment 44

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed fixed in latest Fx50 and Fx51. The preference "urlclassifier.trackingTable" updates appropriately when the blocklist is toggled between basic and strict, as per comment 1.