pilgrimd Reporter
	Description • 15 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5) Gecko/20091204 Thunderbird/3.0

We have use a Mission Control configuration script, called from greprefs/all.js, which, among other settings, locks signon.rememberSignons=false so our users can not store their passwords in Thunderbird's insecure password file.

In Thunderbird 2.x, locking signon.rememberSignons=false has the desired effect: the user is never given the option to save their password--they must enter every time they start Thunderbird.

Thunderbird 3.x with an existing account configuration migrated from an existing profile exhibits the same behaviour.  When setting up a new profile or new mail account using the wizard, the "Remember password" check-box on the form is not disabled or removed.  If the user tells the wizard to save their password, their password is stored in the signons file and subsequent launches use the stored password, thereby completely defeating locking signon.rememberSignons=false.


Reproducible: Always

Steps to Reproduce:
1. Add an MC configuration script which disables saving passwords by calling lockPref("signon.rememberSignons", false);
2. Launch v3 with no existing profile;
3. Configure a mail account using the wizard with the "Remember password" option checked;
4. Restart Thunderbird, observe not being prompted for a password;
5. Go to Tools->Options->Security tab->Saved Passwords... button and observe stored passwords;
6. Go to Tools->Options->Advanced tab->General subtab->Config Editor... button and observe signon.rememberSignons showing a value of "false" and a status of "locked", indicating v3 does register the lockPref() call.
Actual Results:  
A properly configured IMAP account with account password stored in the signons SQLite database in my profile directory, no prompting for a password and signon.rememberSignons locked to a value of false.

Expected Results:  
If signon.rememberSignons is false (either by default or by locking), the wizard should not give the user the option to save their password.

	Ludovic Hirlimann [:Usul]
	Comment 1 • 15 years ago

Blake could this be a good student project ?

	Blake Winton (:bwinton) (:☕️)
	Comment 2 • 15 years ago

I think a student could do this, but I don't know about the wisdom of having student projects on a potential blocking list…

