202045 - order of the smtp servers in the advanced outgoing server settings changes from time to time

Status: RESOLVED WORKSFORME

(SeaMonkey :: MailNews: Account Configuration, defect)

Description

[Johannes Walther]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; de-DE; rv:1.4b) Gecko/20030409
Build Identifier: Mozilla/5.0 (Windows; U; Win98; de-DE; rv:1.4b) Gecko/20030409

I've multiple smtp entries in my account manager. Originally they where in the
order I created them (the same order the corresponding accounts are created and
listed in the folders panel).
But now they changed their orders and still changing them from time to time.

It's no really big thing but if they where in the same order as the
corresponding accounts or at least in a stable order would be very helpful.
And I see this as a indicator that something must be wrong.

Reproducible: Sometimes

Steps to Reproduce:

Comment 1

[Christian Eyrich]

I can confirm this. One part is a regression from fix of bug 96207 and one part
was always there.

The "resort" happens in two cases.
1. When a mail gets send, an list entry with the sending server gets created. If
you then open the account manager, the rest of the servers will be added to the
list - behind the entry created while the sending process.
2. If a smtp server gets deleted, a hole in the smtp server list emerges (e.g.
smtp1, smtp3). If you then create a new one, it becomes smtp2 but will be
appended to the list, so we have smtp1, smtp3, smtp2.

Neither case 1 nor case 2 is a functional problem. But in case 1 this is an
obvious error and should not occur.

Three ways are possible for this:
1. Resort the list before display it.
2. Clear the list before loading the servers from the prefs.js into it.
3. Loading the servers into the list at startup of Mailnews.

1. is quite expensive. 2. can be dangerous when done while sending a mail. 3. Is
easy and no big thing. So I'd vote for 3.

Just add a call to loadSmtpServers() to nsSmtpService::nsSmtpService().

In case 2 it's not clear if it's how it should be or not. The server list will
loose it's ascending order in meaning of the server names. But it saves the
order the servers are created and to this it doesn't happen very often. So I
don't think we'll change this.

Comment 2

[Christian Eyrich]

Attachment: proposed patch

Patch for loading the smtp server list right in the constructor of
nsSmtpService.

That should make all other calls to loadSmtpServers() unnecessary. But I think
for security reasons and since they aren't very costly, we can keep them.

Comment 3

[Johannes Walther]

Hey, you have a patch since nearly a month but it doesn't fix the problem.
The order of the servers still changes from time to time.

Comment 4

[Christian Eyrich]

Johannes, the patch has not been checked in and thus can't take effect. Patches
need a review and super-review before checking in.

I tried to get some attention in the past but failed. Now I'm directly trying to
get a review.

Comment 5

[(not reading, please use [e-mail] instead)]

I want to double check that this won't have any side effects.

eyeballing it, I don't think it will (for migration, account creation, etc).

it looks like alecf did it this way originally, so that we'd do the work lazily.

but it looks like most (if not all) callers who do getservice on this service
end up calling a method that calls loadSmtpServers() right away anyways.

if we do take this for 1.4 final, we should clean it up in 1.5a

1) remove mSmtpServersLoaded
2) make loadSmtpServers() return void
3) remove the callers to loadSmtpServers()
4) clean up nsSmtpService::GetSmtpServers()

Comment 6

[Jose Fandos]

Does this apply to Thunderbird too? 

Comment 7

[Mike Cowperthwaite]

This bug no longer applies, as the UI for the SMTP server selection has changed radically.

Comment 8

[Simon Paquet [:sipaq]]

Comment on attachment 120859 [details] [diff] [review]
proposed patch

Since this bug has been resolved as WFM, I'm removing the
review request.