871464 - Master password menu isn't updated after enable/disable the password

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Paul Feher]

Nightly 23.0a1 (2013-05-12)
Device: Nexus 4 (4.2.2), HTC Desire Z (2.3.3)

Steps to reproduce:
1.Set Master Password.

Expected results:
The Use Master Password option is checked and after accessing the "Use master password" again the "Remove Master Password" pop-up appears. 

Actual results:
The Use Master Password option isn't checked and after accessing the "Use master password" again the "Create Master Password" pop-up appears. After leaving and re-entering the settings menu, Use Master Password option is updated.

Comment 1

[Aaron Train [:aaronmt]]

Please find a regression range.

Comment 2

[Paul Feher]

The regression window for this issue is:

good build:
2013/05/23

bad build 
2013/05/24

possible push-log:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=acf388eaf9e9&tochange=fef5f202b2dc

Comment 3

[Aaron Train [:aaronmt]]

Bug 773535 (http://hg.mozilla.org/mozilla-central/rev/43ddb78c1718) ?

Comment 4

[Wesley Johnston (:wesj)]

I'm not sure what would have broken this, and am even a bit less sure why it was working. My original patches relied on the fact that PreferencesScreen registered for Preferences:Data while it was alive, and just sent back the updated pref from MasterPassword.js when it changed. 

That was all removed in bug 753312 (in fact, you can see it dump an error when the message comes up from Masterpassword.js), but continued working because we were hitting onWindowFocusChanged when the dialog appears/disappears and we refetch all the prefs.

I think we should probably observe the prefs here and pick up changes to any of them...

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Comment 6

[:Margaret Leibovic]

Comment on attachment 749078 [details] [diff] [review]
Patch v1

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

I'm still not 100% confident about how these pref listeners work, but this change seems reasonable. r+ with comments addressed.

::: mobile/android/base/GeckoPreferenceFragment.java
@@ +31,5 @@
>          addPreferencesFromResource(res);
>  
>          PreferenceScreen screen = getPreferenceScreen();
>          setPreferenceScreen(screen);
> +        int mPrefsListener = ((GeckoPreferences)getActivity()).setupPreferences(screen);

You don't need to declare the type here, right?

::: mobile/android/base/GeckoPreferences.java
@@ +56,5 @@
>      public static String PREFS_HEALTHREPORT_UPLOAD_ENABLED = NON_PREF_PREFIX + "healthreport.uploadEnabled";
>  
>      private static boolean sIsCharEncodingEnabled = false;
>      private boolean mInitialized = false;
> +    private int mPrefsListener = -1;

Looking at the PrefsHelper.getPrefs implementation, maybe a better name for this would be something like mPrefsRequestId. Also, you could initialize this to 0, since the requestId returned will always be greater than 0 (unless there's an error, in which case it's -1, maybe we need a check for that?).

@@ +185,5 @@
>              Log.e(LOGTAG, "Exception handling message \"" + event + "\":", e);
>          }
>      }
>  
> +    public int setupPreferences(PreferenceGroup prefs) {

Nit: Add a comment about what the return value means.

@@ +511,5 @@
>          return dialog;
>      }
>  
>      // Initialize preferences by requesting the preference values from Gecko
> +    private int getGeckoPreferences(final PreferenceGroup screen, ArrayList<String> prefs) {

Nit: Same thing here (or maybe just one place is enough, since it's the same value).

::: mobile/android/chrome/content/browser.js
@@ +948,5 @@
>      webBrowserPrint.print(printSettings, download);
>    },
>  
> +  notifyPrefObservers: function(aPref) {
> +    for each (let requestId in this._prefObservers[aPref]) {

Since this._prefObservers is an array, we should just use the built-in forEach method. In fact, for each ... in is deprecated:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/for_each...in

I see we use this elsewhere, we should file a follow-up bug to fix that. There are also other places where we're using for ... in on this._prefObservers, and that's poor form because using for ... in on arrays can have unexpected results. We should update those uses to just use regular for (let i; i < foo; i++) loops.

Comment 7

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b30a01c5924a

Comment 8

[Wesley Johnston (:wesj)]

Made a little mistake fixing the forEach bit. Fixed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6f6eb6cde95

Comment 9

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

Request uplift

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 749078 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 773535
User impact if declined: Pref checkbox isn't updated when you change it
Testing completed (on m-c, etc.): Landed on mc today
Risk to taking this patch (and alternatives if risky): medium low risk. Changes the pref listener we already had to observe pref changes. Alters some js code to use the new observer stuff
String or IDL/UUID changes made by this patch: None.

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 749078 [details] [diff] [review]
Patch v1

Extra bake time on Aurora likely won't uncover regressions anyway. Approving for uplift.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b30a01c5924a
https://hg.mozilla.org/mozilla-central/rev/b6f6eb6cde95

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/47ef97f263ea
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d972d675107

Comment 14

[Paul Feher]

Verified fixed on Nightly 23.0a1 (2013-05-19) and Aurora 23.0a2 (2013-05-19)
Device: Nexus 4 (4.2.2)