534588 - [autoconfig] Cancel doesn't work properly: The account name will change back to default value automatically set by TB in the Mail account Setup

Status: RESOLVED DUPLICATE of bug 549045

(Thunderbird :: Account Manager, defect)

Description

[emily chen]

Steps:
1. Create a new profile by #thunderbird -P 
2. Pop out a "Mail Account Setup" dialog, fill all the field, for example: 
   Your name: Emily Chen
   Email address: [e-mail]
   Password : *******

   Then press "Continue". 
3. Then press "Stop" button
4. The username and the server value has been set by Thunderbird automatically, but it is not the right one.  So need to change manually.  
   For example, the username is set as "emily.chen" by default, incoming and outgoing account is set as: "sun.com", but I need to change it : 
   Change username from emily.chen -> echen 
   Change server from sun.com-> mailserver.sun.com

Actual result: 

After change username from emily.chen -> echen, leave the server as "sun.com", wait for 10 second, the username will change back to emily.chen from echen. 

It is very annoying when user try to re-configure the account name and server.

This bug can be reproduced on Linux and OpenSolaris, on TB 3.0.

Comment 1

[Boying Lu]

Attachment: Cancel probe when user click 'stop' button

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 418969 [details] [diff] [review]
Cancel probe when user click 'stop' button

Blake should review this as it may fix one or two of his other bugs as well

Comment 3

[Mark Banner (:standard8)]

(and he knows the code way better than I do).

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 418969 [details] [diff] [review]
Cancel probe when user click 'stop' button

>diff -r 9f173515ca33 mailnews/base/prefs/content/accountcreation/emailWizard.js
>@@ -88,16 +88,17 @@ function EmailConfigWizard()
>   this._init();
> }
> EmailConfigWizard.prototype =
> {
>   _init : function EmailConfigWizard__init()
>   {
>     gEmailWizardLogger.info("Initializing setup wizard");
>     this._probeAbortable = null;
>+    this._stopped = false; // true: 'stop' button is clicked by user 

I'ld prefer it if this was closer to a full sentence, like:
Set to true when the 'stop' button is clicked.

>@@ -448,16 +449,21 @@ EmailConfigWizard.prototype =
>                 me.startSpinner("all", "looking_up_settings")
>                 me._guessConfig(domain, initialConfig, 'both');
>+                // If user clicks the "stop" button,we should cancel the current probe
>+                // or probe will reset the fields (such as 'Username') when it's timeout
>+                // User should click (Go) button to restart probe. 

There's a space at the end of this line which shouldn't be there, and these lines are longer than 80 characters, so we should probably re-wrap them.

>+                if ( me._stopped)
>+                 me._probeAbortable.cancel() ;

I think we want to set me._probeAbortable to null here, and would like to see a similar check at line 439.

Actually, if we know we're going to cancel the check, why are we even calling me._guessConfig(), or me.startSpinner()?

Furthermore, we're going to start requiring that all patches to mail/, mailnews/, directory/, and editor/ include automated tests in the near future, so I'ld _really_ like to see a test or two for this, which I admit kind of sucks because there aren't any autoconfig front-end tests yet.

Finally, I really do like this patch, (I'm mainly setting it to r- because I want to look over the tests before I give it an r+,) and think it might clear up a couple of the bugs on my plate, so if you need any help with the tests, feel free to ping me on IRC, in #maildev, and I'll be happy to give you a hand.

Thanks,
Blake.

Comment 5

[Boying Lu]

Attachment: new patch based on comments

me._guessConfig() needs to be called, it will fill fields with default values. e.g. SSL port number. User usually won't change this.

We can't just set me._probeAbortable to null because it's detector (created by me._probeAbortable) which resets those fieds. Setting me._probeAbortable to null doesn't help on this.

Comment 6

[Boying Lu]

Attachment: Modified the comments

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 420256 [details] [diff] [review]
Modified the comments

The changes look good, but I'm not going to give it a full review until you've added some unit tests.  :)

(I'm clearing the review status, so that it doesn't show up in my list of bugs to review.)

Thanks,
Blake.

Comment 8

[Boying Lu]

Attachment: test case

Comment 9

[Boying Lu]

Attachment: test case

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 430504 [details] [diff] [review]
test case

>+++ b/mail/test/mozmill/account-setup-wizard/test-account-setup.js	Fri Mar 05 11:47:42 2010 +0800
>@@ -0,0 +1,107 @@
>+/* ***** BEGIN LICENSE BLOCK *****                                                                                                    

There's a lot of trailing spaces on this line which should be removed.
(And a couple of other lines have trailing spaces, too.)

>+ * The Initial Developer of the Original Code is
>+ * Mozilla Messaging, Inc.

I've been told that this should be "The Mozilla Foundation.", since they own the copyrights on Mozilla Messaging code.  (Or I believe it could be you, if you didn't want to assign the copyright to Mozilla for some reason.)

>+ * Portions created by the Initial Developer are Copyright (C) 2009

This should probably be (C) 2010.

>+function setupModule(module) {
>+  let wh = collector.getModule('window-helpers');
>+  wh.installInto(module);
>+  
>+  mc = wh.wait_for_existing_window("mail:3pane");
>+}
>+
>+function test_account_wizard_setup()
>+{
>+  // Step1: open account manager window 
>+  
>+  plan_for_modal_dialog("mailnews:accountmanager",accountmanager_callback);
>+
>+  mc.waitThenClick(new elementslib.Elem(mc.menus.menu_Edit.menu_accountmgr));

Since this is a menu, I think you can just click it.
Also, I really don't see an "accountmgr" menu item in my Edit menu.  (I'm on a Mac.) You could just get the element by id, and click it that way, though, because the id is the same no matter where the menu item has been put.  ("mc.click(mc.eid("menu_accountmgr"));" is what the code would look like.)

>+var accountmanager_callback = function am_callback(c)
>+{
>+   // Step2: click "add mail account" button
>+   // a Mail account setup wizard open will be opened
>+   plan_for_modal_dialog("mail:autoconfig",autoconfig_callback);
>+   
>+   var addMailAccountButton = c.eid("accountActionsAddMailAccount");
>+
>+   c.click(addMailAccountButton);
>+   wait_for_modal_dialog("mail:autoconfig");
>+   c.window.close();    
>+}

So, you should be able to shortcut this step by just clicking the newMailAccountMenuItem right from the main window.

You'll also need to edit mail/test/mozmill/mozmilltests.list, and add account-setup-wizard somewhere in that list.

But other than those small things, I'm happy with it, so r=me with the nits fixed.

And a review of the patch itself is coming up.

Later,
Blake.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 420256 [details] [diff] [review]
Modified the comments

>+++ b/mailnews/base/prefs/content/accountcreation/emailWizard.js	Wed Jan 06 12:11:31 2010 +0800
>@@ -88,16 +88,18 @@ function EmailConfigWizard()
>   this._init();
> }
> EmailConfigWizard.prototype =
> {
>   _init : function EmailConfigWizard__init()
>   {
>     gEmailWizardLogger.info("Initializing setup wizard");
>     this._probeAbortable = null;
>+    // Set to true when 'stop' button is clicked
>+    this._stopped = false; 

There's still a trailing space here.

>+                // If user clicks the 'stop' button, we should cancel 
>+                // the current probe or probe will reset the fields 

And a couple more trailing spaces on the above lines.

Other than that, I like it.

Thanks for the patch!

Later,
Blake.

Comment 12

[Boying Lu]

Attachment: updated patch removing extra trailing space

Comment 13

[Boying Lu]

Attachment: updated test case

Comment 14

[Boying Lu]

(In reply to comment #10)
> (From update of attachment 430504 [details] [diff] [review])
> >+++ b/mail/test/mozmill/account-setup-wizard/test-account-setup.js	Fri Mar 05 11:47:42 2010 +0800
> >@@ -0,0 +1,107 @@
> >+/* ***** BEGIN LICENSE BLOCK *****              
                                                       
> 
> There's a lot of trailing spaces on this line which should be removed.
> (And a couple of other lines have trailing spaces, too.)
> 
> >+ * The Initial Developer of the Original Code is
> >+ * Mozilla Messaging, Inc.
> 
> I've been told that this should be "The Mozilla Foundation.", since they own
> the copyrights on Mozilla Messaging code.  (Or I believe it could be you, if
> you didn't want to assign the copyright to Mozilla for some reason.)
> 
> >+ * Portions created by the Initial Developer are Copyright (C) 2009
> 
> This should probably be (C) 2010.
> 
Updated in the  new  test case 
                               
> >+function setupModule(module) {
> >+  let wh = collector.getModule('window-helpers');
> >+  wh.installInto(module);
> >+  
> >+  mc = wh.wait_for_existing_window("mail:3pane");
> >+}
> >+
> >+function test_account_wizard_setup()
> >+{
> >+  // Step1: open account manager window 
> >+  
> >+  plan_for_modal_dialog("mailnews:accountmanager",accountmanager_callback);
> >+
> >+  mc.waitThenClick(new elementslib.Elem(mc.menus.menu_Edit.menu_accountmgr));
> 
> Since this is a menu, I think you can just click it.
> Also, I really don't see an "accountmgr" menu item in my Edit menu.  (I'm on a
> Mac.) You could just get the element by id, and click it that way, though,
> because the id is the same no matter where the menu item has been put. 
> ("mc.click(mc.eid("menu_accountmgr"));" is what the code would look like.)
> 
> >+var accountmanager_callback = function am_callback(c)
> >+{
> >+   // Step2: click "add mail account" button
> >+   // a Mail account setup wizard open will be opened
> >+   plan_for_modal_dialog("mail:autoconfig",autoconfig_callback);
> >+   
> >+   var addMailAccountButton = c.eid("accountActionsAddMailAccount");
> >+
> >+   c.click(addMailAccountButton);
> >+   wait_for_modal_dialog("mail:autoconfig");
> >+   c.window.close();    
> >+}
> 
> So, you should be able to shortcut this step by just clicking the
> newMailAccountMenuItem right from the main window.
> 
I tried these, but it doesn't work.:(

> You'll also need to edit mail/test/mozmill/mozmilltests.list, and add
> account-setup-wizard somewhere in that list.

Add in the new test case

Comment 15

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #14)
> (In reply to comment #10)
> > >+function test_account_wizard_setup()
> > >+{
> > >+  // Step1: open account manager window 
> > >+  
> > >+  plan_for_modal_dialog("mailnews:accountmanager",accountmanager_callback);
> > >+
> > >+  mc.waitThenClick(new elementslib.Elem(mc.menus.menu_Edit.menu_accountmgr));
> > 
> > Since this is a menu, I think you can just click it.
> > Also, I really don't see an "accountmgr" menu item in my Edit menu.  (I'm on a
> > Mac.) You could just get the element by id, and click it that way, though,
> > because the id is the same no matter where the menu item has been put. 
> > ("mc.click(mc.eid("menu_accountmgr"));" is what the code would look like.)
> > 
> > >+var accountmanager_callback = function am_callback(c)
> > >+{
> > >+   // Step2: click "add mail account" button
> > >+   // a Mail account setup wizard open will be opened
> > >+   plan_for_modal_dialog("mail:autoconfig",autoconfig_callback);
> > >+   
> > >+   var addMailAccountButton = c.eid("accountActionsAddMailAccount");
> > >+
> > >+   c.click(addMailAccountButton);
> > >+   wait_for_modal_dialog("mail:autoconfig");
> > >+   c.window.close();    
> > >+}
> > 
> > So, you should be able to shortcut this step by just clicking the
> > newMailAccountMenuItem right from the main window.
> > 
> I tried these, but it doesn't work.:(

Okay, but:
mc.waitThenClick(new elementslib.Elem(mc.menus.menu_Edit.menu_accountmgr));
fails on the Mac, cause that menu item isn't in that menu, so we can't use that.

What do you think about using:
  plan_for_modal_dialog("mail:autoconfig",autoconfig_callback);
  mc.click(new elementslib.Elem(mc.menus.menu_File.menu_New.newMailAccountMenuItem));
  wait_for_modal_dialog("mailnews:autoconfig");
instead?  Cause that seems to work on the Mac, and should be in the same place on all the platforms we support.

Thanks,
Blake.

Comment 16

[Ben Bucksch (:BenB)]

+   // sleep 20s for timeout
+   c.sleep(20000);

That's going to be a pretty random test.
It will typically end up in guess config, but possibly be finished already, or possibly still in ISP fetch or similar when DNS takes long.

Comment 17

[Ben Bucksch (:BenB)]

I don't think the fix is good or sufficient either.

Why do need the this._stopped var?

Is it because if you stop the "fetch from DB" call, it is properly aborted, but then calls the error callback, which starts the guessing?
If that's the problem, then the canceling could set a certain exception (in the error callback) - in fact, fetchhttp.js already sets a UserCancelledException, and you can (and should) provide it explicitly in the cancel() call, i.e. in onStop(), do me._probeAbortable.cancel(new UserCancelledException()); And the error callback could then check for that with |if (e instanceof UserCancelledException) return;| before continuing to fall back to guess config. That's cleaner and we don't need the global/object var - which would need to be maintained in proper state.

Comment 18

[Ben Bucksch (:BenB)]

Attachment: Possible fix, v5

This shows what I described above.
I didn't test it yet. Please give it a try.

Comment 19

[Ben Bucksch (:BenB)]

Comment on attachment 433319 [details] [diff] [review]
Possible fix, v5

Tested and works for me (and doesn't work without the patch)

Comment 20

[Boying Lu]

Attachment: new test case based on Blake's comments

Comment 21

[Boying Lu]

(In reply to comment #18)
> Created an attachment (id=433319) [details]
> Possible fix, v5
> 
> This shows what I described above.
> I didn't test it yet. Please give it a try.

I tried the patch. But I see two issues:
1. The port field is empty and protocol field is set to none
2. After setting IMAP to 993/SSL and 465/SSL and click "Re-test Configuration" button, TB starts a infinite testing loop

Comment 22

[Boying Lu]

(In reply to comment #18)
> Created an attachment (id=433319) [details]
> Possible fix, v5
> 
> This shows what I described above.
> I didn't test it yet. Please give it a try.

Ben, I saw two issues after applying your patch ( your patch can't apply to the latest trunk code with gpath):
When this._probeAbortable points to FetchHTTP object.
1. The second issue in the comment 21.
   The root cause of it is this._probeAbortable.restart() is called in _startProbeIncoming() fucntion ( this function is called when I click "Re-test Configuration" button) but FetchHTTP object doesn't define restart() member function.

   I suggest to set this._probeAbortable = null after call cancel() in the onStop() function. So restart() won't be called and this._guessConfig() is called instead

2. When these events occur in following order:
   a. FetchHTTP fails to get information from server, an exception will be raised  at http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/fetchhttp.js#213

   b. Since the exception is not UserCancelledException, the code following 
      http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/emailWizard.js#460 will be executed

   c. User clicks "stop" button, but if this._probeAbortable points to FetchHTTP object at this point and before http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/emailWizard.js#473
 get executed, GuessAbortable object (i.e. this._probeAbortable) will never received the UserCanncelledException.
  In this case, the bug still occurs.

  I can't re-produce this every time, but it does occurs very often about one out of three times.
  So I think maintain a global variable may be better.

Comment 23

[Ben Bucksch (:BenB)]

> 1.... The root cause of it is this._probeAbortable.restart() is called in
> _startProbeIncoming() fucntion ( this function is called when I click "Re-test
> Configuration" button) but FetchHTTP object doesn't define restart() member
> function.

ARG! Whoever coded that ...
... had no concept of "API". (Also in other places)

_probeAbortable is an Abortable(), and that has only a cancel() function. He just threw in new functions as he felt like, completely ignoring the interface. Same is true for calling Hostdetector directly and ignoring the guessConfig API. That's just bad code.

I have fixed that in bug 549045, which includes my patch for this bug here.

Thanks for the very precise pointer!

2.
Are you saying that the user would click on "Stop" between line 459 (the fetchFromDB error callback) and line 473 (the call to guessConfig)? This code is syncronous. And as far as I know, this JS runs single-threaded with an event queue, so if the user would press "stop" at this point, the click would end up in the event queue, pulled when the guessConfig() started the network request and went idle, only then would the onStop() be executed and it would end up stopping the GuessAbortable - as expected.
In other words, I can't follow you.

Comment 24

[Boying Lu]

> 2.
> Are you saying that the user would click on "Stop" between line 459 (the
> fetchFromDB error callback) and line 473 (the call to guessConfig)? This code
> is syncronous. And as far as I know, this JS runs single-threaded with an event
> queue, so if the user would press "stop" at this point, the click would end up
> in the event queue, pulled when the guessConfig() started the network request
> and went idle, only then would the onStop() be executed and it would end up
> stopping the GuessAbortable - as expected.
> In other words, I can't follow you.
I think click "stop" button and exception raised from FetchHTTP are in separated thread. So there is a time window here.

 clicking "stop" button event occurs in the main thread (i.e. GUI thread) and
 the exception raised from the thread created from XMLHttpRequest(). the related code is at http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/fetchhttp.js#133. 
  the _resonse() in which the exception is raised is registered as request.onerror.

Comment 25

[Ben Bucksch (:BenB)]

> clicking "stop" button event occurs in the main thread (i.e. GUI thread) and
> the exception raised from the thread created from XMLHttpRequest().

Yes, the network request runs on its own thread. However, XMLHttpRequest then proxies back to the UI thread to call the error and success callbacks - precisely to avoid situations like this. (To my knowledge.)

So, if every third press on Stop doesn't work for you, we probably need another explanation. Can you describe exactly how to reproduce, i.e. what you do and what the effects are?

Comment 26

[Boying Lu]

I use the re-produce steps in comment 0 no special actions.
In step 4, after entering user name, you may need to wait for several seconds.
I can re-produce the bug on my OpenSolaris laptop. I haven't tried it on linux yet.

Can you update your patch? It can't be applied to the trunk code. Thx

Comment 27

[Boying Lu]

I guess I know the root cause now.
Yes. JS is single-threaded. The bug occurs when this._probeAborable is the object returned from fetchConfigFromISP()

We should also add following code 
function(e) // fetchConfigFromISP failed
{
+   if (e instanceof UserCancelledException)
+     return;
   ...
}

we need also change the line 
http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/fetchConfig.js#104
to 
errorCallback(e2);

Comment 28

[Ben Bucksch (:BenB)]

> We should also add following code 
> function(e) // fetchConfigFromISP failed

Ah, yes. My patch was created before that function was added. When applying, we need to add it there, too, of course.

> we need also change the line fetchConfig.js#104 to errorCallback(e2);

Very good catch, I'd never have thought of that. Thanks!

I'm impressed by your analytic skills.

Comment 29

[Ben Bucksch (:BenB)]

This is fixed by bug 549045.

Comment 30

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 433319 [details] [diff] [review]
Possible fix, v5

As mentioned, work on this will continue in bug 549045.

Comment 31

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 433522 [details] [diff] [review]
new test case based on Blake's comments

So, this test fails for me, I think because you're closing the window in the plan_for_modal_dialog callback.

I believe wait_for_modal_dialog returns the new dialog, so you could just say something like:
let ac = wait_for_modal_dialog("mail:autoconfig");
and then copy the code from autoconfig_callback into the method (replacing references to "c" with references to "ac"), and it should all still work.

If you're up for it, please continue working on this test, but update it to run against the code in bug 549045.

Thanks,
Blake.

Comment 32

[Boying Lu]

Sure, I'll continue work on this test case.

Comment 33

[Boying Lu]

Attachment: add a new test case

Add a new test case for this bug.

Comment 34

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 453299 [details] [diff] [review]
add a new test case

on file: mail/test/mozmill/account/test-mail-account-setup-wizard.js line 203
>   awc.sleep(20000);

I think we can do something better here.  Perhaps wait for a button to be
enabled, like on line 137 or line 222?

Other than that, I think I like it, but I'm going to ask you to submit a new
patch which has that change, and the fix in it as well, so that I can test
everything together.  Having said that, I have no doubts that the next patch will get an r+ from me, since this one is very close.

Thanks,
Blake.

Comment 35

[Boying Lu]

Attachment: replace sleep() with awc.waitForVal()

Comment 36

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 455413 [details] [diff] [review]
replace sleep() with awc.waitForVal()

I like it!  My only request would be to figure out if we can mark a test as KNOWN_FAILURE or something, because we know that this test will continue to fail until we get a patch that fixes the bug.

Thanks,
Blake.

Comment 37

[Boying Lu]

(In reply to comment #36)
> (From update of attachment 455413 [details] [diff] [review])
> I like it!  My only request would be to figure out if we can mark a test as
> KNOWN_FAILURE or something, because we know that this test will continue to
> fail until we get a patch that fixes the bug.
> 
> Thanks,
> Blake.

How to mark the test case as "KNOWN_FAILURE"?  I think the test case will be passed when the patch of the bug 549045 is landed.

Comment 38

[Blake Winton (:bwinton) (:☕️)]

I'm not sure.  And while it will pass when bug 549045 lands, that looks like it might take longer than we originally thought.

Comment 39

[Ben Bucksch (:BenB)]

> How to mark the test case as "KNOWN_FAILURE"?

Just comment the function call out, with a comment why and when to enable.

Comment 40

[Ben Bucksch (:BenB)]

(or, maybe better yet, wait with commit)