Closed Bug 871464 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

23 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 24
Tracking Status
firefox22 --- unaffected
firefox23 --- verified
firefox24 --- verified

People

(Reporter: paul.feher, Assigned: wesj)

References

(Depends on 1 open bug)

Details

(Keywords: regression, reproducible)

Attachments

(1 file)

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.
tracking-fennec: --- → ?
Whiteboard: regression
Please find a regression range.
Flags: needinfo?(paul.feher)
Whiteboard: regression
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
Flags: needinfo?(paul.feher)
Keywords: reproducible
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...
Flags: needinfo?(wjohnston)
Attached patch Patch v1Splinter Review
Attachment #749078 - Flags: review?(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.
Attachment #749078 - Flags: review?(margaret.leibovic) → review+
Made a little mistake fixing the forEach bit. Fixed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6f6eb6cde95
Request uplift
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.
Attachment #749078 - Flags: approval-mozilla-aurora?
Comment on attachment 749078 [details] [diff] [review]
Patch v1

Extra bake time on Aurora likely won't uncover regressions anyway. Approving for uplift.
Attachment #749078 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/b30a01c5924a
https://hg.mozilla.org/mozilla-central/rev/b6f6eb6cde95
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Verified fixed on Nightly 23.0a1 (2013-05-19) and Aurora 23.0a2 (2013-05-19)
Device: Nexus 4 (4.2.2)
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
Depends on: 1071261
You need to log in before you can comment on or make changes to this bug.