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

VERIFIED FIXED in Firefox 23

Status

()

VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: paul.feher, Assigned: wesj)

Tracking

(Depends on: 1 bug, {regression, reproducible})

23 Branch
Firefox 24
ARM
Android
regression, reproducible
Points:
---

Firefox Tracking Flags

(firefox22 unaffected, firefox23 verified, firefox24 verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
status-firefox22: --- → unaffected
status-firefox23: --- → affected
(Reporter)

Updated

6 years ago
tracking-fennec: --- → ?
Whiteboard: regression
Please find a regression range.
Flags: needinfo?(paul.feher)
Keywords: regression, regressionwindow-wanted
Whiteboard: regression
(Reporter)

Comment 2

6 years ago
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: regressionwindow-wanted
Bug 773535 (http://hg.mozilla.org/mozilla-central/rev/43ddb78c1718) ?
Flags: needinfo?(wjohnston)

Updated

6 years ago
Keywords: reproducible
(Assignee)

Comment 4

6 years ago
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)
(Assignee)

Comment 5

6 years ago
Created attachment 749078 [details] [diff] [review]
Patch v1
Attachment #749078 - Flags: review?(margaret.leibovic)

Comment 6

6 years ago
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+
(Assignee)

Comment 8

6 years ago
Made a little mistake fixing the forEach bit. Fixed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6f6eb6cde95
Request uplift
(Assignee)

Comment 10

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
(Reporter)

Comment 14

6 years ago
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
status-firefox23: fixed → verified
status-firefox24: fixed → verified
tracking-fennec: ? → ---

Updated

4 years ago
Depends on: 1071261
You need to log in before you can comment on or make changes to this bug.