1025965 - Rename browser.safebrowsing.enabled to browser.safebrowsing.phishing.enabled

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P5)

Description

[Ed Morley [:emorley]]

In bug 1025358, the reporter believed that setting the pref browser.safebrowsing.enabled to false would disable safebrowsing entirely, whereas in fact it only controls the safebrowsing phishing protection, and browser.safebrowsing.malware.enabled needed to be set to false too.

To avoid this confusion, I suggest we rename browser.safebrowsing.enabled to browser.safebrowsing.phishing.enabled, otherwise it implies that browser.safebrowsing.enabled is a superset of browser.safebrowsing.malware.enabled.

If we do this, we will need to migrate the pref state from old profiles.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

I think the problem is too many prefs. If someone doesn't trust safebrowsing, there's no reason to enable it only for phishing and not malware (or vice versa). In fact, only ~1% of users touch these prefs at all (http://monica-at-mozilla.blogspot.com/2013/02/writing-for-98.html).

It would make more sense to consolidate into a single pref, browser.safebrowsing.enabled, and make it easier for people who don't want it to turn it off.

Comment 2

[Ed Morley [:emorley]]

That sgtm as well :-)

Comment 3

[Gian-Carlo Pascutto [:gcp]]

I agree, but I do think you want a separate pref for the new download protection. (I suggested making this change in the real UX instead of about:config and you rebuffed me :-)

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)
> I agree, but I do think you want a separate pref for the new download
> protection. (I suggested making this change in the real UX instead of
> about:config and you rebuffed me :-)

Sorry, I still disagree :) Having yet another pref sounds even more unusable. If you don't trust Google, you shouldn't use their services. Besides, if someone really wants to turn off the download protection then a pref already exists (the apprepURL one) to turn it off.

Comment 5

[François Marier [:francois]]

Thomas, is this something you'd like to take a look at?

I suggest keeping the change to a minimum and simply renaming browser.safebrowsing.enabled to browser.safebrowsing.phishing.enabled. This will be a rather large change because we need to update all of the tests that rely on this. dxr.mozilla.org is your friend :)

Comment 6

[Thomas Nguyen (:tnguyen)]

Sure, I will take it

Comment 7

[:Paolo Amadini]

When you rename a preference just for naming clarity, you also need to add code to nsBrowserGlue.js to migrate the previous value. This ensures that users who had previously disabled the preference don't find it enabled unexpectedly. Example:

http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js?rev=a07ea7fb572f#2090

Firefox for Android may need migration code as well, but I'm not sure how that is handled.

Comment 8

[Thomas Nguyen (:tnguyen)]

Attachment: Part 1 : Rename pref

Comment 9

[Thomas Nguyen (:tnguyen)]

Attachment: Part 2 : migrate pref

Comment 10

[François Marier [:francois]]

Comment on attachment 8750235 [details] [diff] [review]
Part 2 : migrate pref

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

That looks good to me, but I'll let Paolo have a quick look too since I didn't know about BrowserGlue before he mentioned it.

::: browser/components/nsBrowserGlue.js
@@ +2198,5 @@
>      }
>  
> +    if (currentUIVersion < 38) {
> +      const kOldSafeBrowsingPref = "browser.safebrowsing.enabled";
> +      // Default value is set to true, a has user pref means that the pref was

typo: the word "has" should probably not be there

::: mobile/android/chrome/content/browser.js
@@ +1030,5 @@
>      }
>  
> +    if (currentUIVersion < 3) {
> +      const kOldSafeBrowsingPref = "browser.safebrowsing.enabled";
> +      // Default value is set to true, a has user pref means that the pref was

same typo with the word "has"

Comment 11

[François Marier [:francois]]

Comment on attachment 8750233 [details] [diff] [review]
Part 1 : Rename pref

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

Looks good to me.

In terms of testing, you can run:

  mach test toolkit/components/url-classifier
  mach test browser/components/safe-browsing

and then all of the browser/base/content/test/general/browser_trackingUI_* tests too, just to make sure.

I would also do a try run on Windows, Mac, Linux and Android to make sure everything is fine there too.

Comment 12

[Thomas Nguyen (:tnguyen)]

Thanks Francois.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f803013d8b5&selectedJob=20399507
Updated part 2 because there's a failure on try on Android 4.3

Comment 13

[Thomas Nguyen (:tnguyen)]

Attachment: Part 2 : migrate pref

Comment 14

[Thomas Nguyen (:tnguyen)]

Attachment: Part 2 : migrate pref

Comment 15

[:Paolo Amadini]

Comment on attachment 8751062 [details] [diff] [review]
Part 2 : migrate pref

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

The code looks good to me, make sure you also test the migration manually.

Comment 16

[:Paolo Amadini]

I've marked this bug for front-end QA as well, here is an overview of what to test:

1. Create a clean profile with a previous version of the browser
2. Switch the phishing preference from the Preferences window
3. Update to the new version and check the preference remains the same
4. Check that there are no errors from nsBrowserGlue.js in the Browser Console.
5. Repeat the above, but skip step 2.

Comment 17

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8b7901919fa&selectedJob=22754126

Try looks good

Comment 18

[Thomas Nguyen (:tnguyen)]

Attachment: migrate pref

Rebase and carry r+

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/4284721d82dd
Rename browser.safebrowsing.enabled to browser.safebrowsing.phishing.enabled. r=francios
https://hg.mozilla.org/integration/fx-team/rev/13aad7c0cfda
Migrate browser.safebrowsing.enabled to browser.safebrowsing.phishing.enabled. r=paolo.mozmail

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4284721d82dd
https://hg.mozilla.org/mozilla-central/rev/13aad7c0cfda

Comment 21

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

Verified fixed on Nightly 50.0a1, 2016-07-12.

I checked preferences via about:config, plus used the test in comment 16 (thanks).