Closed Bug 1395419 Opened 7 years ago Closed 7 years ago

Migrate values of urlclassifier.malwareTable that were customized via about:preferences

Categories

(Toolkit :: Safe Browsing, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: francois, Assigned: tnguyen)

Details

(Whiteboard: #sbv4-m9)

Attachments

(1 file, 1 obsolete file)

If a user unticked "Warn you about unwanted and uncommon software" prior to upgrading to Firefox 56, then they have a non-default value of urlclassifier.malwareTable:

  goog-malware-shavar,test-malware-simple

During the upgrade to 56, we should replace that with the following:

  goog-malware-proto,test-malware-simple
We could also attempt to fix the prefs that were customized via about:config by going through urlclassifier.*Table and then replacing the known goog-*-shavar lists with the V4 equivalents.

However, that's lower priority (and should be done in a different bug) because we don't support users editing random prefs via about:config. That's something we can think about in 58.
Assignee: nobody → tnguyen
Comment on attachment 8904161 [details] [diff] [review]
Migrate values of urlclassifier.malwareTable that were customized via about:preferences

I think we only don't need to fix in nightly but only need to uplift the patch to beta (upgrade from 55-56).
Attachment #8904161 - Flags: review?(francois)
Status: NEW → ASSIGNED
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #3)
> I think we only don't need to fix in nightly but only need to uplift the
> patch to beta (upgrade from 55-56).

I think it needs to be fixed on mozilla-central, and then uplifted to Beta.

I don't think we can have a higher `UI_VERSION` on Beta than on mozilla-central.
[Tracking Requested - why for this release]: Users who have customized the Safe Browsing preferences in about:preferences will not be getting a clean Safe Browsing V4 if we don't migrate their pref. They will be getting a mixture of V2 and V4.
Comment on attachment 8904161 [details] [diff] [review]
Migrate values of urlclassifier.malwareTable that were customized via about:preferences

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

::: browser/components/nsBrowserGlue.js
@@ +1734,5 @@
>    },
>  
>    // eslint-disable-next-line complexity
>    _migrateUI: function BG__migrateUI() {
> +    const UI_VERSION = 51;

Probably needs to be based on the version that's on mozilla-central right now otherwise Beta and central will be out of sync.

@@ +2095,5 @@
>        }
>      }
>  
> +    if (currentUIVersion < 51) {
> +      const MALWARE_PREF = "urlclassifier.malwareTable";

Add a comment like:

    // Update user customizations that will interfere with the Safe Browsing V2 to V4 migration (bug 1395419).

@@ +2097,5 @@
>  
> +    if (currentUIVersion < 51) {
> +      const MALWARE_PREF = "urlclassifier.malwareTable";
> +      if (Services.prefs.prefHasUserValue(MALWARE_PREF)) {
> +        let malwareList = Services.prefs.getCharPref(MALWARE_PREF).split(",");

I don't think you need to split/join the string.

You could simply do:

    s = getCharPref()
    s.replace('goog-malware-shavar', 'goog-malware-proto')
    setCharPref(s)
Attachment #8904161 - Flags: review?(francois) → review-
(In reply to François Marier [:francois] from comment #4)
> (In reply to Thomas Nguyen[:tnguyen] ni plz from comment #3)
> > I think we only don't need to fix in nightly but only need to uplift the
> > patch to beta (upgrade from 55-56).
> 
> I think it needs to be fixed on mozilla-central, and then uplifted to Beta.
> 
> I don't think we can have a higher `UI_VERSION` on Beta than on
> mozilla-central.

Actually, thinking about it some more, it's more complicated than this.

We'll be initially shipping V2 to 56 and then gradually rolling out V4. This means that we can't migrate the pref in 56.

Instead, we'll have to:

1. migrate the pref in 57 via the mechanism your patch uses
2. migrate the pref on release 56 when we go to 100% (hotfix / point release)
Attachment #8904161 - Attachment is obsolete: true
Agree, I push a new patch with the same idea, but only for nightly.
We may fix in nightly but leave this bug open until we could do the same with release 56.
(Fennec need to be done in the same way but in nightly 58)
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #9)
> (Fennec need to be done in the same way but in nightly 58)

We don't need to do any migrations for Fennec since it doesn't have any UI prefs for Safe Browsing.
Comment on attachment 8904820 [details]
Bug 1395419 - Migrate values of urlclassifier.malwareTable that were customized via about:preferences

https://reviewboard.mozilla.org/r/176578/#review182074
Attachment #8904820 - Flags: review?(francois) → review+
Pushed by tnguyen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30832c0f384d
Migrate values of urlclassifier.malwareTable that were customized via about:preferences r=francois
https://hg.mozilla.org/mozilla-central/rev/30832c0f384d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.