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

VERIFIED FIXED in Firefox 50

Status

()

P3
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: francois, Assigned: past)

Tracking

unspecified
Firefox 51
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox49+ wontfix, firefox50 verified, firefox51 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
(Reporter)

Comment 1

3 years ago
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.
Depends on: 1258038
Whiteboard: [fxprivacy][triage]
Assignee: nobody → past
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [fxprivacy][triage] → [fxprivacy]

Updated

3 years ago
Iteration: --- → 49.3 - Jun 6
Flags: qe-verify?
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?
Flags: needinfo?(francois)
(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?
(Reporter)

Comment 4

3 years ago
(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.
Flags: needinfo?(francois)
(Reporter)

Comment 5

3 years ago
(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.
(Reporter)

Comment 6

3 years ago
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.
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.
Created 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 commit: https://reviewboard.mozilla.org/r/54892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54892/
Parking my (completely untested) WIP for the day.
Tracking for 49 since a new feature aimed at 49 needs it.
status-firefox49: --- → affected
tracking-firefox49: --- → +
(Reporter)

Comment 11

3 years ago
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.
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1274671
_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.
(Reporter)

Comment 14

3 years ago
(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 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/
Attachment #8755936 - Attachment description: MozReview Request: Bug 1258041 - Update the tracking protection list selection UI to account for the DNT-related changes → MozReview Request: Bug 1258041 - Update the tracking protection list selection UI to account for the DNT-related changes. r?jaws r?francois
Attachment #8755936 - Flags: review?(jaws)
Attachment #8755936 - Flags: review?(francois)
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.
(Reporter)

Comment 17

3 years ago
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
Attachment #8755936 - Flags: review?(francois) → review+
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?
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.
Attachment #8755936 - Flags: review?(jaws)
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
Attachment #8755936 - Flags: review?(jaws) → review+
This patch is now waiting for the server-side list changes to make it into production.
Flags: qe-verify? → qe-verify+

Updated

3 years ago
QA Contact: paul.silaghi

Updated

3 years ago
Iteration: 49.3 - Jun 6 → 50.1

Updated

2 years ago
Iteration: 50.1 → 50.2

Updated

2 years ago
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18

Updated

2 years ago
Status: ASSIGNED → NEW
Iteration: 50.3 - Jul 18 → ---
Priority: P1 → P3
(Reporter)

Comment 23

2 years ago
(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.
(Reporter)

Updated

2 years ago
Blocks: 1289177
Panos, Shavar is on prod. Francois confirmed you are now unblocked thanks to bug 1289518.
Flags: needinfo?(past)
Confirmed, I'm going to land the patch shortly.
Flags: needinfo?(past)

Comment 26

2 years ago
Pushed by pastithas@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fff9d624dca2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Depends on: 1292728

Updated

2 years ago
status-firefox50: --- → affected
Flags: needinfo?(past)
Hi :past,
Since this patch also affects 49/50, are you also considering to uplift this patch to 49/50?
The plan is to uplift to 50 once bug 1292728 is fixed.
Flags: needinfo?(past)
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
Attachment #8755936 - Flags: approval-mozilla-aurora?
Bug 1292728 should be uplifted too, but it doesn't need approval as it is a test-only fix.
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.
Flags: needinfo?(past)
(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.
Flags: needinfo?(past) → needinfo?(elancaster)
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.
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.
status-firefox49: affected → wontfix
status-firefox50: affected → wontfix

Updated

2 years ago
Attachment #8755936 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
(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?
Flags: needinfo?(elancaster)
Flags: needinfo?(elancaster)
I' sorry, Panos. You are correct. We need to uplift to *50* Not ride the trains.
Thanks Erin. I don't seem to be able to request for approval again, could you please reconsider based on comment 37?
Flags: needinfo?(elancaster) → needinfo?(gchang)

Updated

2 years ago
status-firefox50: wontfix → affected
Flags: needinfo?(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.
Attachment #8755936 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora+

Comment 40

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/5203cfbd484a
status-firefox50: affected → fixed
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?
Flags: needinfo?(ryanvm)
To add this is bug 1292728.
Flags: needinfo?(ryanvm)
Matt, can you verify this is fixed and working as expected? As you can see it's been uplifted to 50. Many thanks!
Flags: needinfo?(mwobensmith)
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.
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
status-firefox51: fixed → verified
Flags: needinfo?(mwobensmith)
You need to log in before you can comment on or make changes to this bug.