1211597 - Incorrect code in verifyConfig.js / verifyConfig

Status: RESOLVED INCOMPLETE

(MailNews Core :: Account Manager, defect)

Description

[Mike Kaply [:mkaply]]

The code here:

http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/verifyConfig.js#116

Is incorrect.

There is an errorCallback with (e), but we're not in a try catch at this point.

So if we get to the end of this function, there will be an error about e not being defined.

Checked in with bug 1155491

Comment 1

[:aceman]

112   catch (e) {
113     gEmailWizardLogger.error("ERROR: verify logon shouldn't have failed");
114   }
115   // Avoid pref pollution, clear out server prefs.
116   MailServices.accounts.removeIncomingServer(inServer, true);
117   errorCallback(e);
118 }

From the code above this, it looks like these two lines should also be inside the catch block?

Comment 2

[Mike Kaply [:mkaply]]

> From the code above this, it looks like these two lines should also be inside the catch block?

That's my thought.

I'll put together a patch and have rkent review.

Comment 3

[Kent James (:rkent)]

It would be great if you could test and explain why the current code works, as I am not aware of any reported symptoms of this problem.

Comment 4

[Mike Kaply [:mkaply]]

(In reply to Kent James (:rkent) from comment #3)
> It would be great if you could test and explain why the current code works,
> as I am not aware of any reported symptoms of this problem.

It won't cause a problem in todays code unless something fails. I was porting this code to another mail client and I had a bug in my code which exposed this code.

Nothing will break today because this is the tail end of this function. You'll just see a message on the console and wonder why it is there.

It's just a correctness issue.

Comment 5

[Magnus Melin [:mkmelin]]

Yes currently the only time you'd get there is when e is defined.

Comment 6

[Mike Kaply [:mkaply]]

This isn't worth fixing. I have no idea why I saw the problem in Postbox code.