1241566 - Disable malware download protection in Firefox for Android 46

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

We don't want to ship malware download protection (bug 936041) until we have the notification in place (bug 1239094). Since that won't happen until 47, we should disable this feature on 46.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

There's an implicit call here that we rather download malware rather than confuse the user. No judgement, just pointing out the tradeoff.

Comment 2

[:Margaret Leibovic]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #1)
> There's an implicit call here that we rather download malware rather than
> confuse the user. No judgement, just pointing out the tradeoff.

Correct, we discussed this point in the Android funnel meeting earlier this week.

NI to Barbara to confirm that decision.

Comment 3

[Barbara Bermes [:barbara] - NI please!]

confirmed

Comment 4

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1241566 - Put malware download protection behind a switchboard flag. r=liuche

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

Comment 5

[:Margaret Leibovic]

I still need to update the experiments config, but what do you think of this approach for setting gecko prefs?

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8713804 [details]
MozReview Request: Bug 1241566 - Put malware download protection behind a switchboard flag. r=liuche

https://reviewboard.mozilla.org/r/32829/#review29823

With change to address always changing the pref.

::: mobile/android/chrome/content/browser.js:6974
(Diff revision 1)
> +            Services.prefs.setBoolPref("browser.safebrowsing.downloads.enabled", true);

What if a user turns off malware protection by toggling the pref - doesn't this turn it on again every time time Experiments gets init-ed?

I'm not sure how to check that - prefHasUserValue doesn't record if a user has toggled the pref back to the default value (which we're changing in the mobile.js part of this patch I think).

Comment 7

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #6)
> Comment on attachment 8713804 [details]
> MozReview Request: Bug 1241566 - Put malware download protection behind a
> switchboard flag. r=liuche
> 
> https://reviewboard.mozilla.org/r/32829/#review29823
> 
> With change to address always changing the pref.
> 
> ::: mobile/android/chrome/content/browser.js:6974
> (Diff revision 1)
> > +            Services.prefs.setBoolPref("browser.safebrowsing.downloads.enabled", true);
> 
> What if a user turns off malware protection by toggling the pref - doesn't
> this turn it on again every time time Experiments gets init-ed?

Good catch.

> I'm not sure how to check that - prefHasUserValue doesn't record if a user
> has toggled the pref back to the default value (which we're changing in the
> mobile.js part of this patch I think).

I don't think we need to worry about this case - this is just an about:config pref, and we don't make any guarantees of service over that.

But you know what... this patch is actually bad because it doesn't handle reverting the pref back to the mobile.js default when the experiment is over. I think I actually need to rethink this set the pref to false if the experiment *isn't* active.

Comment 8

[:Margaret Leibovic]

Comment on attachment 8713804 [details]
MozReview Request: Bug 1241566 - Put malware download protection behind a switchboard flag. r=liuche

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

Comment 9

[:Margaret Leibovic]

I decided to update the patch to clear the prefs before we check the active experiments. This should take care of resetting the prefs if the experiment is disabled.

I don't think we should try to support users changing this pref in about:config. It's an implementation detail that they can change this in about:config, they shouldn't be manually enabling/disabling malware protection.

Some people may disagree with me on this, but I see about:config as a developer/tester tool, not a tool for end users.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Why not use default prefs branch for managing these experiments? This is how we set prefs via distributions.

Comment 11

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #10)
> Why not use default prefs branch for managing these experiments? This is how
> we set prefs via distributions.

This is the right call, I forgot about default prefs. I'm going to update the patch to do this, and it will solve the problems mentioned above.

Comment 12

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/6599b14546f1a62e7089e9b8b404799db54b5da0
Bug 1241566 - Put malware download protection behind a switchboard flag. r=liuche

Comment 13

[:Margaret Leibovic]

Comment on attachment 8713804 [details]
MozReview Request: Bug 1241566 - Put malware download protection behind a switchboard flag. r=liuche

Approval Request Comment
[Feature/regressing bug #]: Bug 936041.

[User impact if declined]: Feature will be enabled that we did not intend to ship.

[Describe test coverage new/current, TreeHerder]: Tested locally to confirm this disables the feature. 

[Risks and why]: Low-risk, pref change to disable feature. There's more risk than normal because I also added code to control this pref via switchboard, but if that logic is broken, the feature would just stay disabled. So it wouldn't introduce a new problem.

[String/UUID change made/needed]: None.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6599b14546f1

Comment 15

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Comment on attachment 8713804 [details]
MozReview Request: Bug 1241566 - Put malware download protection behind a switchboard flag. r=liuche

Holding back this feature until 47. Please uplift to aurora.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/294bb4d39a9a