Closed Bug 1039069 Opened 10 years ago Closed 7 years ago

Warn the user that customizing the preferred language list (Accept-Language) can be used for fingerprinting

Categories

(Firefox :: Settings UI, defect, P1)

Product:
Firefox
Component:
Settings UI
Version:
30 Branch
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: ruben, Assigned: cfu)

References

(Blocks 1 open bug)

Details

(Keywords: privacy, Whiteboard: [tor][fingerprinting][fp:m3][ux])

Attachments

(10 files, 5 obsolete files)

Tor_Language_Prompt_Simplified_Chinese.png
595.87 KB, image/png
Details
Firefox_Language_Prompt.png
53.53 KB, image/png
Details
Firefox_Language_Prompt_V1.png
23.16 KB, image/png
Details
Bug 1039069 - Provide a popup about English for international users.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1039069 - Show a hint of the change of the preferred language list.
59 bytes, text/x-review-board-request
mconley
: review+
Details
Screen Shot 2017-11-20 at 3.45.14 PM.png
454.36 KB, image/png
Details
Screen Shot 2017-11-20 at 3.46.07 PM.png
458.35 KB, image/png
Details
Screen Shot 2017-11-20 at 3.46.32 PM.png
463.43 KB, image/png
Details
Bug 1039069 - Gray-out style for disabled listbox.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Screen Shot 2017-11-21 at 4.17.09 PM.png
446.66 KB, image/png
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140616191956

Steps to reproduce:

I modified the list of accepted languages that implement the Accept-Language header.


Actual results:

As a result I made it easier to be uniquely identified, a change that most users won't associate with their act, thus potentially harming their privacy.


Expected results:

I should have been warned about this problem in order to make an informed decision.

I suggest a text is added to the Languages config window informing the user about this issue.

This was originally recommended in the HTTP 1.1. spec:
http://www.ietf.org/rfc/rfc2616.txt
   15.1.4 Privacy Issues Connected to Accept Headers
   User agents which offer the option to configure the contents of an
   Accept-Language header to be sent in every request are strongly
   encouraged to let the configuration process include a message which
   makes the user aware of the loss of privacy involved.

The current version of the spec states:
https://tools.ietf.org/html/rfc7231
   5.3.5. Accept-Language
   It might be contrary to the privacy expectations of the user to send
   an Accept-Language header field with the complete linguistic
   preferences of the user in every request (Section 9.7).
   [...]
   User agents ought to provide guidance to users when setting
   a preference, since users are rarely familiar with the details of
   language matching as described above.
Component: Untriaged → Preferences
Keywords: privacy
Whiteboard: [fingerprinting]
Status: UNCONFIRMED → NEW
QA Whiteboard: [bugday-20140721]
Ever confirmed: true
Summary: Warn the user that customizing the acepted language list can be used for fingerprinting → Warn the user that customizing the preferred language list (Accept-Language) can be used for fingerprinting
Priority: -- → P2
Priority: P2 → P1
Whiteboard: [fingerprinting] → [tor][fingerprinting][fp:m1]
Currently, Tor Browser displays a yes/no prompt in non-English locales, asking users if they want to reveal their locale or not:

"To give you more privacy, Torbutton can request the English language version of web pages. This may cause web pages that you prefer to read in your native language to display in English instead.

Would you like to request English language web pages for better privacy?"

Answering "yes" sets torbutton's "extensions.torbutton.spoof_english" to true.

Then Firefox prefs are set like this:

if (m_tb_prefs.getBoolPref("extensions.torbutton.spoof_english")) {
  m_tb_prefs.setCharPref("intl.accept_languages", "en-US, en");
  m_tb_prefs.setBoolPref("javascript.use_us_english_locale", true);
} else {
  if(m_tb_prefs.prefHasUserValue("intl.accept_languages"))
    m_tb_prefs.clearUserPref("intl.accept_languages");
    m_tb_prefs.setBoolPref("javascript.use_us_english_locale", false);
}
Whiteboard: [tor][fingerprinting][fp:m1] → [tor][fingerprinting][fp:m2]
Hey Arthur,

I downloaded the latest Tor Browser 6.5.2, modified the preferred language to zh-tw in about:preferences#content, and visited https://www.google.com.

I expect to see a prompt, but none was displayed and the page is in traditional Chinese, so my locale is revealed.

Is something wrong?
Flags: needinfo?(arthuredelstein)
It seems that Tor Browser displayed the prompt only if "general.useragent.locale" is not English.

But changing the preferred language in about:preferences affects "intl.accept_langauges" instead.
(In reply to Jonathan Hao [:jhao] from comment #3)
> It seems that Tor Browser displayed the prompt only if
> "general.useragent.locale" is not English.
> 
> But changing the preferred language in about:preferences affects
> "intl.accept_langauges" instead.

Yes, this sounds like the answer. I expect you will see the prompt if you visit this page:
https://www.torproject.org/download/download
and choose a non-English language from the drop-down menu before you click the Download button. Unfortunately we don't have a zh-tw version currently.
Flags: needinfo?(arthuredelstein)
Assignee: nobody → jhao
Status: NEW → ASSIGNED
(In reply to Arthur Edelstein [:arthuredelstein] from comment #5)
> (In reply to Jonathan Hao [:jhao] from comment #3)
> > It seems that Tor Browser displayed the prompt only if
> > "general.useragent.locale" is not English.
> > 
> > But changing the preferred language in about:preferences affects
> > "intl.accept_langauges" instead.
> 
> Yes, this sounds like the answer. I expect you will see the prompt if you
> visit this page:
> https://www.torproject.org/download/download
> and choose a non-English language from the drop-down menu before you click
> the Download button. Unfortunately we don't have a zh-tw version currently.

Not sure if this only happens to me, but I still get Tor browser in English even I choose some other language from the drop-down menu.  I have to manually modify the download URL to get a non-English one.
Attachment #8869390 - Flags: review?(arthuredelstein)
Hi Arthur,

Could you take a look at this patch?

Although it listens to the TabOpen event like TorButton does, its behavior is a little different.
TorButton's prompt is displayed after the page is loaded, but my patch will display the prompt as soon as you open a new tab.

Not sure what is the best event to listen to.
(In reply to Jonathan Hao [:jhao] from comment #6)

> Not sure if this only happens to me, but I still get Tor browser in English
> even I choose some other language from the drop-down menu.  I have to
> manually modify the download URL to get a non-English one.

That's not good! Thanks for noticing it. I opened a ticket:
https://trac.torproject.org/projects/tor/ticket/22357

(I have built Firefox with your patch and I will test soon.)
Comment on attachment 8869390 [details]
Bug 1039069 - Provide a popup about English for intl users.

https://reviewboard.mozilla.org/r/141044/#review148606

Looks good overall.

::: browser/base/content/browser.js:8640
(Diff revision 3)
>      }
>      return browser;
>    },
>  };
> +
> +var LanguagePrompt = {

Maybe it's better to put this in a separate js module, to keep all the parts together?

::: browser/base/content/browser.js:8641
(Diff revision 3)
>      return browser;
>    },
>  };
> +
> +var LanguagePrompt = {
> +  _prefSpoofEnglish: "privacy.resistFingerprinting.spoof_english",

I would suggest calling this "privacy.spoof_english". Also I would suggest giving it three values. Something like:
0: prompt
1: don't spoof
2: spoof

::: browser/base/content/browser.js:8653
(Diff revision 3)
> +    }
> +    this._initialized = true;
> +    // XXX: This is possibly slightly the wrong place to do this check,
> +    // but we know the TabOpen effect is late enough to provide the popup
> +    // after firefox is visible, which makes it more clear whose popup this is.
> +    gBrowser.tabContainer.addEventListener("TabOpen", this);