(Thinking about it, I propose that we should mark them as student-project, and if it turns out that it is blocking and isn't being moved forward quickly enough, someone else can take it over and drive it in.)

Thanks,
Blake.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 3 • 15 years ago

Daniel might be interested in working on this as part of a student project - assigning to him.

	David :Bienvenu
	Comment 4 • 15 years ago

Not a blocker, though we'd welcome a patch.

	Daniel Assignee
	Comment 5 • 15 years ago

- Add onFocus() function to correct the behavior of remember_password checkbox in case we change signon.rememberSignons = true and come back.

- Minor changes to oninputPassword() and onblurPassword().

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 6 • 15 years ago

Comment on attachment 429723 [details] [diff] [review]
Patch #1

Unfortunately, I'm not in a position to review the code, deferring to bienvenu instead...

	David :Bienvenu
	Comment 7 • 15 years ago

Comment on attachment 429723 [details] [diff] [review]
Patch #1

Blake's review should be sufficient, but some comments anyway -

can you use let instead of var? Also, change the name of "pref" to something that indicates which pref it is, e.g., let rememberSignons = prefs.getBoolPref("...");

Also, you can use Ci instead of Components.interfaces.

	Mark Banner (:standard8)
	Comment 8 • 15 years ago

Comment on attachment 429723 [details] [diff] [review]
Patch #1

>+    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                    .getService(Components.interfaces.nsIPrefBranch);
>+    var pref = prefs.getBoolPref("signon.rememberSignons");

Sorry, I'm going to chime in here as well. Using:

Application.prefs.getValue("signon.rememberSignons", true);

(where true is the default value)

would be a much simpler way, the added benefit is that Application caches the prefs service in javascript so that we're not obtaining it from the component cache all the time. Not many people have realised this, which is why I'm bringing it up.

	Daniel Assignee
	Comment 9 • 15 years ago

Thanks everyone for all the feedback. I will revise my patch and include tests soon.

	Daniel Assignee
	Comment 10 • 15 years ago

Same behavior as Patch #1, apart from changes below:
    - Change pref variable to a meaningful name, i.e. rememberSignons_pref
    - Use Application.prefs.getValue("signon.rememberSignons", true); as Standard8 suggested
    - Use "let" instead of "var" for local variables

	Daniel Assignee
	Comment 11 • 15 years ago

Test case when signon.rememberSignons is set to "true" (default value)

	Daniel Assignee
	Comment 12 • 15 years ago

Test case when signon.rememberSignons is set to "false"

	Daniel Assignee
	Comment 13 • 15 years ago

Apart from the two testcases uploaded, there is still one case we have to consider.

That is when the user is currently in the account wizard, and decided to toggle signon.rememberSignons value (from true->false, or false->true) and come back. The onFocus() function will ensure correct behavior for this situation. (I'm not sure if someone actually do this, but just to be sure =) ).

When running the test, you will have to manually set signon.rememberSignons to the correct value. Sorry for this troublesome, but I haven't figured out how to set preference value inside Mozmill test.

Thanks.

	Daniel Assignee
	Comment 14 • 15 years ago

Patch with tests included.

Thanks.

	Blake Winton (:bwinton) (:☕️)
	Comment 15 • 15 years ago

Comment on attachment 430111 [details] [diff] [review]
Patch #3

Getting closer, but it looks like this patch contains the tests and a file which is the first patch, instead of containing all the changes from both the tests and the first patch…

(Specifically, I don't expect to see these lines in the diff:
diff -r 6804e330ba06 patch/emailWizard.patch2
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/patch/emailWizard.patch2	Thu Mar 04 03:29:43 2010 +0800
)

(If that doesn't make sense, please jump onto IRC and I'll try to explain it more.)

Thanks,
Blake.

	Daniel Assignee
	Comment 16 • 15 years ago

This is a new patch which contains only the necessary fix to the bug.

Thanks,
Daniel

	Blake Winton (:bwinton) (:☕️)
	Comment 17 • 14 years ago

Comment on attachment 432739 [details] [diff] [review]
Patch #4

>+++ b/mail/test/mozmill/accountcreation-wizard/test-accountcreation-wizard-pref_false.js	Tue Mar 16 13:31:40 2010 +0800
>+++ b/mail/test/mozmill/accountcreation-wizard/test-accountcreation-wizard-pref_true.js	Tue Mar 16 13:31:40 2010 +0800

These two files seem _really_ similar.  Perhaps you could make one file with a function that takes a parameter, and call that function twice, so that we don't duplicate logic?

>@@ -0,0 +1,34 @@
>+var setupModule = function (module) {  
>+    module.controller = new mozmill.controller.MozMillController(mozmill.utils.getWindowByType("mail:autoconfig"));

But, that's not why I'm giving this an r-.  I'm giving it an r- because the previous line causes both tests to fail for me:
TEST-UNEXPECTED-FAIL |  setupModule
  EXCEPTION: window is null
    at: controller.js line 200
            controller.js 200
            test-accountcreation-wizard-pref_false.js 8
            frame.js 463
            frame.js 498
            frame.js 562
            frame.js 411
            frame.js 568
            server.js 164
            server.js 168
TEST-PASS |  test_remember_checked_when_enter_pass_and_pref
TEST-UNEXPECTED-FAIL |  setupModule
  EXCEPTION: window is null
    at: controller.js line 200
            controller.js 200
            test-accountcreation-wizard-pref_true.js 8
            frame.js 463
            frame.js 498
            frame.js 562
            frame.js 411
            frame.js 568
            server.js 164
            server.js 168
TEST-PASS |  test_remember_checked_when_enter_pass_and_pref

Oh, and you should replace:
  var setupModule = function (module) {
with:
  function setupModule(module)
  {

And add "accountcreation-wizard" to mail/test/mozmill/mozmilltests.list

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js	Tue Mar 16 13:31:40 2010 +0800
>@@ -157,6 +157,24 @@
>     document.getElementById("fullspacer").width = document.width;
>     window.sizeToContent();
>   },
>+  
>+  // Correct the behaviour of remember_password checkbox in case we
>+  // change signon.rememberSignons = true and come back

Please switch that to javadoc style.
(with /** 
       * comments here.
       */
)

>+  onFocus: function() {
>+    let passwordElt = document.getElementById("password");
>+    let rememberPasswordElt = document.getElementById("remember_password");
>+    let rememberSignons_pref = Application.prefs.getValue("signon.rememberSignons", true);

Since we already have "passwordElt", and "rememberPasswordElt", what do you think about changing this to "rememberSignonsPref", for consistency?

And that's probably enough out of me for this review.  :)

I look forward to reviewing the next version of this patch, and to seeing what clarkbw thinks of the UI.

Thanks,
Blake.

	Daniel Assignee
	Comment 18 • 14 years ago

Hi Blake,

The test is written to be run by Mozmill extension right after you start Thunderbird/Shredder. The test assumes that the wizard is opened before the test begin. (This is the default behavior; when you start, the wizard will be opened first).

Moreover, because I cannot dynamically set the preference signons.rememberSignons to the desired value using Mozmill (Mozmill cannot select the preference value, it can only recognize the "configTreeBody" -> therefore, we cannot double-click or do anything to change the value), I choose to create 2 test cases for it. That explains the troublesome of manually setting the preference to the correct value before running the test.

Thanks,
Daniel

PS: Please take a look at the screenshot, it explains why Mozmill cannot select a preference value (this may be a bug?? I'm not sure).

	Daniel Assignee
	Comment 19 • 14 years ago

Mozmill only recognizes the container as "configTreeBody". It cannot select a particular preference value.

	Blake Winton (:bwinton) (:☕️)
	Comment 20 • 14 years ago

(In reply to comment #18)
> The test is written to be run by Mozmill extension right after you start
> Thunderbird/Shredder. The test assumes that the wizard is opened before the
> test begin. (This is the default behavior; when you start, the wizard will be
> opened first).

Okay, that makes sense, but it would be really nice if we could run the tests in an automated way from the command line, instead of having to start Thunderbird manually.

Fortunately, we can do this!  ;)

Check out <https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing>, and on your command line, try to run "make SOLO_TEST=accountcreation-wizard mozmill-one", and see what you can do to get that working.

> Moreover, because I cannot dynamically set the preference
> signons.rememberSignons to the desired value using Mozmill (Mozmill cannot
> select the preference value, it can only recognize the "configTreeBody" ->
> therefore, we cannot double-click or do anything to change the value), I choose
> to create 2 test cases for it. That explains the troublesome of manually
> setting the preference to the correct value before running the test.

If you check mail/test/mozmill/content-tabs/test-content-tab.js you can see how to get and set a pref programmatically from a mozmill test.

Thanks,
Blake.

	Mark Banner (:standard8)
	Comment 22 • 14 years ago

Andrew: iirc there was a reason why we don't use the mc.assert* functions in the mozmill tests with Thunderbird? Was it something like the stacks don't always unwind correctly?

	Andrew Sutherland [:asuth] (he/him)
	Comment 23 • 14 years ago

The asserts just end up throwing exceptions (although some of them do emit side-channel action meta-info over jsbridge) which is equivalent to what we do.

I think some of the DOM-related assert stuff just didn't work.  Otherwise, I probably just didn't see an advantage to calling an assert, especially if it was going to result in a generic exception being thrown instead of a very specific one.

I would make sure that any asserts that might rely on magic (like DOM fun and XBL bindings) fail appropriately.  For example, change the DOM id or cause a checkbox to not be checked when it should be checked, etc.  (This all would be done via human tweaking and not part of the permanent test.)

	Blake Winton (:bwinton) (:☕️)
	Comment 24 • 14 years ago

Comment on attachment 435102 [details] [diff] [review]
Patch #5

>+++ b/mail/test/mozmill/accountcreation-wizard/test.js	Fri Mar 26 01:53:39 2010 +0800

Please merge these tests into the (newly-created) mail/test/mozmill/account/test-mail-account-setup.wizard.js

(I recommend creating a new test function in that file to test the things that you want to.  It should already have the correct scaffolding.)

>+function setupModule (module) {
[snip…]
>+  plan_for_modal_dialog("mail:autoconfig", autoconfig_callback); // running test with pref value set to true

In particular, I don't think running the tests from the setupModule function was really what you wanted to do here.

>+++ b/mail/test/mozmill/mozmilltests.list	Fri Mar 26 01:53:39 2010 +0800
>@@ -6,3 +6,4 @@
> folder-pane
>+accountcreation-wizard

And we won't need this change, either, if you put it in the other file.

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js	Fri Mar 26 01:53:39 2010 +0800
>@@ -157,6 +157,26 @@
>+  

You have some trailing whitespace here which we should remove.

>+  /**
>+   * Correct the behaviour of remember_password checkbox in case we
>+   * change signon.rememberSignons = true and come back
>+   */
>+  onFocus: function() {
>+    let passwordElt = document.getElementById("password");
>+    let rememberPasswordElt = document.getElementById("remember_password");
>+    let rememberSignons_pref = Application.prefs.getValue("signon.rememberSignons", true);
>+ 
>+    if (rememberSignons_pref) {
>+      rememberPasswordElt.disabled = false;
>+      if (!this._userChangedPassword && passwordElt.value.length >= 1)
>+        rememberPasswordElt.checked = true;

Why are we checking if the user changed their password here?  Is it because the value will be the emptyText if they didn't?

>+    }
>+    else {
>+      rememberPasswordElt.disabled = true;
>+      rememberPasswordElt.checked = false; 

I don't think it matters, but I'ld prefer to see this unchecked before it's disabled.  (It makes more sense to me that you would uncheck it while it's still enabled.)

>@@ -387,15 +407,22 @@
>-    rememberPasswordElt.disabled = false;
>+    let rememberSignonsPref = Application.prefs.getValue("signon.rememberSignons", true);
>+    if (rememberSignonsPref)
>+      rememberPasswordElt.disabled = false;
>+    else {
>+      rememberPasswordElt.disabled = true;
>+      rememberPasswordElt.checked = false; 
>+    }
>     if (passwordElt.value.length < 1) {

I'ld like to see a blank line before this if statement, so that I don't think it's an else if from the previous block.

>-    else if (!this._userChangedPassword)
>-      rememberPasswordElt.checked = true;
>-  },
>+    else if (!this._userChangedPassword && rememberSignonsPref)
>+       rememberPasswordElt.checked = true;
>+   },

You seem to have indented this closing } one too many spaces.

Other than those nits, I'm really happy with it, but I'm going to r- because I want to see how you fix the tests.

Finally, since this is affecting some UI, we should probably get a ui-r from clarkbw before this gets committed.  Please post a list of steps Bryan can use to see what you've changed in the UI.

Thanks,
Blake.

	Daniel Assignee
	Comment 25 • 14 years ago

> >+  /**
> >+   * Correct the behaviour of remember_password checkbox in case we
> >+   * change signon.rememberSignons = true and come back
> >+   */
> >+  onFocus: function() {
> >+    let passwordElt = document.getElementById("password");
> >+    let rememberPasswordElt = document.getElementById("remember_password");
> >+    let rememberSignons_pref = Application.prefs.getValue("signon.rememberSignons", true);
> >+ 
> >+    if (rememberSignons_pref) {
> >+      rememberPasswordElt.disabled = false;
> >+      if (!this._userChangedPassword && passwordElt.value.length >= 1)
> >+        rememberPasswordElt.checked = true;
> 
> Why are we checking if the user changed their password here?  Is it because the
> value will be the emptyText if they didn't?
Yes, this is exactly the reason why. The password field's type is "text" when it is empty. Its type is changed to "password" only when it is not empty.

> Finally, since this is affecting some UI, we should probably get a ui-r from
> clarkbw before this gets committed.  Please post a list of steps Bryan can use
> to see what you've changed in the UI.

I'm not sure if my code made any changes to the UI or not. It only registered the onFocus() function in emailWizard.xul to handle the case where user changes signon.rememberSignons, then comes back to the wizard.

Finally, the test has been added to "mail/test/mozmill/account/test-mail-account-setup.wizard.js".

Thanks,
Daniel

	Bryan Clark (DevTools PM) [:clarkbw]
	Comment 27 • 14 years ago

Comment on attachment 443056 [details] [diff] [review]
Patch #6

This looks fine, I probably would have recommended just hiding the option from the dialog when it's not available but disabled will work as well.  I haven't tested this yet, I'm a little concerned that the onFocus will lead to the item looking enabled and then changing to disabled.  Without any feedback as to why this will be a bit confusing and possibly upsetting for some users; if this is the case.

In general though this looks fine.

	Blake Winton (:bwinton) (:☕️)
	Comment 28 • 14 years ago

Comment on attachment 443056 [details] [diff] [review]
Patch #6

>+++ b/mail/test/mozmill/account/test-mail-account-setup-wizard.js	Mon May 03 16:34:46 2010 +0800
>@@ -186,3 +186,62 @@
>+function test_remember_password() {
>+  remember_password_test(true);
>+  remember_password_test(false);
>+}
>+
>+/* Test remember_password checkbox behavior with
>+ * signon.rememberSignons set to "prefValue"
>+ */
>+function remember_password_test(prefValue) {

We like to name our parameters "aPrefValue", to distinguish them from local variables.

>+  let pref = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
>+  let rememberSignons_pref_save = pref.getBoolPref("signon.rememberSignons", true); // save the pref for backup purpose

This line is way over 80 characters, and doesn't really need to be if you move the comment to a different line.

>+  

This line and some others have some trailing spaces.  Please remove them.

>+  pref.setBoolPref("signon.rememberSignons", prefValue);
>+  let rememberSignons_pref = pref.getBoolPref("signon.rememberSignons", true);

Didn't you just set that to prefValue?  I think you can just use prefValue instead of rememberSignons_pref in the rest of this function.

>+  // password field is empty and the checkbox is disabled initially -> uncheck checkbox

If you break this after the ->, it'll fit into 80 characters.
(See below for some examples.)

>+  awc.sleep(1000);

Why are you sleeping here?  It makes the tests much slower, and there are usually better ways to do whatever it is you want to do.

>+  if (rememberSignons_pref) {
>+    // password field is not empty any more -> enable and check checkbox
>+    awc.assertProperty(rememberPassword, "disabled", false);
>+    awc.assertChecked(rememberPassword);
>+  }
>+  else {
>+    // password field is not empty any more, but prefValue is false -> disable and uncheck checkbox
>+    awc.assertProperty(rememberPassword, "disabled", true);
>+    awc.assertNotChecked(rememberPassword);
>+  }

You could do both the assertProperty checks with the single line:
awc.assertProperty(rememberPassword, "disabled", !aPrefValue);
which would turn that block into:

  awc.assertProperty(rememberPassword, "disabled", !aPrefValue);
  if (aPrefValue)
    // password field is not empty any more, and aPrefValue is true ->
    //   enable and check checkbox
    awc.assertChecked(rememberPassword);
  else
    // password field is not empty any more, and prefValue is false ->
    //   disable and uncheck checkbox
    awc.assertNotChecked(rememberPassword);

>+  // empty the password field
>+  awc.keypress(password, 'a', {ctrlKey: true});
>+  awc.keypress(password, 'VK_DELETE', {});

Heh.  Nice!

So, my final problem is that I can't get this test to run successfully.
I'm using "make SOLO_TEST=account/test-mail-account-setup-wizard.js mozmill-one", but it fails with the following error:
TEST-PASS |  setupModule
TEST-PASS |  test_mail_account_setup
TEST-UNEXPECTED-FAIL |  test_remember_password
  EXCEPTION: Timed out waiting for window open!
    at: test-window-helpers.js line 193
       Error("Timed out waiting for window open!")  0
       WindowWatcher_waitForWindowOpen("mail:autoconfig") test-window-helpers.js 193
       wait_for_new_window("mail:autoconfig") test-window-helpers.js 504
       open_mail_account_setup_wizard() test-mail-account-setup-wizard.js 68
       remember_password_test(true) test-mail-account-setup-wizard.js 202
       test_remember_password() test-mail-account-setup-wizard.js 191
            frame.js 468
            frame.js 520
            frame.js 562
            frame.js 420
            frame.js 574
            server.js 164
            server.js 168

Apart from that, the logic looks good, so if we can just fix these few things, you'll get an r+ from me.  (But we're not quite there yet.)

Thanks,
Blake.

	Wayne Mery (:wsmwk)
	Comment 29 • 14 years ago

Daniel, (small world.) Are you able to do another patch iteration?

Looks like my institution might be bit by this.

	Daniel Assignee
	Comment 30 • 14 years ago

(In reply to comment #29)
> Daniel, (small world.) Are you able to do another patch iteration?
> 
> Looks like my institution might be bit by this.

Sure, I will try to close this bug by the end of this week :P I'm currently in the middle of the last week of GSoC2010, lots of documentation to finish :)

	Daniel Assignee
	Comment 31 • 14 years ago

I have fixed all the mentioned errors. This now I used http://beaufour.dk/jst-review/ to check my patch before submitting :P

There is a point I don't understand. In the js test file, if without "mc.sleep(0);" then after the two original tests finished, the mail account wizard cannot be opened again.

	Daniel Assignee
	Comment 32 • 14 years ago

Remove all "awc.sleep(1000);" instructions and one longer-than-80-char line.

	Blake Winton (:bwinton) (:☕️)
	Comment 33 • 14 years ago

Comment on attachment 466205 [details] [diff] [review]
Patch #8

>+++ b/mail/test/mozmill/account/test-mail-account-setup-wizard.js	Mon Aug 16 10:31:38 2010 +0800
>@@ -242,3 +242,65 @@
>+function test_remember_password() {
>+  remember_password_test(true);
>+  remember_password_test(false);
>+}
>+
>+/* Test remember_password checkbox behavior with
>+ * signon.rememberSignons set to "prefValue"

This should be "aPrefValue".

>+function remember_password_test(aPrefValue) {
[snip]
>+  let password = new elementslib.ID(awc.window.document, "password");
>+  let rememberPassword =
>+      new elementslib.ID(awc.window.document, "remember_password");
>+  

Still some trailing spaces here.

>+  pref.setBoolPref("signon.rememberSignons", aPrefValue);
>+  

And here, and a few other places in this file.

>+  awc.assertProperty(rememberPassword, "disabled", !aPrefValue);
>+  if (aPrefValue) {
>+    // password field is not empty any more
>+    // -> enable and check checkbox
>+    awc.assertChecked(rememberPassword);
>+  }
>+  else {
>+    // password field is not empty any more, but prefValue is false

This should also be "aPrefValue".

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js	Mon Aug 16 10:31:38 2010 +0800
>@@ -158,6 +158,27 @@
>+  onFocus: function() {
>+    let passwordElt = document.getElementById("password");
>+    let rememberPasswordElt = document.getElementById("remember_password");
>+    let rememberSignons_pref =
>+      Application.prefs.getValue("signon.rememberSignons", true);
>+
>+    if (rememberSignons_pref) {
>+      rememberPasswordElt.disabled = false;
>+
>+      if (!this._userChangedPassword && passwordElt.value.length >= 1)
>+        rememberPasswordElt.checked = true;
>+    }
>+    else {
>+      rememberPasswordElt.checked = false;
>+      rememberPasswordElt.disabled = true;

I get the feeling we could be more clever here...

rememberPasswordElt.disabled = !rememberSignons_pref;
rememberPasswordElt.checked = rememberSignons_pref &&
                              !this._userChangedPassword &&
                              passwordElt.value.length >= 1;

But I don't know if that's actually a win.

>@@ -420,13 +441,22 @@
>   {
>     let passwordElt = document.getElementById("password");
>     let rememberPasswordElt = document.getElementById("remember_password");
>-    rememberPasswordElt.disabled = false;
>+    let rememberSignons_pref =
>+        Application.prefs.getValue("signon.rememberSignons", true);
>+
>+    if (rememberSignons_pref)
>+      rememberPasswordElt.disabled = false;
>+    else {
>+      rememberPasswordElt.checked = false;
>+      rememberPasswordElt.disabled = true;
>+    }
>+
>     if (passwordElt.value.length < 1) {
>       rememberPasswordElt.disabled = true;
>       rememberPasswordElt.checked = false;
>       this._userChangedPassword = false;
>     }
>-    else if (!this._userChangedPassword)
>+    else if (!this._userChangedPassword && rememberSignons_pref)
>       rememberPasswordElt.checked = true;

This code looks suspiciously familiar.  Can we merge it with the code above?

And finally, this test fails under Windows 7.

$ make SOLO_TEST=account/test-mail-account-setup-wizard.js mozmill-one
TEST-PASS |  setupModule
TEST-PASS |  test_mail_account_setup
TEST-PASS |  test_bad_password_uses_old_settings
TEST-UNEXPECTED-FAIL | c:\Thunderbird\comm-central\mail\test\mozmill\account\test-mail-account-setup-wizard.js | test_remember_password
  EXCEPTION: Controller.assertProperty(ID: remember_password) : false == true
    at: controller.js line 796
       Error("Controller.assertProperty(ID: remember_password) : false == true") 0
            controller.js 796
       remember_password_test(true) test-mail-account-setup-wizard.js 274
       test_remember_password() test-mail-account-setup-wizard.js 247
            frame.js 474
            frame.js 530
            frame.js 573
            frame.js 426
            frame.js 585
            server.js 164
            server.js 168
TEST-PASS |  teardownModule
make: *** [mozmill-one] Error 1

Thanks,
Blake.

	Blake Winton (:bwinton) (:☕️)
	Comment 34 • 14 years ago

Hey Daniel,
the patch at <http://pastebin.mozilla.org/770781> passes on my Windows 7 netbook, so if you want to post that as a new patch, with the rest of my suggestions, I'll give it an r+.

Thanks,
Blake.

	Daniel Assignee
	Comment 35 • 14 years ago

Checkbox is disabled and unchecked either when signon.rememberSignons == false or password's textfield is empty.

Address all bwinton's suggestion. r=bwinton.

Thanks,
Daniel

	Mark Banner (:standard8)
	Comment 36 • 14 years ago

Checked in: http://hg.mozilla.org/comm-central/rev/a219ce33c927

Thanks for all the work Daniel.

	Mark Banner (:standard8)
	Comment 37 • 14 years ago

Unfortunately I had to back this out due to test failures:

http://hg.mozilla.org/comm-central/rev/5797048efddd

TEST-PASS |  setupModule
TEST-PASS |  test_mail_account_setup
TEST-PASS |  test_bad_password_uses_old_settings
TEST-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js | test_remember_password
  EXCEPTION: no message!
    at: nonesuch line 0
TEST-PASS |  teardownModule
TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run
INFO | (runtestlist.py) | account: 4 passed, 1 failed

	Daniel Assignee
	Comment 38 • 14 years ago

(In reply to comment #37)
> TEST-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js |
> test_remember_password
>   EXCEPTION: no message!
>     at: nonesuch line 0

This is strange. I've never encountered such exception before. I'll look into this later.

	Ludovic Hirlimann [:Usul]
	Comment 39 • 14 years ago

Daniel have you had a chance to look into it ?

	Blake Winton (:bwinton) (:☕️)
	Comment 40 • 14 years ago

(In reply to comment #38)
> (In reply to comment #37)
> > TEST-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js |
> > test_remember_password
> >   EXCEPTION: no message!
> >     at: nonesuch line 0
> This is strange. I've never encountered such exception before. I'll look into
> this later.

I think I've hit this when I've tried to call a function/method that didn't exist in the client.  Perhaps that will give you a place to start looking, at least…

Later,
Blake.

	Mark Banner (:standard8)
	Comment 41 • 14 years ago

This is updated a bit, but the test still doesn't pass for me.

I discovered that input_value needed an extra argument, and the close_mail_account_setup_wizard() call was to an undefined function.

I've tried replacing that call with one from the previous test, but now I get:

SUMMARY-UNEXPECTED-FAIL | test-mail-account-setup-wizard.js | test_remember_password
  EXCEPTION: this.window.document is null
    at: test-window-helpers.js line 629
       _get_element_by_id_helper("cancel_button") test-window-helpers.js 629
       remember_password_test(true) test-mail-account-setup-wizard.js 296
       test_remember_password() test-mail-account-setup-wizard.js 235
            frame.js 530
            frame.js 598
            frame.js 641
            frame.js 480
            frame.js 653
            server.js 164
            server.js 168

I've run out of time today, but hopefully someone else can move this along a bit more.

	Mark Banner (:standard8)
	Comment 42 • 14 years ago

I'm assigning this to Blake in the hopes that he can finish fixing the unit test. Daniel, if you get time in the meantime do feel free to take another look at it - we will get this landed soon.

	Blake Winton (:bwinton) (:☕️)
	Comment 43 • 14 years ago

It was the classic "ctrlKey" vs. "accelKey" on Mac bug.
I wonder if we can get the MozMill folk to rename "ctrlKey" to "ctrlKeyThatDoesntWorkOnMacAndYouReallyWantAccelKeyInstead"?  :)

Later,
Blake.

	Mark Banner (:standard8)
	Comment 44 • 14 years ago

Comment on attachment 493286 [details] [diff] [review]
A patch that lets the tests pass.

That's better, thanks Blake.

	Mark Banner (:standard8)
	Comment 45 • 14 years ago

Checked into trunk: http://hg.mozilla.org/comm-central/rev/e757b415fed3

We've decided not to go with this for 3.1.7 as we are very close to release and this hasn't had any time for testing. This will be included in 3.1.8 assuming no issues are reported.

And a big thanks to Daniel for working on fixing this bug.

	Mark Banner (:standard8)
	Comment 46 • 14 years ago

Comment on attachment 493286 [details] [diff] [review]
A patch that lets the tests pass.

I'm going to take this for 1.9.2, the tests pass there and this fixes a regression from 2.0 for enterprise set-ups. a=Standard8

	Mark Banner (:standard8)
	Comment 47 • 14 years ago

Checked into 1.9.2:

http://hg.mozilla.org/releases/comm-1.9.2/rev/7e2b12c5b9ec