Last Comment Bug 758237 - Account Provisioner tab should close and respawn AP dialog if provider XML is corrupt or returns error
: Account Provisioner tab should close and respawn AP dialog if provider XML is...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Account Manager (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: Thunderbird 15.0
Assigned To: Mike Conley (:mconley)
:
:
Mentors:
Depends on: 757823
Blocks: AccountProvisioner
  Show dependency treegraph
 
Reported: 2012-05-24 08:56 PDT by Mike Conley (:mconley)
Modified: 2012-05-28 14:07 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
+
fixed


Attachments
Patch v1 (12.30 KB, patch)
2012-05-24 09:01 PDT, Mike Conley (:mconley)
bwinton: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
Patch v2 (for comm-beta and comm-aurora) - carrying forward ui-r+ / r+ from bwinton. (12.58 KB, patch)
2012-05-25 11:11 PDT, Mike Conley (:mconley)
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review
Patch v2 (for comm-central) - carrying forward ui-r+ / r+ from bwinton (12.55 KB, patch)
2012-05-25 11:17 PDT, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review

Description Mike Conley (:mconley) 2012-05-24 08:56:52 PDT
STR:

1)  Go to File > New > Get a New Mail Account
2)  Make sure Hover.com is selected, and fill in a name
3)  Choose an address and fill in the form
4)  If there is no credit card field, press "Continue", and the field should reveal itself. Fill the credit card field with a valid credit card number  (it will not be charged) and press "Continue".  
5)  Choose to "Cancel this order"

What happens?

A big ugly XML page is displayed to the user.

What's expected?

The tab should close, and we should go back to the Account Provisioner dialog.
Comment 1 Mike Conley (:mconley) 2012-05-24 09:01:09 PDT
Created attachment 626835 [details] [diff] [review]
Patch v1

If the provider returns corrupt XML, or XML that we cannot construct an account out of, this patch closes the tab and respawns the Account Provisioner dialog.
Comment 2 Blake Winton (:bwinton) (:☕️) 2012-05-24 17:28:12 PDT
patching file mail/test/mozmill/newmailaccount/test-newmailaccount.js
Hunk #2 FAILED at 618
Hunk #3 FAILED at 911
Hunk #4 FAILED at 981
3 out of 4 hunks FAILED -- saving rejects to file mail/test/mozmill/newmailaccount/test-newmailaccount.js.rej
patch failed, unable to continue (try -v)