Would potentially be useful to show the dialog as soon as the user sets "privacy.resistFingerprinting" to true.

::: browser/base/content/browser.js:8668
(Diff revision 3)
> +      gPrefService.setCharPref("intl.accept_charsets", "iso-8859-1,*,utf-8");
> +      gPrefService.setCharPref("intl.charsetmenu.browser.cache", "UTF-8");
> +      gPrefService.setCharPref("javascript.default_locale", "en-US");
> +    } else {
> +      if(gPrefService.prefHasUserValue("intl.accept_languages")) {
> +        gPrefService.clearUserPref("intl.accept_languages");

Is there a need here to cache the user's previous values?

::: browser/base/content/browser.js:8732
(Diff revision 3)
> +
> +    var response = prompts.confirmEx(null, "", message, flags, null, null, null, null, {value: false});
> +
> +    // Update preferences to reflect their response and to prevent the prompt from
> +    // being displayed again.
> +    gPrefService.setBoolPref("privacy.resistFingerprinting.prompted_language", true);

This additional pref isn't needed if privacy.spoof_english has 3 values.
Attachment #8869390 - Flags: review?(arthuredelstein)
Comment on attachment 8869390 [details]
Bug 1039069 - Provide a popup about English for intl users.

https://reviewboard.mozilla.org/r/141044/#review148606

> Is there a need here to cache the user's previous values?

We probably won't because if we only cache them in memory, they will still be lost after the programm is shutdown.
Comment on attachment 8869390 [details]
Bug 1039069 - Provide a popup about English for intl users.

https://reviewboard.mozilla.org/r/141044/#review148606

> Maybe it's better to put this in a separate js module, to keep all the parts together?

Hi Arthur,

What do you mean by keeping all the parts together?
Comment on attachment 8869390 [details]
Bug 1039069 - Provide a popup about English for intl users.

https://reviewboard.mozilla.org/r/141044/#review148606

> Hi Arthur,
> 
> What do you mean by keeping all the parts together?

Hm, I'm not sure what I meant by that. But the latest revision looks good to me!
Comment on attachment 8869390 [details]
Bug 1039069 - Provide a popup about English for intl users.

https://reviewboard.mozilla.org/r/141044/#review156456
Attachment #8869390 - Flags: review?(arthuredelstein) → review+
Comment on attachment 8869390 [details]
Bug 1039069 - Provide a popup about English for intl users.

Requesting an additional review from Zibi, as we're doing quite a few changes to the prefs these days, and there are some parts here that might not work as they have used to for a decade.

Also, the logic around general.useragent.locale is rather brittle, I think.

I only glanced at the patch, though.

Also, still needs a firefox peer to review?
Attachment #8869390 - Flags: review?(gandalf)
Comment on attachment 8869390 [details]
Bug 1039069 - Provide a popup about English for intl users.

https://reviewboard.mozilla.org/r/141044/#review165754

On a general note, I'm a bit surprise that this goes to prompts. Prompt windows are usually bad UX, especially if their result is not later reflected in the UI. I would rather see us go for "Fx Tour" style UX for privacy where after triggering the "Moar privacy" option, we show a tour to various places in the UI with descriptions explaining how selecting particular options around various Preferences sections will increase the privacy.

Such UX would be even "gamifiable" as I can imagine a "fingerprinting-meter" showing some number and as the user selects/unselects options the number drops/increases to indicate better privacy.

Which brings me back to a more general issue with this UI - there doesn't seem to be anything in the Languages UI that helps user understand this choice. Can we add a checkbox by the Languages panel that when checked masks selected languages to "en-US"?

I imagine something along the lines of a set of "power options" that are hidden by default in Preferences, and when the user enters "more privacy" mode, we unfold them, the tour guides the user through them, and after this, they're part of the Preferences UI.

This would help the user understand what's going on with his languages preferences increasing their learning about their privacy over time (since they always can go back to Languages if they forget why the pages show in english and read/change/update their settings).

I know it's not my call, but I'm preparing Gecko to work on bug 1325870 which I'd like to drag in the direction of UI like this: https://rawgit.com/zbraniecki/fx-pref-l10n/master/ .
I'd be very happy to see a checkbox in it to mask the list of languages submitted to web pages.
Maybe such options would have some small indicator in the Preferences, like a different background/text color or an icon to indicate that they affect users privacy.

It seems to me that it would be way more informative fo the user and lead to way less confusion over time (when the user selects such an option, then forgers and then doesn't know how to bring it back).

That's not my decision of course, just a suggestion :)

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:77
(Diff revision 5)
> +    try {
> +      var locale = Services.prefs.getCharPref("general.useragent.locale");
> +      if (/chrome:\/\//.test(locale)) {
> +        return Services.prefs.getComplexValue("general.useragent.locale",
> +          Components.interfaces.nsIPrefLocalizedString).data;
> +      }

Please, use Services.locale.getAppLocaleAsLangTag instead.

And remove the try/catch - the method is infallible.
Attachment #8869390 - Flags: review?(gandalf)
Assignee: jonathan → nobody
Status: ASSIGNED → NEW
Whiteboard: [tor][fingerprinting][fp:m2] → [tor][fingerprinting][fp:m3]
Assignee: nobody → ettseng
The language prompt in Tor Browser 7.0.4.
The language prompt in Firefox based on the current patch.
The language prompt in Firefox based on the current patch.
Rebased the patch.

Although the previous patch got r+'d, we have to change it because:
1. The UX needs to be improved.
2. We have to warn the users when they navigate a website for the first time,
   not only when the pref privacy.resistFingerprinting is turned on.
Attachment #8869390 - Attachment is obsolete: true
Comment on attachment 8898192 [details] [diff] [review]
Bug 1039069 - Provide a popup about English for international users

Review of attachment 8898192 [details] [diff] [review]:
-----------------------------------------------------------------

I've got a suite of questions here, from blocking prompts to technical details.

Also one of more general question about "en-US" prefs implying less fingerprint. I wonder if we're not increasing the fingerprint by choosing English settings? Say, in Germany, most firefox users use the German version, so identifying yourself as part of a small minority in the userbase might actually do the opposite to what we intend?

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm
@@ +35,5 @@
> +    if (data == this._prefResistFingerprinting) {
> +      if (Services.prefs.getBoolPref(this._prefResistFingerprinting)) {
> +        // Ask the user if they want to make "English requests" if their default
> +        // language isn't English and the prompt hasn't been displayed before.
> +        if (this._getGeneralUseragentLocale().substring(0, 2) != "en" &&

Please use Services.locale.getAppLocaleAsBCP47().

That said, shouldn't this warn indepenent of UI locale whenever your language prefs are set to something that's not the en-US default setting?
(In reply to Axel Hecht [:Pike] from comment #25)
> > +    if (data == this._prefResistFingerprinting) {
> > +      if (Services.prefs.getBoolPref(this._prefResistFingerprinting)) {
> > +        // Ask the user if they want to make "English requests" if their default
> > +        // language isn't English and the prompt hasn't been displayed before.
> > +        if (this._getGeneralUseragentLocale().substring(0, 2) != "en" &&
> Please use Services.locale.getAppLocaleAsBCP47().

Thanks!  I'll modify the patch for this and comment 20.
(In reply to Axel Hecht [:Pike] from comment #25)
> Also one of more general question about "en-US" prefs implying less
> fingerprint. I wonder if we're not increasing the fingerprint by choosing
> English settings? Say, in Germany, most firefox users use the German
> version, so identifying yourself as part of a small minority in the userbase
> might actually do the opposite to what we intend?

Fingerprinting defense is effective only when we eliminate all the fingerprinting sources
which can be used to track individual users.
When users choose to enable fingerprinting resistance, their locale information such as
geolocation will be hidden.
So they should not be identified as part of small minority.


> That said, shouldn't this warn indepenent of UI locale whenever your
> language prefs are set to something that's not the en-US default setting?

Yes, I think we should warn users whenever the language prefs are set to non en-US
settings.

Arthur, would you like to add some comments?
Flags: needinfo?(arthuredelstein)
(In reply to Ethan Tseng [:ethan] from comment #27)

> Arthur, would you like to add some comments?

I second everything Ethan says in comment 27. Apart from that, happy to discuss any further questions Axel might have.
Flags: needinfo?(arthuredelstein)
Whiteboard: [tor][fingerprinting][fp:m3] → [tor][fingerprinting][fp:m3][ux]
(In reply to Ethan Tseng [:ethan] from comment #24)

> 2. We have to warn the users when they navigate a website for the first time,
>    not only when the pref privacy.resistFingerprinting is turned on.

Indeed, that is how Tor Browser works. When the browser starts for this first time, the Tor Browser "welcome" page is shown first. That page is located at about:tor and is local. Only when the user first attempts to navigate to a remote content page is the warning prompt shown, asking users if they want to spoof their locale as en-US.
Assignee: ettseng → cfu
Comment on attachment 8906965 [details]
Bug 1039069 - Provide a popup about English for international users.

I updated the patch to make the work flow closer to the Tor Browser.  The dialog now prompts when the first HTTP connection is going to be established.
Ref: https://gitweb.torproject.org/torbutton.git/tree/src/chrome/content/torbutton.js#n2219

The difference is that we have to listen to the pref change event of "privacy.resistFingerprinting".  If "privacy.resistFingerprinting" is being disabled, the value of "spoof_english" pref will be cleared so that once "privacy.resistFingerprinting" is enabled, the dialog can prompt again.
Attachment #8906965 - Flags: review?(arthuredelstein)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #20)
> ::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:77
> > +    try {
> > +      var locale = Services.prefs.getCharPref("general.useragent.locale");
> > +      if (/chrome:\/\//.test(locale)) {
> > +        return Services.prefs.getComplexValue("general.useragent.locale",
> > +          Components.interfaces.nsIPrefLocalizedString).data;
> > +      }
> Please, use Services.locale.getAppLocaleAsLangTag instead.
> And remove the try/catch - the method is infallible.

Hey CS,

It seems you didn't apply these two suggestions yet.
Please modify the patch according to comment 20, thanks.  :)
Attachment #8906965 - Flags: review?(arthuredelstein)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #20)

Thank you for sharing with us :)

> ::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:77
> (Diff revision 5)
> > +    try {
> > +      var locale = Services.prefs.getCharPref("general.useragent.locale");
> > +      if (/chrome:\/\//.test(locale)) {
> > +        return Services.prefs.getComplexValue("general.useragent.locale",
> > +          Components.interfaces.nsIPrefLocalizedString).data;
> > +      }
> 
> Please, use Services.locale.getAppLocaleAsLangTag instead.
> 
> And remove the try/catch - the method is infallible.

I have a question about Services.locale.getAppLocaleAsLangTag.
http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/intl/locale/LocaleService.cpp#162
According to my test, here requestedLocales has "zh-TW" because I set "general.useragent.locale" to "zh-TW", but availableLocales only has "en-US" maybe because of my system settings (I looked into ReadAvailableLocales and nsChromeRegistryChrome::GetLocalesForPackage but still have no idea how it works).  As a result, Services.locale.getAppLocaleAsLangTag returns "en-US".

So the behavior is different from just getting the "general.useragent.locale" pref.  Is the value returned from Services.locale.getAppLocaleAsLangTag more correct?
Flags: needinfo?(gandalf)
> but availableLocales only has "en-US" maybe because of my system settings

Your availableLocales only has "en-US" because that's the only locale you have installed properly. If you installed zh-TW browser, you should have it in your availableLocales. Similarly if you installed a language pack.

> So the behavior is different from just getting the "general.useragent.locale" pref.  Is the value returned from Services.locale.getAppLocaleAsLangTag more correct?

Yes. The value from the LocaleService API gives you the locale the browser is using. The pref just tells you what locale the user requested.

The requested locale is not exposed anywhere, while the UI locale may be fingerprinted IIUC.
Flags: needinfo?(gandalf)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #34)
> > but availableLocales only has "en-US" maybe because of my system settings
> 
> Your availableLocales only has "en-US" because that's the only locale you
> have installed properly. If you installed zh-TW browser, you should have it
> in your availableLocales. Similarly if you installed a language pack.
> 
> > So the behavior is different from just getting the "general.useragent.locale" pref.  Is the value returned from Services.locale.getAppLocaleAsLangTag more correct?
> 
> Yes. The value from the LocaleService API gives you the locale the browser
> is using. The pref just tells you what locale the user requested.
> 
> The requested locale is not exposed anywhere, while the UI locale may be
> fingerprinted IIUC.

Thanks a lot, that's very clear!  I will update the patch.
Comment on attachment 8906965 [details]
Bug 1039069 - Provide a popup about English for international users.

I changed Services.prefs.getCharPref("general.useragent.locale") to Services.locale.getAppLocaleAsLangTag(), and I think we don't need the _getGeneralUserAgentLocale wrapper anymore.
Attachment #8906965 - Flags: review?(arthuredelstein)
Comment on attachment 8906965 [details]
Bug 1039069 - Provide a popup about English for international users.

https://reviewboard.mozilla.org/r/178716/#review184608

::: browser/app/profile/firefox.js:570
(Diff revision 2)
> +
> +// If Accept-Language should be spoofed by en-US
> +// 0 - unset
> +// 1 - don't spoof
> +// 2 - spoof
> +pref("privacy.spoof_english", 0);

Why is spoofEnglish an integer pref? I would suggest changing it to a boolean pref unless there is a good reason not to.

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:23
(Diff revision 2)
> +class _LanguagePrompt {
> +  constructor() {
> +    this._initialized = false;
> +  }
> +
> +  get resistFingerprinting() {

This getter seems to be unnecessarily complex. I don't think we should cache pref values unless there is a significant performance benefit. But probably we should be providing a default return value in getBoolPref.

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:31
(Diff revision 2)
> +        Services.prefs.getBoolPref(kPrefResistFingerprinting);
> +    }
> +    return this._resistFingerprinting;
> +  }
> +
> +  get spoofEnglish() {

Same for this getter. I would suggest the `this._spoofEnglish` but also providing a default value for the pref.

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:100
(Diff revision 2)
> +    }
> +  }
> +
> +  _handleResistFingerprintingChanged() {
> +    delete this._resistFingerprinting; // clear pref cache
> +    if (this.resistFingerprinting) {

Read this pref directly.

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:113
(Diff revision 2)
> +    }
> +  }
> +
> +  _handleSpoofEnglishChanged() {
> +    delete this._spoofEnglish; // clear pref cache
> +    switch (this.spoofEnglish) {

Read the pref directly here also.
Attachment #8906965 - Flags: review?(arthuredelstein) → review+
(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #38)
> Comment on attachment 8906965 [details]
> Bug 1039069 - Provide a popup about English for international users.
> 
> https://reviewboard.mozilla.org/r/178716/#review184608
> 
> ::: browser/app/profile/firefox.js:570
> (Diff revision 2)
> > +
> > +// If Accept-Language should be spoofed by en-US
> > +// 0 - unset
> > +// 1 - don't spoof
> > +// 2 - spoof
> > +pref("privacy.spoof_english", 0);
> 
> Why is spoofEnglish an integer pref? I would suggest changing it to a
> boolean pref unless there is a good reason not to.

Because we also have to remember whether the message has prompted or not.  torbutton uses another bool pref "extensions.torbutton.prompted_language" to do so, but I think we can use only 1 int pref to present these 3 states in order to reduce pref query.

> ::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:23
> (Diff revision 2)
> > +class _LanguagePrompt {
> > +  constructor() {
> > +    this._initialized = false;
> > +  }
> > +
> > +  get resistFingerprinting() {
> 
> This getter seems to be unnecessarily complex. I don't think we should cache
> pref values unless there is a significant performance benefit. But probably
> we should be providing a default return value in getBoolPref.

The default values are given in browser/app/profile/firefox.js, so getBoolPref/getIntPref will not fail.

Actually I don't know how much the performance improvement is when using pref cache, but I agree that getting the prefs directly provides better readability.

> ::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:31
> (Diff revision 2)
> > +        Services.prefs.getBoolPref(kPrefResistFingerprinting);
> > +    }
> > +    return this._resistFingerprinting;
> > +  }
> > +
> > +  get spoofEnglish() {
> 
> Same for this getter. I would suggest the `this._spoofEnglish` but also
> providing a default value for the pref.
> 
> ::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:100
> (Diff revision 2)
> > +    }
> > +  }
> > +
> > +  _handleResistFingerprintingChanged() {
> > +    delete this._resistFingerprinting; // clear pref cache
> > +    if (this.resistFingerprinting) {
> 
> Read this pref directly.
> 
> ::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:113
> (Diff revision 2)
> > +    }
> > +  }
> > +
> > +  _handleSpoofEnglishChanged() {
> > +    delete this._spoofEnglish; // clear pref cache
> > +    switch (this.spoofEnglish) {
> 
> Read the pref directly here also.
(In reply to Chung-Sheng Fu [:cfu] from comment #39)

> Because we also have to remember whether the message has prompted or not. 
> torbutton uses another bool pref "extensions.torbutton.prompted_language" to
> do so, but I think we can use only 1 int pref to present these 3 states in
> order to reduce pref query.

OK, makes sense to me. I think it would be good to document that in firefox.js. Such as "0 - will prompt" or something else clearer than "unset".

> The default values are given in browser/app/profile/firefox.js, so
> getBoolPref/getIntPref will not fail.

That's true, but I get nervous that something might change in the future and break the code. But I defer to your judgment. :)

Thanks, Chung-Sheng!
Attachment #8898192 - Attachment is obsolete: true
Attachment #8906965 - Flags: review?(bugs)
Comment on attachment 8906965 [details]
Bug 1039069 - Provide a popup about English for international users.

https://reviewboard.mozilla.org/r/178716/#review185924

I think I'd prefer someone dealing with Firefox UI code to take a look. Prompts are always a bit icky, and I don't know whether we should try to avoid accessing that .jsm in case privacy.resistFingerprinting isn't set.
It might affect startup performance. Perhaps mconley has an opinion?

::: browser/app/profile/firefox.js:575
(Diff revision 3)
>  pref("privacy.panicButton.enabled",         true);
>  
>  // Time until temporary permissions expire, in ms
>  pref("privacy.temporary_permission_expire_time_ms",  3600000);
>  
> +pref("privacy.resistFingerprinting", false);

Why is this added here?
Attachment #8906965 - Flags: review?(bugs)
I forgot to add some description about this bug and patch to make it easier to read.  Sorry for inconvenience.

(In reply to Olli Pettay [:smaug] from comment #42)
> Comment on attachment 8906965 [details]
> Bug 1039069 - Provide a popup about English for international users.
> 
> https://reviewboard.mozilla.org/r/178716/#review185924
> 
> I think I'd prefer someone dealing with Firefox UI code to take a look.
> Prompts are always a bit icky, and I don't know whether we should try to
> avoid accessing that .jsm in case privacy.resistFingerprinting isn't set.
> It might affect startup performance. Perhaps mconley has an opinion?

Thanks for suggestion, I will ask mconley.

> ::: browser/app/profile/firefox.js:575
> (Diff revision 3)
> >  pref("privacy.panicButton.enabled",         true);
> >  
> >  // Time until temporary permissions expire, in ms
> >  pref("privacy.temporary_permission_expire_time_ms",  3600000);
> >  
> > +pref("privacy.resistFingerprinting", false);
> 
> Why is this added here?

Oh we have had this in all.js so this should be redundant.  I will remove it.
Attachment #8906965 - Flags: review?(mconley)
When "privacy.resistFingerprinting" is true, we want to show a prompt to ask user if we can modify the Accept-Language header to en-US.  The prompt shows when the first time a HTTP connection is established.  For expected result, please reference attachment 8898179 [details] and attachment 8898180 [details].
:cfu - can you please look at comment 20 and respond to it?

In particular, on the very minimal level I suggested:

> there doesn't seem to be anything in the Languages UI that helps user > understand this choice. Can we add a checkbox by the Languages panel that when checked masks selected languages to "en-US"?

(...)

> It seems to me that it would be way more informative fo the user and lead to way less confusion over time (when the user selects such an option, then forgers and then doesn't know how to bring it back).

I'm bringing it back because if you land this patch in its current form it will be me who'll get CCed on bugs people will report to us that websites in Firefox display in a wrong language after not understanding how this feature affects their experience and how to revert it.
Flags: needinfo?(cfu)
Comment on attachment 8906965 [details]
Bug 1039069 - Provide a popup about English for international users.

https://reviewboard.mozilla.org/r/178716/#review186636

Thanks for the patch! r-'ing due to the location of initialization, and some other issues and questions I had. See below.

::: browser/base/content/browser.js:1593
(Diff revision 3)
>            panel.hidePopup();
>          }
>        }
>      });
>  
> +    LanguagePrompt.init();

browser.js isn't the right place to init a JSM like this, since it'll be executed once per browser window. I know you've got a guard preventing the initialization code from running multiple times, but there's a better place in nsBrowserGlue.js:

http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/browser/components/nsBrowserGlue.js#1041-1112

Please schedule it inside an idle callback to minimize the probability that running the code causes us to get in the user's way.

::: browser/locales/en-US/chrome/browser/browser.properties:462
(Diff revision 3)
>  offlineApps.manageUsageAccessKey=S
>  
> +# Spoof Accept-Language prompt
> +# LOCALIZATION NOTE (privacy.spoof_english):
> +# %S is brandShortName
> +privacy.spoof_english=To give you more privacy, %S can request the English language version of web pages. This may cause web pages that you prefer to read in your native language to display in English instead.\n\nWould you like to request English language web pages for better privacy?

Has this message been approved by UX?

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:117
(Diff revision 3)
> +      default:
> +        break;
> +    }
> +  }
> +
> +  _handleHttpOnModifyRequest(subject, data) {

I'm actually not sure this is how we're supposed to hook into HTTP requests now that e10s is enabled. If you'd asked me before I saw this code, I would have said that you'd need to use a framescript to do this work.

ni?ing mixedpuppy in case I'm missing something - like, maybe we proxy http-on-modify-request to the parent and it's okay to use, but I have no idea.

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:121
(Diff revision 3)
> +
> +  _handleHttpOnModifyRequest(subject, data) {
> +    // If we are loading an HTTP page from content, show the
> +    // "request English language web pages?" prompt.
> +    try {
> +      let httpChannel = subject.QueryInterface(Ci.nsIHttpChannel);

Might as well move this down to just above it's first usage.

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:158
(Diff revision 3)
> +    } catch (e) {
> +      // do nothing
> +    }

This kind of massive try/catch worries me. This could cause us to silently let serious problems slip by. Could we only wrap the things we expect to maybe fail instead?

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:164
(Diff revision 3)
> +    let  prompts = Cc["@mozilla.org/embedcomp/prompt-service;1"]
> +      .getService(Ci.nsIPromptService);

You can use Services.prompt.
Attachment #8906965 - Flags: review?(mconley) → review-
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #45)
> :cfu - can you please look at comment 20 and respond to it?
> 
> In particular, on the very minimal level I suggested:
> 
> > there doesn't seem to be anything in the Languages UI that helps user > understand this choice. Can we add a checkbox by the Languages panel that when checked masks selected languages to "en-US"?
> 
> (...)
> 
> > It seems to me that it would be way more informative fo the user and lead to way less confusion over time (when the user selects such an option, then forgers and then doesn't know how to bring it back).
> 
> I'm bringing it back because if you land this patch in its current form it
> will be me who'll get CCed on bugs people will report to us that websites in
> Firefox display in a wrong language after not understanding how this feature
> affects their experience and how to revert it.

Thank you very much for the helpful suggestion.  Your concern has been seriously taken into account.

We also discussed with UX designer about the UI issue.  However, since fingerprinting resistance is still a hidden preference, it seems difficult for us to make changes to the product.  So we would like to implement this as how Tor Browser works at this time, and refine the flow, like adding some hints in UI, in the future if fingerprinting resistance becomes a public preference.


Hi Jacqueline,

We need your help with the UI and work flow issue.  Could you please explain the difficulty to add something in preference setting page for fingerprinting resistance?

Besides, could you also take a look at this string?

    # Spoof Accept-Language prompt
    # LOCALIZATION NOTE (privacy.spoof_english):
    # %S is brandShortName
    privacy.spoof_english=To give you more privacy, %S can request the English language version of web pages. This may cause web pages that you prefer to read in your native language to display in English instead.\n\nWould you like to request English language web pages for better privacy?

I guess it is from Tor Browser but I'm not sure if it fits our standard.  Thanks a lot!
Flags: needinfo?(cfu) → needinfo?(jsavory)
Thank you for reviewing it!

(In reply to Mike Conley (:mconley) (:⚙️) from comment #46)
> ::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:117
> (Diff revision 3)
> > +      default:
> > +        break;
> > +    }
> > +  }
> > +
> > +  _handleHttpOnModifyRequest(subject, data) {
> 
> I'm actually not sure this is how we're supposed to hook into HTTP requests
> now that e10s is enabled. If you'd asked me before I saw this code, I would
> have said that you'd need to use a framescript to do this work.

Sorry I have never heard about framescript.  Where can I learn it from?  Maybe I can still rewrite this patch using framescript if the http-on-modify-request hook has some problem with e10s enabled.

> ni?ing mixedpuppy in case I'm missing something - like, maybe we proxy
> http-on-modify-request to the parent and it's okay to use, but I have no
> idea.
Zibi - I agree with your idea of onboarding for fingerprinting since the entire idea of fingerprinting is a bit difficult to grasp, let alone each feature and why its being done. This feature in particular would certainly benefit from a better explanation. However this would be a much larger project and would require more exploration and design, personally I don't currently have bandwidth for this in the immediate future, but I will talk with the UX team and figure out priorities :)   

In terms of the UI pref, I do think we need to have an option in Preferences for the user to switch this on and off. There is a high chance that the user might not have read the initial prompt and simply click 'yes' and therefore be very confused about why their pages are not in their native language. We need to have a way for the user to change this particular setting without disabling all of fingerprinting. 

To clarify, the UI preference for this feature would be a separate checkbox from the one in Language/General selection, correct? My understanding is that the current selection of "Choose your preferred language for displaying pages" would not affect this option, let me know if I'm wrong about this. My idea is that we would have another checkbox option under the Language section that would affect this feature. 

For the string itself, I agree that it could be enhanced. I was wondering if we could use something shorter and more to the point, like:
"Changing your language setting to English will make you more difficult to identify and enhance your privacy. Do you want to request English language version of web pages?"

Learn more.. 
[yes] [no]

I'm hoping for some some feedback on these strings and plan of action before specing out everything, thank you!
Flags: needinfo?(jsavory)
> To clarify, the UI preference for this feature would be a separate checkbox from the one in Language/General selection, correct?

Sure, we can start there. I believe we'll want to end up with "Privacy" section, but I'm not opinionated if it should happen here already.

The value of having it in Languages section is that it would be right next to it. You select languages and have a checkbox saying "Screw that! For privacy reasons show only en-US".

So yeah, generally speaking. Works for me :)
Attached image Screen Shot 2017-10-24 at 4.01.26 PM.png (obsolete) — 

    
    

    
  

      
      
      
      

        Attached image
          
        Screen Shot 2017-10-24 at 4.01.42 PM.png (obsolete)
        — 
      

    


  

    
    

    
  


  
I added a checkbox in the language dialog.  It only shows when "privacy.resistFingerprinting" is true (fingerprinting resistance is enabled) and "privacy.spoof_english" is 2 (preferred language list is changed).  Attachment 8921370 [details] shows how it looks, and attachment 8921369 [details] gives an image that the checkbox is hidden.  Does it look good to you?

Besides, the patch is also attached for your reference.
Flags: needinfo?(jsavory)
Flags: needinfo?(gandalf)

    
    

    
  
yes, that looks great!

My general belief is that we'll need an overhaul of the preferences to accommodate for more holistic UX around fingerprinting options since adding a checkbox per setting is not very sustainable, but that's a great start!
Flags: needinfo?(gandalf)

    
    

    
  
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #54)
> yes, that looks great!
> 
> My general belief is that we'll need an overhaul of the preferences to
> accommodate for more holistic UX around fingerprinting options since adding
> a checkbox per setting is not very sustainable, but that's a great start!

Making fingerprinting resistance user friendly is an important issue for us in the next phase, and preference integration will be a significant part.  Thank you very much for the advice!

Besides, do you think we can ship this feature, with the initial improvement of preference settings (i.e., the checkbox in the language dialog), at this point?
Flags: needinfo?(gandalf)

    
    

    
  
> Besides, do you think we can ship this feature, with the initial improvement of preference settings (i.e., the checkbox in the language dialog), at this point?

Not my call, but with the checkbox I'm not worried about users not being able to understand what they did.
Flags: needinfo?(gandalf)

    
    

    
  
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #56)
> > Besides, do you think we can ship this feature, with the initial improvement of preference settings (i.e., the checkbox in the language dialog), at this point?
> 
> Not my call, but with the checkbox I'm not worried about users not being
> able to understand what they did.

OK then we will start reviewing the patches.  Thanks a lot!

    
    

    
      
  


  

    
    

    
      
  

    
  

        Attachment #8921375 -
        Attachment is obsolete: true

    
  

        Attachment #8906965 -
        Flags: review?(mconley)

        Attachment #8927169 -
        Flags: review?(mconley)

    
  

        Attachment #8906965 -
        Flags: review?(mconley)

    
    

    
  
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #46)
> ::: browser/locales/en-US/chrome/browser/browser.properties:462
> (Diff revision 3)
> >  offlineApps.manageUsageAccessKey=S
> >  
> > +# Spoof Accept-Language prompt
> > +# LOCALIZATION NOTE (privacy.spoof_english):
> > +# %S is brandShortName
> > +privacy.spoof_english=To give you more privacy, %S can request the English language version of web pages. This may cause web pages that you prefer to read in your native language to display in English instead.\n\nWould you like to request English language web pages for better privacy?
> 
> Has this message been approved by UX?

I updated it as jsavory suggested in comment 49.

> ::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:117
> (Diff revision 3)
> > +      default:
> > +        break;
> > +    }
> > +  }
> > +
> > +  _handleHttpOnModifyRequest(subject, data) {
> 
> I'm actually not sure this is how we're supposed to hook into HTTP requests
> now that e10s is enabled. If you'd asked me before I saw this code, I would
> have said that you'd need to use a framescript to do this work.
> 
> ni?ing mixedpuppy in case I'm missing something - like, maybe we proxy
> http-on-modify-request to the parent and it's okay to use, but I have no
> idea.

So far I can always receive the http-on-modify-request event with e10s enabled.  Can we trust it?

Besides, could you also please help with the preferences UI patch?  Thanks!

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8906965 [details]
Bug 1039069 - Provide a popup about English for international users.

https://reviewboard.mozilla.org/r/178716/#review204152

Hi! Just a few more questions - see below.

::: browser/components/nsBrowserGlue.js:1173
(Diff revision 5)
> +    Services.tm.idleDispatchToMainThread(() => {
> +      LanguagePrompt.init();
> +    });

Thanks for putting this in an idle callback. :)

::: browser/locales/en-US/chrome/browser/browser.properties:495
(Diff revision 5)
>  canvas.remember=Always remember my decision
>  
> +# Spoof Accept-Language prompt
> +# LOCALIZATION NOTE (privacy.spoof_english):
> +# %S is brandShortName
> +privacy.spoof_english=Changing your language setting to English will make you more difficult to identify and enhance your privacy. Do you want to request English language version of web pages?

English nit:

"Do you want to request English language version of web pages?"

should probably be:

"Do you want to request English language versions of web pages?"

where "version" -> "versions"

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:20
(Diff revision 5)
> +const kPrefSpoofEnglish = "privacy.spoof_english";
> +const kTopicHttpOnModifyRequest = "http-on-modify-request";
> +
> +class _LanguagePrompt {
> +  constructor() {
> +    this._initialized = false;

Minor suggestion: You might want to use the opportunity to set up a lazyPreferenceGetter on the singleton `_LanguagePrompt` instance for things like `privacy.resistFingerprinting` and `privacy.spoof_english`. See this documentation:

https://searchfox.org/mozilla-central/rev/fe75164c55321e011d9d13b6d05c1e00d0a640be/js/xpconnect/loader/XPCOMUtils.jsm#365-385

That will mean you can define a lazy property on this instance that will be an always-up-to-date representation of the preference value, and also allows you to register an update observer when the preference changes.

Not obligatory, but something worth considering.

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:87
(Diff revision 5)
> +
> +  _handleResistFingerprintingChanged() {
> +    if (Services.prefs.getBoolPref(kPrefResistFingerprinting)) {
> +      Services.prefs.addObserver(kPrefSpoofEnglish, this);
> +      if (this._shouldPromptForLanguagePref()) {
> +        Services.obs.addObserver(this, kTopicHttpOnModifyRequest, false);

Why, out of curiosity, do you need to hold a strong reference to the observer here?

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:100
(Diff revision 5)
> +  _handleSpoofEnglishChanged() {
> +    switch (Services.prefs.getIntPref(kPrefSpoofEnglish)) {
> +      case 0: // will prompt
> +        if (Services.prefs.getBoolPref(kPrefResistFingerprinting) &&
> +            this._shouldPromptForLanguagePref()) {
> +          Services.obs.addObserver(this, kTopicHttpOnModifyRequest, false);

Doesn't this mean that this observer might get added twice?

Example:

1. I have privacy.resistFingerprinting set to false and privacy.spoof_english set to 0
2. I set privacy.resistFingerprinting to true
3. I set privacy.spoof_english to 2
4. I set privacy.spoof_english to 0

Will steps 2 and 4 not mean that the observer is added twice?

::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:178
(Diff revision 5)
> +    let response = Services.prompt.confirmEx(
> +      null, "", message, flags, null, null, null, null, {value: false});
> +

Just so we're clear, has UX signed off on us using a modal for this?

I ask, because I'm pretty certain UX has wanted us to move further and further away from modals, in favour of things like doorhangers.

If the reason we're using a modal here is because we're trying to synchronously make a decision on whether or not to flip the pref before continuing the network activity, you might want to investigate using WebRequest.jsm instead.

WebRequest.jsm powers part of the WebExtension network request API, and one of the things it does is allow you to add hooks before we send request headers. What's also nice, is that those hooks can be marked "blocking", but not in a "block the main thread" kind of way - I believe if your onBeforeSendHeaders listener is blocking and returns a Promise, the nsIChannel will be suspended until the Promise resolves. This was added in bug 1254204.

This means we should be able to intercept a network request like this and suspend the network activity until a Promise is resolved - and that Promise can be resolved by something other than a modal.

Who from UX is working with us on this?

        Attachment #8906965 -
        Flags: review?(mconley) → review-

    
    

    
  
Comment on attachment 8927169 [details]
Bug 1039069 - Show a hint of the change of the preferred language list.

https://reviewboard.mozilla.org/r/198414/#review204160

This looks mostly okay, but I don't think I can sign-off on this until I'm sure that UX has also signed off on this. Specifically, Tina Hsieh (thsieh@mozilla.com) is who I'd want to look at this to ensure that this gels properly with UX's vision of how Preferences should operate.

::: browser/components/preferences/languages.js:306
(Diff revision 2)
> +      checkbox.style.opacity = 0;
> +      checkbox.style.visibility = "hidden";

We could do:

```
if (resistFingerprinting && (spoofEnglish == 2)) {
  checkbox.hidden = false;
} else {
  checkbox.hidden = true;
}
```

here instead, no?

::: browser/components/preferences/languages.xul:43
(Diff revision 2)
>                    name="pref.browser.language.disable_button.down"
>                    type="bool"/>
>        <preference id="pref.browser.language.disable_button.remove"
>                    name="pref.browser.language.disable_button.remove"
>                    type="bool"/>
> +      <preference id="privacy.spoof_english" name="privacy.spoof_english" type="int"/>

We should probably follow the formatting conventions set up by the other <preference> nodes above.

        Attachment #8927169 -
        Flags: review?(mconley)

    
    

    
  
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #63)
> ::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:20
> (Diff revision 5)
> > +const kPrefSpoofEnglish = "privacy.spoof_english";
> > +const kTopicHttpOnModifyRequest = "http-on-modify-request";
> > +
> > +class _LanguagePrompt {
> > +  constructor() {
> > +    this._initialized = false;
> 
> Minor suggestion: You might want to use the opportunity to set up a
> lazyPreferenceGetter on the singleton `_LanguagePrompt` instance for things
> like `privacy.resistFingerprinting` and `privacy.spoof_english`. See this
> documentation:
> 
> https://searchfox.org/mozilla-central/rev/
> fe75164c55321e011d9d13b6d05c1e00d0a640be/js/xpconnect/loader/XPCOMUtils.
> jsm#365-385
> 
> That will mean you can define a lazy property on this instance that will be
> an always-up-to-date representation of the preference value, and also allows
> you to register an update observer when the preference changes.
> 
> Not obligatory, but something worth considering.

This looks great, thanks for introducing it :)

Maybe we can refine the code structure with it as a follow-up improvement.

> ::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:87
> (Diff revision 5)
> > +
> > +  _handleResistFingerprintingChanged() {
> > +    if (Services.prefs.getBoolPref(kPrefResistFingerprinting)) {
> > +      Services.prefs.addObserver(kPrefSpoofEnglish, this);
> > +      if (this._shouldPromptForLanguagePref()) {
> > +        Services.obs.addObserver(this, kTopicHttpOnModifyRequest, false);
> 
> Why, out of curiosity, do you need to hold a strong reference to the
> observer here?

Yes... it doesn't exactly make sense.  I will remove the false argument.

> ::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:100
> (Diff revision 5)
> > +  _handleSpoofEnglishChanged() {
> > +    switch (Services.prefs.getIntPref(kPrefSpoofEnglish)) {
> > +      case 0: // will prompt
> > +        if (Services.prefs.getBoolPref(kPrefResistFingerprinting) &&
> > +            this._shouldPromptForLanguagePref()) {
> > +          Services.obs.addObserver(this, kTopicHttpOnModifyRequest, false);
> 
> Doesn't this mean that this observer might get added twice?
> 
> Example:
> 
> 1. I have privacy.resistFingerprinting set to false and
> privacy.spoof_english set to 0
> 2. I set privacy.resistFingerprinting to true
> 3. I set privacy.spoof_english to 2
> 4. I set privacy.spoof_english to 0
> 
> Will steps 2 and 4 not mean that the observer is added twice?

Yes.  But we expect that the only chance for privacy.spoof_english to change from 0 to 2 or 1 is that the user makes decision through the prompt.  In this way, the observer should have been removed when privacy.spoof_english is not 0, unless the user sets its value from about:config.  If we worry about this kind of special case, I will find some way to prevent it.

> ::: toolkit/components/resistfingerprinting/LanguagePrompt.jsm:178
> (Diff revision 5)
> > +    let response = Services.prompt.confirmEx(
> > +      null, "", message, flags, null, null, null, null, {value: false});
> > +
> 
> Just so we're clear, has UX signed off on us using a modal for this?
> 
> I ask, because I'm pretty certain UX has wanted us to move further and
> further away from modals, in favour of things like doorhangers.
> 
> If the reason we're using a modal here is because we're trying to
> synchronously make a decision on whether or not to flip the pref before
> continuing the network activity, you might want to investigate using
> WebRequest.jsm instead.
> 
> WebRequest.jsm powers part of the WebExtension network request API, and one
> of the things it does is allow you to add hooks before we send request
> headers. What's also nice, is that those hooks can be marked "blocking", but
> not in a "block the main thread" kind of way - I believe if your
> onBeforeSendHeaders listener is blocking and returns a Promise, the
> nsIChannel will be suspended until the Promise resolves. This was added in
> bug 1254204.
> 
> This means we should be able to intercept a network request like this and
> suspend the network activity until a Promise is resolved - and that Promise
> can be resolved by something other than a modal.

I totally understand your concern.  However, in our plan, we would like to align our fingerprinting resistance behaviors to the Tor Browser in the first phase.  We are going to improve them as next steps, making them more Firefox-like.  I believe refining this prompt will definitely be an important part of our incoming works.

> Who from UX is working with us on this?

Jacqueline Savory [:jsavory] is the UX designer who is helping us with fingerprinting related issues.
Flags: needinfo?(mconley)

    
  

        Attachment #8921369 -
        Attachment is obsolete: true

    
  

        Attachment #8921370 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Tina suggests that the behavior should be

1.  privacy.resistFingerprinting = false
    attachment 8930004 [details]
    The checkbox is hidden.

1.5 privacy.resistFingerprinting = true, privacy.spoof_english = 0 (will prompt)
    The modal has not been prompt, hide the checkbox.

2.  privacy.resistFingerprinting = true, privacy.spoof_english = 1 (don't spoof)
    attachment 8930005 [details]
    This can be reached by choosing "no" from the modal, or unchecking the checkbox.  We don't reset intl.accept_languages but allow users to change preferred language settings through Preferences UI.  This behavior is different from Tor Browser.

3.  privacy.resistFingerprinting = true, privacy.spoof_english = 2 (spoof)
    attachment 8930006 [details]
    This can be reached by choosing "yes" from the modal, or checking the checkbox.  intl.accept_languages is set to "en-US, en" and the preferred language settings UI is disabled, so users are not able to change preferred language settings once they decided to spoof accept-languages.


Hi Arthur,

What do you think of this behavior change?  Can Tor Browser apply it?


Hi Tina,

Could you please help clarify whether I understood correctly?  And please also help review the label of the checkbox "For privacy reasons show only en-US".


Thanks.
Flags: needinfo?(thsieh)
Flags: needinfo?(arthuredelstein)

    
    

    
  
Comment on attachment 8906965 [details]
Bug 1039069 - Provide a popup about English for international users.

https://reviewboard.mozilla.org/r/178716/#review206404

If UX has signed off on this, then I'm good with it. Thanks for addressing my review comments!

::: browser/locales/en-US/chrome/browser/browser.properties:502
(Diff revision 6)
>  canvas.allow.accesskey=A
>  canvas.remember=Always remember my decision
>  
> +# Spoof Accept-Language prompt
> +# LOCALIZATION NOTE (privacy.spoof_english):
> +# %S is brandShortName

Nit: %S isn't in this string, so this LOCALIZATION NOTE can be removed.

        Attachment #8906965 -
        Flags: review?(mconley) → review+

    
    

    
  
Comment on attachment 8927169 [details]
Bug 1039069 - Show a hint of the change of the preferred language list.

https://reviewboard.mozilla.org/r/198414/#review206410

Again, assuming UX has signed off on this change (specifically, I really want to ensure that Tina Hsieh has signed off on the UI change to about:preferences), then it looks good to me. Thanks!

::: browser/components/preferences/languages.js:309
(Diff revision 3)
> +    let activeLanguages = document.getElementById("activeLanguages");
> +    let availableLanguages = document.getElementById("availableLanguages");

You should be able to use `this._activeLanguages` and `this._availableLanguages`, I think.

::: browser/components/preferences/languages.js:313
(Diff revision 3)
> +    let spoofEnglish = document.getElementById("privacy.spoof_english").value;
> +    let activeLanguages = document.getElementById("activeLanguages");
> +    let availableLanguages = document.getElementById("availableLanguages");
> +    checkbox.hidden = false;
> +    switch (spoofEnglish) {
> +    case 1:

Could you please put some definitions for these int values in comments? Example:

```js
case 1: // Don't spoof accepted language
...
case 2: // Spoof accepted language
...
default: // Will prompt for spoofing if resisting fingerprinting

```

::: browser/components/preferences/languages.js:317
(Diff revision 3)
> +    switch (spoofEnglish) {
> +    case 1:
> +      activeLanguages.disabled = false;
> +      activeLanguages.selectItem(activeLanguages.firstChild);
> +      availableLanguages.disabled = false;
> +      document.getElementById("addButton").disabled = (availableLanguages.selectedIndex < 0);

Can you modify this function:

https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/browser/components/preferences/languages.js#159-164

to set the button states appropriately if `availableLanguages.selectedIndex != -1` and `!this._activeLanguages.disabled` and then call that function from both here and in case 2?

That way, we can keep the logic for enabling and disabling that button in the same place.

        Attachment #8927169 -
        Flags: review?(mconley) → review+

    
    

    
  
(In reply to Mike Conley (:mconley) (:⚙️) - Backlogged on reviews and needinfos from comment #72)
> Comment on attachment 8906965 [details]
> Bug 1039069 - Provide a popup about English for international users.
> 
> https://reviewboard.mozilla.org/r/178716/#review206404
> 
> If UX has signed off on this, then I'm good with it. Thanks for addressing
> my review comments!

Thank you very much for understanding, and let's wait for Tina's confirmation of the Preferences UI part :)

    
    

    
  
Hi Chung-Sheng,
Thanks for ni-ing me for a review! It looks good to me except for the language list should be gray-out as well in the case 3 (attachment 8930006 [details]). I'd like to see if Jacqueline is OK with these minor tweaks. Jacqueline, Feel free to let us know if there are any concerns from your side :)

The final copy string should be delivered by our content team. Before we get their recommendations, I'll propose a temporary copy here as: 
[x] Use recommended language settings for enhanced privacy
Flags: needinfo?(thsieh)

    
    

    
  
Thanks for the help on this one Tina! This looks great!

I'm thinking for the copy we want to include that we are specifically requesting English, in case the user is searching for the reason why certain web pages are appearing in English and isn't sure about the source. 

Possibly: 
[x] Request English version of web pages for enhanced privacy

Also a quick question, if the user already has the English version and enables the pref, I'm assuming that the prompt simply won't appear for them?
Flags: needinfo?(jsavory)

    
    

    
  
(In reply to Jacqueline Savory [:jsavory] UX from comment #76)
> Also a quick question, if the user already has the English version and
> enables the pref, I'm assuming that the prompt simply won't appear for them?

This is absolutely right :)

    
    

    
  
(In reply to Chung-Sheng Fu [:cfu] from comment #71)

> 1.5 privacy.resistFingerprinting = true, privacy.spoof_english = 0 (will
> prompt)
>     The modal has not been prompt, hide the checkbox.

Maybe it's better not to hide the checkbox in this case -- just show it as unchecked. So the user can check it if they prefer, before the modal is ever shown.

> 
> What do you think of this behavior change?  Can Tor Browser apply it?

Yes, I think this looks like a good plan. Thanks! I also agree with Jacqueline and Tina's suggestions for improvements.
Flags: needinfo?(arthuredelstein)

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8931202 -
        Flags: review?(jaws)

    
    

    
  
Hi jaws,

We intend to apply the gray-out effect to disabled listbox (i.e. <listbox disabled="true">).  Could you please help review the patch?

You can compare attachment 8930006 [details] and attachment 8931203 [details] for reference.

    
    

    
  
Clearing ni? from comment 65, since I think it was addressed.
Flags: needinfo?(mconley)

    
    

    
  
Comment on attachment 8931202 [details]
Bug 1039069 - Gray-out style for disabled listbox.

https://reviewboard.mozilla.org/r/202298/#review208528

::: toolkit/themes/shared/in-content/common.inc.css:247
(Diff revision 1)
> +xul|listbox[disabled="true"] xul|listitem:hover {
> +  background-color: transparent;

Why would we change the background-color on hover of an item that is in a disabled element?

If this were to stay, it would need to use the child selector instead of the descendent selector.

        Attachment #8931202 -
        Flags: review?(jaws) → review-

    
    

    
  
Thank you very much for reviewing this.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #85)
> Comment on attachment 8931202 [details]
> Bug 1039069 - Gray-out style for disabled listbox.
> 
> https://reviewboard.mozilla.org/r/202298/#review208528
> 
> ::: toolkit/themes/shared/in-content/common.inc.css:247
> (Diff revision 1)
> > +xul|listbox[disabled="true"] xul|listitem:hover {
> > +  background-color: transparent;
> 
> Why would we change the background-color on hover of an item that is in a
> disabled element?

Because the UX wants to remove the selecting effect from disabled listbox, in order to make it look really disabled.

> If this were to stay, it would need to use the child selector instead of the
> descendent selector.

Besides, there are some listitem rules using decendant selectors, should I also change them to child selector?
https://searchfox.org/mozilla-central/search?q=xul%7Clistitem&path=
Flags: needinfo?(jaws)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Chung-Sheng Fu [:cfu] from comment #86)
> Thank you very much for reviewing this.
> 
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #85)
> > Comment on attachment 8931202 [details]
> > Bug 1039069 - Gray-out style for disabled listbox.
> > 
> > https://reviewboard.mozilla.org/r/202298/#review208528
> > 
> > ::: toolkit/themes/shared/in-content/common.inc.css:247
> > (Diff revision 1)
> > > +xul|listbox[disabled="true"] xul|listitem:hover {
> > > +  background-color: transparent;
> > 
> > Why would we change the background-color on hover of an item that is in a
> > disabled element?
> 
> Because the UX wants to remove the selecting effect from disabled listbox,
> in order to make it look really disabled.
> 
> > If this were to stay, it would need to use the child selector instead of the
> > descendent selector.
> 
> Besides, there are some listitem rules using decendant selectors, should I
> also change them to child selector?
> https://searchfox.org/mozilla-central/search?q=xul%7Clistitem&path=

Yes, if you can. The descendant selector might be necessary to cross the anonymous content barrier. Can you try changing to child selector, and if that doesn't work then descendant selector will be fine.
Flags: needinfo?(jaws)

    
    

    
  
Comment on attachment 8931202 [details]
Bug 1039069 - Gray-out style for disabled listbox.

https://reviewboard.mozilla.org/r/202298/#review209846

We should stay with the descendant selector since other elements may exist between the listbox and listitem. Can you find places where a listitem is not a descendant of a listbox?

        Attachment #8931202 -
        Flags: review?(jaws) → review+

    
    

    
  
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #91)
> Comment on attachment 8931202 [details]
> Bug 1039069 - Gray-out style for disabled listbox.
> 
> https://reviewboard.mozilla.org/r/202298/#review209846
> 
> We should stay with the descendant selector since other elements may exist
> between the listbox and listitem. Can you find places where a listitem is
> not a descendant of a listbox?

Yes, in fact there are xul:listrows and xul:listbody between listbox and listitem (somehow I cannot select listitem by xul|listbox > xul|listrows > xul|listbody > xul|listitem, but xul|listbox > xul|listitem works). I will move it back to descendant selector.

So far I cannot find any listitem that is not a descendant of a listbox.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  
Keywords: checkin-needed

    
    

    
  
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4fce39a14630
Provide a popup about English for international users. r=arthuredelstein,mconley
https://hg.mozilla.org/integration/autoland/rev/69af55073d39
Show a hint of the change of the preferred language list. r=mconley
https://hg.mozilla.org/integration/autoland/rev/036a91aed264
Gray-out style for disabled listbox. r=jaws
Keywords: checkin-needed

    
    

    
  
Backed out for ESlint failures on browser/components/preferences/languages.js

https://hg.mozilla.org/integration/autoland/rev/6a06b1f9e0f17707c8059a686d765ee2f41689b2
https://treeherder.mozilla.org/logviewer.html#?job_id=148907102&repo=autoland&lineNumber=241

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/preferences/languages.js:304:32 | 'Services' is not defined. (no-undef)
Flags: needinfo?(cfu)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  
Flags: needinfo?(cfu)
Keywords: checkin-needed

    
    

    
  
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4036881d8ab0
Provide a popup about English for international users. r=arthuredelstein,mconley
https://hg.mozilla.org/integration/autoland/rev/ab3d38eb9e7d
Show a hint of the change of the preferred language list. r=mconley
https://hg.mozilla.org/integration/autoland/rev/7ef0d6e35d69
Gray-out style for disabled listbox. r=jaws
Keywords: checkin-needed

    
    

    
  
Glad to see this bug was landed.
CS, thanks for your effort!  Good job!

I appreciate all the reviewers and people who helped out. Thanks!

    
    

    
  
> When users choose to enable fingerprinting resistance, their locale
> information such as
> geolocation will be hidden.

Just for the record, that's only possible while on Tor or a VPN. Otherwise my IP address will give away enough geolocation information, so as to make en-US a very poor "resist fingerprinting" language choice. Right now Firefox asks me to use en-US from a public IP in Uruguay. It's easy to see how that could be an issue for other locales as Pike said in Comment #25

> So they should not be identified as part of small minority.

They would. Actually I am identified as such buy asking for en-US content in UY.
See Also:  → 1426413

    
  
Depends on: 1515001

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: