1447226 - shield studies Developer/Nightly UI option(s) vs about:config changes

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Simon Mainey]

Attachment: OhNoes.png

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20100101

Steps to reproduce:

This may be related to bug 1430391

STR (UI)
- In Developer (60, tested) or Nightly (not tested), go to Options>Privacy & Security>Firefox* Data Collection and Use
- There are two checkboxes (labeled 1 and 1b in the attached image). These are collect data (1) and shield studies (1b)
- In the UI, if you check or uncheck send technical data (1), it auto checks or unchecks studies (1b)
- Note: **from the UI**, you can allow data collected and disallow studies, but NOT vice versa

[1 - data] governs datareporting.healthreport.uploadEnabled
[1b - studdies] governs app.shield.optoutstudies.enabled

STR (about:config)
- in about: config set datareporting.healthreport.uploadEnabled=>false and app.shield.optoutstudies.enabled=>true
- look at the UI, it has achieved a state that contradicts itself?

Note: I do not know if this is just cosmetic or affects the sending of shield data


Actual results:

See attachment


Expected results:

When datareporting.healthreport.uploadEnabled=false then app.shield.optoutstudies.enabled needs to also be toggled to false

Comment 1

[Simon Mainey]

Sorry, attachment is missing labels - you can refer to https://github.com/ghacksuserjs/ghacks-user.js/issues/381 for more info if needed

Comment 2

[Carmen Fat [:cfat] - Ecosystem QA]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0

Hello Simon, thanks for reporting this issue.

I have managed to reproduce it on latest Nightly 61.0a1 and Firefox Release 59.0.1 using Mac 10.11.6, Arch Linux and Windows 10 x64.

After setting the "datareporting.healthreport.uploadEnabled" to false and "app.shield.optoutstudies.enabled" to true, I navigated to "about:preferences#privacy" page and observed the UI from the Firefox Data Collection and Use section. Even though the "Allow Firefox to install and run studies" child setting is greyed out, the check box displays the blue checked sign, contradicting the "Allow Firefox to send technical and interaction data to Mozilla" parent setting.

Comment 3

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

Mike, can you take a look at this? It's pretty concerning that our UI can be misleading here about user privacy configurations.

Comment 4

[Michael Cooper [:mythmon]]

Normandy reads both settings. If data reporting is turned off, it won't run studies, regardless of what the studies pref says. Specifically, Normandy has a whole will currently take no action when Telemetry data reporting is disabled, and will only run add-on studies (a subset of it's overall behavior) if the studies pref is *also* enabled.

I agree it is confusing, but I think it is currently safe and privacy-preserving, so it isn't an emergency. I think this is a UX problem. How are these kind of prefs usually handled in about:settings? It would be good to be consistent.

Comment 5

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

The standard approach to these is to uncheck all child/indented checkboxes if the parent checkbox is unchecked. You can see this at the Search Suggestions preference. If the "Provide search suggestions" pref is unchecked, the "Show search suggestions in address bar results" checkbox will get automatically unchecked.

Per a privacy risk, this may not be an emergency. But based on user perception we should fix this ASAP because it gives the perception that there is a privacy leak, which is especially hurtful when there really isn't one :)

Can you put together a quick patch to fix this?

Comment 6

[Michael Cooper [:mythmon]]

Sorry about the slow response. I'm at a work week this week. I've started to look at this, and I don't think a "quick patch" is going to be sufficient. There are a bunch of moving parts and interesting logic, and this bit of UI is dynamically injected by Normandy for historical reasons (see bug 1440156).

Comment 7

[Simon Mainey]

(In reply to Michael Cooper [:mythmon] from comment #4)
> I think this is a UX problem. How are these kind of prefs usually handled in about:settings? It would be good to be consistent.

The UX logic is fine, this is an issue when someone makes changes in about:config. I can think of one example when changes in about:config are picked up and control other prefs

about:preferences#privacy > Address Bar : has three options (browsing, bookmarks, open tabs). These are:
 - `browser.urlbar.suggest.history`, `browser.urlbar.suggest.bookmark`, `browser.urlbar.suggest.openpage`
These three control (and vice versa) a pref called `browser.urlbar.autocomplete.enabled` (not in the UI)

The rules are quite simple:
- If **all 3** of the suggestion prefs are false, *autocomplete is also false
- If **any** of the suggestion prefs are true, autocomplete must also be true

Load up two about:config pages with "browser.urlbar.suggest" in one and "browser.urlbar.autocomplete" in the other. At default all four prefs should be true.
- change all three suggest prefs to false, and then look at the autocomplete pref, it is changed to false
- change autocomplete to true, and voila, the three suggest prefs have been automatically changed to true

So we just need to listen(?) for when "app.shield.optoutstudies.enabled" is set to true in about:config, then datareporting.healthreport.uploadEnabled is also set to true.

Hope the example helps :)