:(

Hand-patching now; review coming soon.
Comment 3 Mike Conley (:mconley) 2012-05-24 17:29:25 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #2)
> patching file mail/test/mozmill/newmailaccount/test-newmailaccount.js
> Hunk #2 FAILED at 618
> Hunk #3 FAILED at 911
> Hunk #4 FAILED at 981
> 3 out of 4 hunks FAILED -- saving rejects to file
> mail/test/mozmill/newmailaccount/test-newmailaccount.js.rej
> patch failed, unable to continue (try -v)
> 
> :(
> 
> Hand-patching now; review coming soon.

Yikes - I should have mentioned, this patch relies on the patch in bug 757823. Should apply cleanly on top of it (you're conflict was likely in the tests.)
Comment 4 Blake Winton (:bwinton) (:☕️) 2012-05-24 18:52:58 PDT
Comment on attachment 626835 [details] [diff] [review]
Patch v1

Okay, I tried the steps to reproduce, and it looks much better now.  ui-r=me.

>+++ b/mail/components/newmailaccount/content/uriListener.js
>@@ -196,53 +196,56 @@ TracingListener.prototype = {
>       let accountConfig = accountCreationFuncs.readFromXML(xml);
>       accountCreationFuncs.replaceVariables(accountConfig,
>+                                            this.params.realName,
>+                                            this.params.email);
>+      account = accountCreationFuncs.createAccountInBackend(accountConfig);
>+      success = true;

I wonder if we should also check some properties on the account, or if we know that the createAccountInBackend will always throw if it can't create the account?

>     } catch (e) {
>       // Something went wrong.  Right now, we just dump the problem out
>       // to the Error Console.  We should really do something smarter and
>       // more user-facing, because if - for example - a provider passes
>       // some bogus XML, this routine silently fails.

Heh.  Prescient comment.  Now that we're doing something more, perhaps we can make the comment more appropriate?

>+++ b/mail/test/mozmill/newmailaccount/html/configError.xml
>@@ -0,0 +1,6 @@
>+<clientConfig version="1.1">
>+  <emailProvider id="%DOMAIN%"/>
>+  <error code="USER_CANCEL">
>+    You have cancelled your order.
>+  </error>
>+</clientConfig>

Should we test the other error codes, or will they all be handled the same?  (I'm pretty sure they'll all be handled the same for now.  Maybe fix the handling in v2?)

So r=me with those questions answered/nits fixed.

Thanks,
Blake.
Comment 5 Mike Conley (:mconley) 2012-05-25 07:04:44 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #4)
> Comment on attachment 626835 [details] [diff] [review]
> Patch v1
> 
> Okay, I tried the steps to reproduce, and it looks much better now.  ui-r=me.

Cool, thanks.

> 
> >+++ b/mail/components/newmailaccount/content/uriListener.js
> >@@ -196,53 +196,56 @@ TracingListener.prototype = {
> >       let accountConfig = accountCreationFuncs.readFromXML(xml);
> >       accountCreationFuncs.replaceVariables(accountConfig,
> >+                                            this.params.realName,
> >+                                            this.params.email);
> >+      account = accountCreationFuncs.createAccountInBackend(accountConfig);
> >+      success = true;
> 
> I wonder if we should also check some properties on the account, or if we
> know that the createAccountInBackend will always throw if it can't create
> the account?

We do have a tool for checking configuration (http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/verifyConfig.js#65). This function asynchronously tries to log-in with the credentials given back with the XML to check their validity.

We could use that, but what should be the behaviour if the transaction completed, and the XML that was given back was valid, but the account cannot be logged into? What should TB do in this case?

> 
> >     } catch (e) {
> >       // Something went wrong.  Right now, we just dump the problem out
> >       // to the Error Console.  We should really do something smarter and
> >       // more user-facing, because if - for example - a provider passes
> >       // some bogus XML, this routine silently fails.
> 
> Heh.  Prescient comment.  Now that we're doing something more, perhaps we
> can make the comment more appropriate?

Agreed.

> 
> >+++ b/mail/test/mozmill/newmailaccount/html/configError.xml
> >@@ -0,0 +1,6 @@
> >+<clientConfig version="1.1">
> >+  <emailProvider id="%DOMAIN%"/>
> >+  <error code="USER_CANCEL">
> >+    You have cancelled your order.
> >+  </error>
> >+</clientConfig>
> 
> Should we test the other error codes, or will they all be handled the same? 
> (I'm pretty sure they'll all be handled the same for now.  Maybe fix the
> handling in v2?)

If the XML cannot be turned into incoming/outgoing servers, we close the tab and re-open the provisioner. Not the greatest situation, but we're past string freeze. :/

I'll file a bug to fix that handling.

> 
> So r=me with those questions answered/nits fixed.

Thanks for the review!
Comment 6 Mike Conley (:mconley) 2012-05-25 07:24:51 PDT
So the createAccountInBackend will throw if it attempts to reach for something that doesn't exist (like config.incoming.port, if incoming doesn't exist).

However, if incoming is provided but port is missing, I'm not sure what kind of protections we have against that sort of thing.

David, do you know?
Comment 7 David :Bienvenu 2012-05-25 07:43:55 PDT
(In reply to Mike Conley (:mconley) from comment #6)
> So the createAccountInBackend will throw if it attempts to reach for
> something that doesn't exist (like config.incoming.port, if incoming doesn't
> exist).
> 
> However, if incoming is provided but port is missing, I'm not sure what kind
> of protections we have against that sort of thing.
> 
> David, do you know?

I don't. I believe the mail backend code will treat a missing port as meaning to use the default port for the protocol and connection type. But if we're really worried about it, we should verify it. I'm also not sure what the account autoconfig code will do with a missing port (e.g., will it create an incoming server in the backend?)
Comment 8 Mike Conley (:mconley) 2012-05-25 10:06:43 PDT
Ok, so from what I can tell, readFromXML does the validation that bwinton is talking about, so I think we're covered. BenB can correct me if I'm wrong.
Comment 9 Mike Conley (:mconley) 2012-05-25 11:11:00 PDT
Created attachment 627297 [details] [diff] [review]
Patch v2 (for comm-beta and comm-aurora) - carrying forward ui-r+ / r+ from bwinton.

Thanks Blake - made the changes you suggested, we're safe via readFromXML, and I filed bug 758700.

I had to backport this for beta, so here's the beta patch.
Comment 10 Mike Conley (:mconley) 2012-05-25 11:16:11 PDT
Comment on attachment 627297 [details] [diff] [review]
Patch v2 (for comm-beta and comm-aurora) - carrying forward ui-r+ / r+ from bwinton.

Looks like backport is required for both beta and aurora.
Comment 11 Mike Conley (:mconley) 2012-05-25 11:17:23 PDT
Created attachment 627299 [details] [diff] [review]
Patch v2 (for comm-central) - carrying forward ui-r+ / r+ from bwinton
Comment 12 Mike Conley (:mconley) 2012-05-28 09:52:40 PDT
comm-central: https://hg.mozilla.org/comm-central/rev/539dc131baf1
Comment 13 Mike Conley (:mconley) 2012-05-28 14:00:44 PDT
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/f2e688291060
Comment 14 Mike Conley (:mconley) 2012-05-28 14:07:16 PDT
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/f875a2e01339

Note You need to log in before you can comment on or make changes to this bug.