Comment 8

[Michael Cooper [:mythmon]]

Attachment: Bug 1447226 - Don't show shield study opt-in as checked when studies won't run

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

Comment 9

[Michael Cooper [:mythmon]]

Simon, considering that these two prefs are used by disparate parts of Firefox, I don't think that automatically changing either pref is appropriate. I've wrote a patch to make the UI reflect the actual behavior though.

Gijs, the patch here doesn't include tests because the existing code doesn't have tests. I can't figure out how to add tests to this weird preference patching method, and I think we'll get rid of this soon-ish in bug 1440156 anyways.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8964373 [details]
Bug 1447226 - Don't show shield study opt-in as checked when studies won't run

https://reviewboard.mozilla.org/r/233074/#review238794

::: toolkit/components/normandy/lib/ShieldPreferences.jsm:118
(Diff revision 1)
>      // Preference instances for prefs that we need to monitor while the page is open.
>      doc.defaultView.Preferences.add({ id: OPT_OUT_STUDIES_ENABLED_PREF, type: "bool" });
>  
>      // Weirdly, FHR doesn't have a Preference instance on the page, so we create it.
>      const fhrPref = doc.defaultView.Preferences.add({ id: FHR_UPLOAD_ENABLED_PREF, type: "bool" });
>      function onChangeFHRPref() {

Nit: can we rename this to something like `updateStudyCheckboxState()` or whatever?

::: toolkit/components/normandy/lib/ShieldPreferences.jsm:134
(Diff revision 1)
> +      // so showing a checkbox would be confusing.
> +      //
> +      // * MOZ_TELEMETRY_REPORTING is true
> +      // * the policy allows Shield
> +      // * the FHR pref is true
> +      // * Normandy is disabled

This last item doesn't look right. I think you mean *enabled*, not *disabled* ?

::: toolkit/components/normandy/lib/ShieldPreferences.jsm:139
(Diff revision 1)
> +        Services.prefs.getBoolPref(FHR_UPLOAD_ENABLED_PREF) &&
> +        Services.prefs.getBoolPref(NORMANDY_ENABLED_PREF)

Nit: can you add default values of `false` for both of these, per belt/suspenders fashion? :-)

Comment 11

[Michael Cooper [:mythmon]]

Comment on attachment 8964373 [details]
Bug 1447226 - Don't show shield study opt-in as checked when studies won't run

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7f47849e748
Don't show shield study opt-in as checked when studies won't run r=Gijs

Comment 13

[Cosmin Sabou [:CosminS]]

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

Comment 14

[Stefania]

I could reproduce this bug on Win10 x64 with the affected builds [1] .
On the latest Firefox builds [2], the following behaviour is triggered across platforms [3]: after setting the "datareporting.healthreport.uploadEnabled" to false and "app.shield.optoutstudies.enabled" to true
- the "Allow Firefox to install and run studies" becomes grayed out and the check box does NOT display anymore the blue checked sign (meaning that the fix is successfully applied)
- the "Allow Firefox to send technical and interaction data to Mozilla" remains checked until the browser is restarted and this is a potential issue. 

[1] Nightly build 61.0a1 (2018-03-19) and DevEdition 60.0b5 build 1 (20180319175655)
[2] 61.0b10 build 1 (20180530184300) and Devedition 61.0b10 build 1 (20180530184300)
[3] Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12.6

Any thoughts about this, Michael?

Comment 15

[Michael Cooper [:mythmon]]

Hi Stefania, thanks for checking this. I think that the behavior of the "Allow Firefox to send technical and interaction data to Mozilla" is unrelated to this bug. That certainly seems like a problem, but I think it should probably be a new bug since it would be a different part of the code than my previous patch. This bug was specifically about the "Allow Firefox to install and run studies" checkbox.

Comment 16

[Stefania]

Thank you Michael for this info.
Taking into account the previous comment, I will mark Firefox 61 as verified and also file a new bug for the above mentioned issue.