BuildID: 2000060720 Attempting to create two IMAP accounts for the same host, but with a different port, fails silently. Reproducible: Always I have an IMAP account (localhost port 143) set up as 'Work'. I use ssh to port-forward my home account's port 143 to 1143 on localhost. If I try to add another 'Home' IMAP account on localhost, the account is not added (perhaps because the host and port are identical?). If I add the account as 'orion', it's added properly and I can change the port from the advanced options. However, access to orion:1143 via ssh forwarding does not work (it requires 'localhost:1143'). Actual Results: A new entry is placed in prefs.js, but it's not shown in the sidebar and cannot be edited. Tried to add 'orion' and then change "mail.server.server4.hostname" in prefs.js to "localhost"; the 'Home' account then disappeared from the sidebar, although it was still shown in prefs.js. Relevant lines from prefs.js: user_pref("mail.server.server1.directory", "/home/peterj/.mozilla/mozProfile/ImapMail/server1"); user_pref("mail.server.server1.hostname", "localhost"); user_pref("mail.server.server1.name", "Work"); user_pref("mail.server.server1.type", "imap"); user_pref("mail.server.server1.userName", "peterj"); user_pref("mail.server.server4.directory", "/home/peterj/.mozilla/mozProfile/ImapMail/orion"); user_pref("mail.server.server4.hostname", "orion"); user_pref("mail.server.server4.max_cached_connections", 5); user_pref("mail.server.server4.name", "Home"); user_pref("mail.server.server4.port", 1143); user_pref("mail.server.server4.type", "imap"); user_pref("mail.server.server4.userName", "peterj");

known issue, not sure if we'll ever fix it.

I know this is a hack, but one workaround you can do is refer to localhost by it's real hostname or something.. i.e. set up the server as localhost and myhost:1143

oh, and we will be able to fix this if our architecture ever allows us to eliminate FindServer(), but we're a long way off from that.

Tried that hack, but because I've already got 'Work' set up as localhost, I can't change it to myhost. ssh requires localhost:1143, not orion:1143. Could just re-generate the accounts, I guess.

massive reassign of account manager bugs -> sspitzer please feel free to put me back on the CC if you have any questions/comments

passing the buck to racham.

This is a big deal for people who get multiple email accounts forwarded to the same server. Nominating for mozilla 0.9.

Also for NEWS it is very useful to have two different accounts on the same server because in selecting newsgroups in different categories [e.g. like 'privat' and 'business'].

This also fails for multiple accounts on the same server on different ports. Services are provided on a host:port tuple, not a host. Any architecture that breaks this is fundamentally broken. This was supposedly fixed as bug 13833 many moons ago - why is it broken _again_?

I'm encountering problems with multiple accounts on the same server with different logins. I have ten IMAP mailboxes; one is home stuff, one is Mozilla stuff. We really should be able to support that - it's not an uncommon configuration. Gerv

*** Bug 106916 has been marked as a duplicate of this bug. ***

*** Bug 122808 has been marked as a duplicate of this bug. ***

*** Bug 116949 has been marked as a duplicate of this bug. ***

*** Bug 159404 has been marked as a duplicate of this bug. ***

*** Bug 133228 has been marked as a duplicate of this bug. ***

*** Bug 136933 has been marked as a duplicate of this bug. ***

A tip for Peter Janes and anybody else having this problem: you can set up your ssh port forwarding using the name "localhost" or 127.0.0.1 or whatever you want. The only thing that matters in this setup is the hostnames given to mozilla. I have just added aliaes (brokenimap1 brokenimap2 etc) to my /etc/hosts line for "127.0.0.1 localhost". Having done that, I can assign multiple accounts easily. Moz really needs to match account conflicts on a "proto://user@host:port" not just "user@host" or "proto://user@host" basis. The current behavior is badly broken. Perhaps a relnote mentioning the problem and workaround would be appropriate

A tip for Peter Janes and anybody else having this problem: you can set up your ssh port forwarding using the name "localhost" or 127.0.0.1 or whatever you want. The only thing that matters in this setup is the hostnames given to mozilla. I have just added aliaes (brokenimap1 brokenimap2 etc) to my /etc/hosts line for "127.0.0.1 localhost". Having done that, I can assign multiple accounts easily. Moz really needs to match account conflicts on a "proto://user@host:port" not just "user@host" or "proto://user@host" basis. The current behavior is badly broken. Perhaps a relnote mentioning the problem and workaround would be appropriate

*** Bug 119900 has been marked as a duplicate of this bug. ***

A related problem was fixed in bug 26768, "Two POP accounts with same server shouldn't use same password". See also bug 17289, "[4.x MIGRATION] collision if the user has two imap accounts on the same server, but different ports". I think that makes this bug 4xp.

*** Bug 173268 has been marked as a duplicate of this bug. ***

*** Bug 184569 has been marked as a duplicate of this bug. ***

*** Bug 188406 has been marked as a duplicate of this bug. ***

Hi! I know this is a known problem. But why can I have multiple Accounts on the same imap server with Mozilla 1.21 on Windows, while on Linux, they are shown as blank envelopes without the name attached to them? In fact, the account name is still in the settings, but the mail client becomes unuseable without the account names if you have more than one account to manage (e.g. we have accounts for different teams on the server, but all use the same mail client on the same machine). Thank You, CU, Lars.

(My comment #24) Sorry, it's not the Linux client, but the Mac OS 8/9 version of Mozilla 1.21 that has problems showing account names.

My two cents, as I also have a need for a fix to this bug (I have a regular email address, and I also have emails forwarded from one of my domains). It would make many people happy if it were easy to alter the "From" field while sending an email. I know that it's possible to alter the "Reply-to" field, but I've heard reports that some email clients ignore the "reply-to" field. Also, there are situations where it's necessary to give the impression that an email message is coming "From" a certain domain. Possibly another option could be made available in the "To/Cc/Bcc/etc" dropdown while composing an email. That would be a "From" field. Possibly the list of possible addresses accessible to the "From" field could be specified in the "Account Settings" panel. That way it makes it more difficult to accidentally set the "From" field to an unexpected address. If I could easily set the "From" field, my problem would be solved. No need to modify "/etc/hosts", or eliminate "FindServer()", or to provide multiple identical accounts. I think that many people would be happy with such a solution. Thanks, Derek Ross.

I'm not sure that really solves the problem. Unfortunately, I and I expect a number of others need to access multiple IMAP accounts on the same server with the same username, but on different ports. For example, I have an SSH tunnel that forwards requests for a port on my firewall to the IMAP server at work behind their firewall. I also have a local IMAP server on my firewall that is used to collect mail for several POP3 accounts using fetchmail. As you can see, your proposed solution would not fix my problem, nor to my mind the central problem behind this bug: you still couldn't have multiple accounts on the same host+username but on different ports. Craig Ringer

I'm sure there is another bug on making the From: field editable. Gerv

I am currently suffering from this bug, and using the "multiple aliases" workaround, so one of my accounts is on "mail.foo.com", another on "web.foo.com" (which happens to be the same machine) and so on. However, because I use IMAP over SSL, this has the irritating effect that I have to dismiss a "certificate mismatch" dialog every time I start mail, because the mail cert is for mail.foo.com, not web.foo.com. This also means I don't have the protection that SSL is supposed to give me, because I get the aforementioned "someone might be doing a man-in-the-middle attack" dialog every time, and so ignore it. I've just tried creating a new profile with two accounts on the same server and, sure enough, it still doesn't work. Both accounts fail to collect messages. sspitzer, or anyone: is this bug a deep architectural problem in mail, or quite easy to fix? Gerv

To continue: a bit of searching shows about six other bugs filed, all of which probably have the same root cause - Mozilla does not distinguish carefully enough between services which have some configuration data in common, be it several news servers on the same machine, or two SSL-enabled services (SMTP and IMAP) on the same machine, or whatever. Gerv

*** Bug 131764 has been marked as a duplicate of this bug. ***

*** Bug 201016 has been marked as a duplicate of this bug. ***

I have read somewhere that there is a workaround for this by manually editing the prefs.js file to create more "from" addresses. However, I cannot find this anymore. Can anybody help?

My situation is similar and slightly different. I need to be able to access multiple IMAP accounts on the same server, same port, same username, same passwords, different email addresses. Why ? I am accessing a MS exchange server and I am controlling multiple accounts on it, and some accounts don't have their own 'NT user' attached to them - I am using a single 'NT user' with several email accounts. These email accounts are NOT aliases. Alias email works OK, since thats a server side function, but these other accounts I would prefer not to be aliased. Outlook works fine with this setup.

mass re-assign.

I fell foul of this bug too. My needs are that I have a single catch-all pop account that I want to service several family members. In order to get different email addresses in the From: field I need to set up multiple accounts to the same pop server. Mozilla flatly refuses to do this :( One solution I thought about was to have the option to creat a new mail account and set the outgoing server to *none*. This means Mozilla will never try to collect mail from this account but does allow sending mail from this account. Another solution would be to simply allow Mozilla to have several accounts to the same pop server. Obviously you would need to prevent mozilla from collecting mail redundently.

Comment on attachment 113554 [details] A possible solution: modifiable "From" field in compose. i'll minus this. it's confusing since it conflicts with the other from field also visible in the picture. as to the last comment if you only need return addresses and not inboxes, create news server accounts for each name.

I put a vote on this bug, but what I really think would keep users happy is if they could manage email aliases for an account. (i.e. use different names/email-addresses for an account).

Nobody denies that aliases would be great, and it has been mentioned many times. But aliases should be viewed as a distinctly different issue then this. I personally don't care about aliases, but this bug causes me all sort of grief. A lot of people have the need to access multiple, *different*, accounts in this manner.

*** Bug 217994 has been marked as a duplicate of this bug. ***

I think it's useless to check the account, perhaps it's wrong to set the same account and server. Tools can hint him that he is 'probably' wrong but shouldn't deny this setting, at least it should take a way to set it.

*** Bug 223774 has been marked as a duplicate of this bug. ***

The solution I propose here is not for the bug as described here, but for bug 122808 that is designated as duplicate of this one. I have a situation where I have set up an account for each of my e-mail aliases (@seghers.net) under which I post. Only one of the accounts actually fetches the mails and distributes them to the inboxes of the other accounts. The others have an invalid user specified, so there is no conflict from TB's point of view, and they also are set up to never request mail themselves. The From e-mail address for each account is different, so when I reply from that account's inbox, my From field is the same as to which the original mail was addressed. The only glitch is that when I want to get new mails, I have to first click the receiving account and then ask for new mail. If I stay inside the other's account inbox (or other folder) I am asked the password for the invalid user, which is not what I want ofcourse. So, to make everything work seamlessly, I think it should be possible to specify the same server, user and port for different accounts, but be given the option to designate one account as the master, and the others as slaves. Requesting mail for the slave accounts would simply pass the request to the master account, which distributes the incoming mail over the slave accounts. Should be a relatively easy thing to do, no?

*** Bug 234749 has been marked as a duplicate of this bug. ***

Here is the first part of a possible solution. This patch adds the possiblity of using a given url to find the server. Included is the capability to match on the port number also. A value of "0" is used for the port if any port is acceptable. I have discussed this with bienvenu briefly. The second half of this patch would be to convert the users of FindServer over to using FindServerByUrl Please comment. Kevin

Kevin, that looks reasonable so far...I'll review the whole patch when it's done.

After a complete new-installation and -configuration it works now well. So I can´t say what the mistake was, because the configuration before was exactly the same.

Full patch

same patch without whitespace changes (there were lots of tabs in nsLocalUtils.cpp and nsLocalMailFolder.cpp)

thx, Kevin, looks good in general. Can you remove the commented out code? +/* // extract the user name and host name information... nsresult rv = mailnewsUrl->GetAsciiHost(hostName); if (NS_FAILED(rv)) return rv; @@ -2642,6 +2645,7 @@ if (!userPass.IsEmpty()) NS_UnescapeURL(userPass); // hopefully we're not unescaping ':' or nasty control chars +*/ Also, why do you call SetScheme on the url's? I would have thought the scheme would get parsed out by the uri parser...

(In reply to comment #50) > thx, Kevin, looks good in general. > > Can you remove the commented out code? > > +/* > // extract the user name and host name information... > nsresult rv = mailnewsUrl->GetAsciiHost(hostName); > if (NS_FAILED(rv)) return rv; > @@ -2642,6 +2645,7 @@ > > if (!userPass.IsEmpty()) > NS_UnescapeURL(userPass); // hopefully we're not unescaping ':' or nasty > control chars > +*/ No problem; missed one when I was cleaning up after testing. I will generate a patch for that this weekend. I'm away from my tree 'til then. > > > Also, why do you call SetScheme on the url's? I would have thought the scheme > would get parsed out by the uri parser... The uri comes in with the scheme set to 'mailbox'; since that could be either a set of local folders or a pop account (or a movemail account on unix) you have to replace try each scheme. It seemed easiest to just use SetScheme to switch this instead of doing string wrangling. Since the url I'm working with is built from the input string, I'm not changing the original uri. I guess, that I could move this check into "InternalFindServerByUrl" and do the testing there, but it might be a little messy (i.e. I need to think about it and see if it makes sense) Kevin Kevin

David: after this goes in someone will only be able to take advantage if they know how to manage the prefs.js or are willing to create an account with a bogus host and then edit it to the proper one in prefs.js. Do we want to add UI for this capability, or is this to be left as an advanced feature only? Another thought: Should we rework FindServer to create a url from the input and then pass it to FindServerByURL to eliminate code duplication? Kevin

Kevin, re the first question, is this because the new account code prevents you from creating an account with the same user name and host name as another account? If so, I think that code should be changed to prevent you from creating an account with the same user name, host name, and port - which would mean also prompting for the port in the new account wizard...hmm. But, can't the user go in and edit the server name through the account settings UI, by using a bogus server name first, w/o going into prefs.js? Re the second question, if it's just as reliable, and less code, it's worth a try. But I'm confused - doesn't FindServer just call InternalFindServer? It's hard to tell from the diffs.

(In reply to comment #53) > Kevin, re the first question, is this because the new account code prevents you > from creating an account with the same user name and host name as another > account? Yes, the FE UI calls FindServer which only allows input of username,host,proto >If so, I think that code should be changed to prevent you from creating > an account with the same user name, host name, and port - which would mean also > prompting for the port in the new account wizard...hmm. But, can't the user go > in and edit the server name through the account settings UI, by using a bogus > server name first, w/o going into prefs.js? I'll have to take another look, but I think if you change the hostname in the account settings, it might check for a duplicate based only on (username,host,proto). That is where you would get an error. I need to verify that behavior > > Re the second question, if it's just as reliable, and less code, it's worth a > try. But I'm confused - doesn't FindServer just call InternalFindServer? It's > hard to tell from the diffs. I was thinking of having FindServer make a url out of its input and send that to InternalFindServerByUrl and then eliminating InternalFindServer and findServer (these are what I copied basically to make InternalFindServerByUrl and findServerUrl). Right now with the submitted patch, we basically have duplication of tasks for InternalFindServer/InternalFindServerByUrl and findServer/findServerUrl. Kevin

Removed the commented out code per David's comment. I am not obsoleting the diff -w version, but it has not been updated to show removal of what I had commented out. David, I rethought about converting FindServer over to calling InternalFindServerUrl. There are some cases (blank userpass, blank host) that are used that I need to investigate prior to converting (Have to see how the URL code responds to those inputs) and since beta is close, thought it would be better to get this in rather than push. If we get this in for beta, we can at least allow advanced users to use this functionality in 1.7 and TB 0.6 Requesting r and sr. Do I have time for appr1.7b? Kevin

Comment on attachment 143649 [details] [diff] [review] Full patch for backend changes Removing request on obsolete patch

Comment on attachment 144039 [details] [diff] [review] Cleaned up version Removing review requests since I have more to do. bienvenu: In working on bug 238583 (the UI stuff) I am finding that we don't import port info from other programs. If I modify the CreateIncomingServer function to add the port number, I need to do something with the import code. What is your opinion, fix the import code at the same time or rename the current "CreateIncomingServer" to CreateIncomingServerLegacy" and change the import code to point at it and then do a new bug to update the import code? I can do it all here, but it is going to turn into a big bug fix with lots of changes to review. Kevin

*** Bug 249398 has been marked as a duplicate of this bug. ***

Review patch for adding FindServerByURL patch for checkin has some tabs and whitespace fixes. I'll attach later. Should be good for 1.8a I have put on the back burner the problem of importing ports from other programs. I got around the problem in the JS by setting the port and secure properties on the incomingServer object that is exposed to JS. Have at for review. Kevin

Comment on attachment 155024 [details] [diff] [review] Review version I think this is ready to review

Comment on attachment 155024 [details] [diff] [review] Review version thx for doing this! + (!nsCRT::strcmp((hostname.get()?hostname.get():""),m_lastFindServerHostName.get ())) && + (!nsCRT::strcmp((username.get()?username.get():""),m_lastFindServerUserName.get ())) && You should be able to just use m_lastFindServerHostName.Equals(hostName), i.e., use .Equals for all these. + *aResult = serverInfo.server; + NS_ADDREF(*aResult); this can just be NS_ADDREF(*aResult = serverInfo.server); which generates less code. + // treat "" as a wild card, so if the caller passed in "" for the desired attribute + // treat it as a match + PRBool checkType = PL_strcmp(entry->type, ""); + PRBool checkHostname = PL_strcmp(entry->hostname,""); + PRBool checkUsername = PL_strcmp(entry->username,""); + PRBool checkPort = (entry->port != 0); + if ((!checkType || (PL_strcmp(entry->type, thisType)==0)) && + (!checkHostname || (PL_strcasecmp(entry->hostname, thisHostname)==0)) && + (!checkPort || (entry->port == thisPort)) && + (!checkUsername || (PL_strcmp(entry->username, thisUsername)==0))) + { + entry->server = server; + return PR_FALSE; // stop on first find + } don't need temp vars, or PL_strcmp so this code can just be: if (!*entry->type || strcmp(entry->type, thisType) etc. could you just remove this commented out code? +// nsUnescape(username); +// nsUnescape(hostname); maybe add a comment that InternalFindServerByUrl will do the unescaping?

(In reply to comment #61) > thx for doing this! You're welcome! :) >+ >(!nsCRT::strcmp((hostname.get()?hostname.get():""),m_lastFindServerHostName.get >())) && >+ >(!nsCRT::strcmp((username.get()?username.get():""),m_lastFindServerUserName.get >())) && > >You should be able to just use m_lastFindServerHostName.Equals(hostName), i.e., >use .Equals for all these. Changed to use .Equals >+ *aResult = serverInfo.server; >+ NS_ADDREF(*aResult); > >this can just be NS_ADDREF(*aResult = serverInfo.server); which generates less >code. These instances, plus some others in the same file changed to single line form. >+ // treat "" as a wild card, so if the caller passed in "" for the desired >attribute >+ // treat it as a match >+ PRBool checkType = PL_strcmp(entry->type, ""); >+ PRBool checkHostname = PL_strcmp(entry->hostname,""); >+ PRBool checkUsername = PL_strcmp(entry->username,""); >+ PRBool checkPort = (entry->port != 0); >+ if ((!checkType || (PL_strcmp(entry->type, thisType)==0)) && >+ (!checkHostname || (PL_strcasecmp(entry->hostname, thisHostname)==0)) && >+ (!checkPort || (entry->port == thisPort)) && >+ (!checkUsername || (PL_strcmp(entry->username, thisUsername)==0))) >+ { >+ entry->server = server; >+ return PR_FALSE; // stop on first find >+ } > >don't need temp vars, or PL_strcmp so this code can just be: > >if (!*entry->type || strcmp(entry->type, thisType) etc. Changed per your suggestion >could you just remove this commented out code? > >+// nsUnescape(username); >+// nsUnescape(hostname); Code removed >maybe add a comment that >InternalFindServerByUrl will do the unescaping? Comment added just above call to FindServerByUrl() in that procedure patch is a diff -w -u8. There are a bunch of tabs in some of these files. Do you want a patch with those fixed to check-in? P.S. I don't have cvs access so someone will have to do the honors after reviews are done.

Comment on attachment 155024 [details] [diff] [review] Review version remove requests on old patch

Comment on attachment 155084 [details] [diff] [review] Updated per comments other than that, looks fine. When we're finished with the reviews, I'll checkin the patch that has the whitespace changes. + if (type.Equals("pop")) + type.Assign("pop3"); + // we use "nntp" in the server list so translate it here. + if (type.Equals("news")) + type.Assign("nntp"); should be else if (type.Equals...)

patch with whitespace fixes and else if change from last comment

Comment on attachment 155084 [details] [diff] [review] Updated per comments switching sr request to Neil...

Comment on attachment 155084 [details] [diff] [review] Updated per comments >+ * search for the server with the given url >+ * an analog to FindServer() >+ */ >+ nsIMsgIncomingServer >+ FindServerByUrl(in string url); >+ >+ /* >+ * Same as FindServerByURL() except it compares the input values against >+ * 'realhostname' and 'realuserName' pref settings. >+ */ >+ nsIMsgIncomingServer >+ FindRealServerByUrl(in string url); Do these need to be accessible from JS? a) the "string" type doesn't completely express a URL, e.g. a username of François or a hostname of öko.de b) all the callers already have a URL, and currently have to GetSpec() and get() a spec to pass to these functions. I think you would be better of with something along the lines of nsIMsgIncomingServer FindServerByUri(in nsIURI aURI, in boolean aRealFlag); >+ // Get username and hostname and port so we can get the server >+ nsCAutoString username; >+ rv = aUrl->GetUserPass(username); Shouldn't you be using the Username functions rather than the UserPass ones? >+ if (NS_SUCCEEDED(rv) && !username.IsEmpty()) >+ nsUnescape(NS_CONST_CAST(char*,username.get())); I think NS_UnescapeURL will suffice. Please don't abuse the string API with const casts. >+ if (type.Equals("pop")) EqualsLiteral, please. >+ type.Assign("pop3"); AssignLiteral, please. >+ // hostname might be blank, pass "" instead >+ serverInfo.hostname = hostname.get() ? hostname.get() : ""; Actually only xpidl strings may return null from get(). >+ // Dummy string to initialize the URL >+ nsCAutoString spec("http://user@hostname:1111"); Intriguing... does setting the scheme, host, user and port not work without this? >+ nsXPIDLCString thisHostname; >+ if (entry->useRealSetting) >+ rv = server->GetRealHostName(getter_Copies(thisHostname)); >+ else >+ rv = server->GetHostName(getter_Copies(thisHostname)); You missed some indent here, I think. >+ nsXPIDLCString turl; Why not an nsCAutoString? (Although given my comments above this should get removed entirely.) >+ tschm.Assign(GetIncomingServerType()); >+ url->SetScheme(tschm); I think you can use nsDependentCString(GetIncomingServerType()) here.

Neil, Like this?

Neil, Replies to your review. (In reply to comment #67) > (From update of attachment 155084 [details] [diff] [review]) > >+ * search for the server with the given url > >+ * an analog to FindServer() > >+ */ > >+ nsIMsgIncomingServer > >+ FindServerByUrl(in string url); > >+ > >+ /* > >+ * Same as FindServerByURL() except it compares the input values against > >+ * 'realhostname' and 'realuserName' pref settings. > >+ */ > >+ nsIMsgIncomingServer > >+ FindRealServerByUrl(in string url); > Do these need to be accessible from JS? a) the "string" type doesn't completely > express a URL, e.g. a username of François or a hostname of öko.de b) all the > callers already have a URL, and currently have to GetSpec() and get() a spec to > pass to these functions. I think you would be better of with something along > the lines of nsIMsgIncomingServer FindServerByUri(in nsIURI aURI, in boolean > aRealFlag); Only findRealServer is used by JS, so I took your idea and went with it. > > >+ // Get username and hostname and port so we can get the server > >+ nsCAutoString username; > >+ rv = aUrl->GetUserPass(username); > Shouldn't you be using the Username functions rather than the UserPass ones? Using Userpass functions because that is what is used throughout mailnews. bienvenu might have the background. As I type, I just thought of a reason why we use UserPass: Some ISPs have the username as something@isp.net. If we use just the username functions, then we would not get the complete username from the URL because it would be parsed at the @ > > >+ if (NS_SUCCEEDED(rv) && !username.IsEmpty()) > >+ nsUnescape(NS_CONST_CAST(char*,username.get())); > I think NS_UnescapeURL will suffice. Please don't abuse the string API with > const casts. > Done. > >+ if (type.Equals("pop")) > EqualsLiteral, please. > Done. > >+ type.Assign("pop3"); > AssignLiteral, please. > Done. > >+ // hostname might be blank, pass "" instead > >+ serverInfo.hostname = hostname.get() ? hostname.get() : ""; > Actually only xpidl strings may return null from get(). > > >+ // Dummy string to initialize the URL > >+ nsCAutoString spec("http://user@hostname:1111"); > Intriguing... does setting the scheme, host, user and port not work without > this? That is correct. I ran into the problem while debugging. If the userpass, scheme or port aren't set at the beginning, the set functions won't allow them to be changed. In other words, you can't build a URL from scratch. You have to put a dummy value like this in and then change the parts you need to change. I'm not sure if this is a bug or if there is a purpose behind the behavior. Darin would know, I suppose. > > >+ nsXPIDLCString thisHostname; > >+ if (entry->useRealSetting) > >+ rv = server->GetRealHostName(getter_Copies(thisHostname)); > >+ else > >+ rv = server->GetHostName(getter_Copies(thisHostname)); > You missed some indent here, I think. Got it. > > >+ nsXPIDLCString turl; > Why not an nsCAutoString? (Although given my comments above this should get > removed entirely.) Removed as you mentioned. > > >+ tschm.Assign(GetIncomingServerType()); > >+ url->SetScheme(tschm); > I think you can use nsDependentCString(GetIncomingServerType()) here. Done. > I'm going to ask bienvenu to review again, since I did modify this quite a bit. Could you look at the beginning of a patch in 238583 for the UI portion and give me some feedback? Thanks, Kevin

Comment on attachment 155979 [details] [diff] [review] v2 OK, so if bienvenu's happy with GetUserPass then so be it, it just looks odd to me. You only appear to have two callers of InternalFindServerByUri, would it be too much to ask to move the code into FindServerByUri and call that from FindRealServer?

Kevin is right; mailnews has always used those odd necko GetUserPass methods, though I think we could use GetUsername instead - probably better to deal with that after this patch lands. I'll wait for Kevin to respond to Neil's last comment about moving InternalFindServerByUri into FindServerByUri...I'm all for getting rid of an extra method if it doesn't result in more than negligible code duplication.

(In reply to comment #71) > Kevin is right; mailnews has always used those odd necko GetUserPass methods, > though I think we could use GetUsername instead - probably better to deal with > that after this patch lands. Because of my example (username@isp.net as the mail username), I don't think you'll be able to get away from UserPass (unless you do some magic with escaping). At first glance, doesn't seem worth the effort, but I'm a rookie, so I could very easily be wrong. > > I'll wait for Kevin to respond to Neil's last comment about moving > InternalFindServerByUri into FindServerByUri...I'm all for getting rid of an > extra method if it doesn't result in more than negligible code duplication. Makes sense. I'll spin a new patch (including whitespace changes). As a followup bug after this is all in, we could move FindServer() to use FindServerByUri() and get rid of the support functions as well. I didn't want to do that in this bug because of the use of FindServer() in the import code. Kevin

Comment on attachment 155979 [details] [diff] [review] v2 >Index: mozilla/mailnews/base/public/nsIMsgAccountManager.idl >+ nsIMsgIncomingServer >+ FindServerByUri(in nsIURI aURI, in boolean aRealFlag); please make the method lowercase. we can fix the other methods now or later. > nsIMsgIncomingServer >- findRealServer(in string userName, in string hostname, in string type); >+ findRealServer(in string userName, in string hostname, in string type, in long port ); >Index: mozilla/mailnews/base/src/nsMsgAccountManager.cpp >+nsMsgAccountManager::InternalFindServerByUri(nsIURI *aUri, >+ PRBool useRealSetting, >+ nsIMsgIncomingServer** aResult) Does this cover the netscape mock protocol, or scale to cover other internal protocols that people may add? >+ nsCAutoString hostname; this pattern bothers me: >+ serverInfo.hostname = hostname.get() ? hostname.get() : ""; >+ serverInfo.username = username.get() ? username.get() : ""; >+ serverInfo.type = type.get() ? type.get() : ""; >+ if (!serverInfo.server) return NS_ERROR_UNEXPECTED; return on same line makes breakpointing harder. > nsMsgAccountManager::FindRealServer(const char* username, > const char* hostname, > const char* type, >+ PRInt32 port, > nsIMsgIncomingServer** aResult) > { >- InternalFindServer(username, hostname, type, PR_TRUE, aResult); >+ // Dummy string to initialize the URL >+ nsCAutoString spec("http://user@hostname:1111"); >+ nsresult rv; >+ nsCOMPtr<nsIURL> aUrl = do_CreateInstance(NS_STANDARDURL_CONTRACTID, &rv); >+ if (NS_FAILED(rv)) return NS_OK; you aren't clearing aResult but you're returning NS_OK. this feels really bad >+#ifdef DEBUG please change this to DEBUG_<you> or use nspr logging. thanks. >+ aUrl->GetSpec(spec); >+ printf("aUrl == %s\n", spec.get()); >+#endif >+ InternalFindServerByUri(aUrl, PR_TRUE, aResult); > return NS_OK; > } >+nsMsgAccountManager::findServerUrl(nsISupports *aElement, void *data) >+{ >+ nsresult rv; >+ >+ nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(aElement, &rv); since you aren't really using rv here, i'd suggest you save do_QI from setting it. just nullcheck server. >+ if (NS_FAILED(rv)) return PR_TRUE; >+ // Don't try and get a port for the 'none' service service or protocol? >Index: mozilla/mailnews/base/util/nsMsgMailNewsUrl.cpp >@@ -167,53 +167,50 @@ > NS_IMETHODIMP nsMsgMailNewsUrl::GetServer(nsIMsgIncomingServer ** aIncomingServer) >+ nsresult rv; >+ nsCAutoString turl; turl? >Index: mozilla/mailnews/local/src/nsLocalUtils.cpp >+ // No unescaping of username or hostname done here. >+ // This is done inside of InternalFindServerByUri This => It (?) > // in an imap hiearchy. look for an imap server. ^^^^^^^^ (sp) > nsLocalURI2Server(const char* uriStr, > nsIMsgIncomingServer ** aResult) > *aResult = server; > NS_IF_ADDREF(*aResult); I wonder why you didn't combine those two lines >Index: mozilla/mailnews/local/src/nsLocalMailFolder.cpp >@@ -3107,81 +3107,63 @@ that code looks like it's asking for a loop.

(In reply to comment #73) > (From update of attachment 155979 [details] [diff] [review]) > >Index: mozilla/mailnews/base/public/nsIMsgAccountManager.idl > >+ nsIMsgIncomingServer > >+ FindServerByUri(in nsIURI aURI, in boolean aRealFlag); > > please make the method lowercase. we can fix the other methods now or later. Done + nsIMsgIncomingServer + findserverbyuri(in nsIURI aURI, in boolean aRealFlag); > > > nsIMsgIncomingServer > >- findRealServer(in string userName, in string hostname, in string type); > >+ findRealServer(in string userName, in string hostname, in string type, in long port ); Done also. + findrealserver(in string userName, in string hostname, in string type, in long port ); > > >Index: mozilla/mailnews/base/src/nsMsgAccountManager.cpp > > >+nsMsgAccountManager::InternalFindServerByUri(nsIURI *aUri, > >+ PRBool useRealSetting, > >+ nsIMsgIncomingServer** aResult) > > Does this cover the netscape mock protocol, or scale to cover other internal > protocols that people may add? > > >+ nsCAutoString hostname; > > this pattern bothers me: > >+ serverInfo.hostname = hostname.get() ? hostname.get() : ""; > >+ serverInfo.username = username.get() ? username.get() : ""; > >+ serverInfo.type = type.get() ? type.get() : ""; > > > >+ if (!serverInfo.server) return NS_ERROR_UNEXPECTED; > > return on same line makes breakpointing harder. > > > nsMsgAccountManager::FindRealServer(const char* username, > > const char* hostname, > > const char* type, > >+ PRInt32 port, > > nsIMsgIncomingServer** aResult) > > { > >- InternalFindServer(username, hostname, type, PR_TRUE, aResult); > >+ // Dummy string to initialize the URL > >+ nsCAutoString spec("http://user@hostname:1111"); > >+ nsresult rv; > >+ nsCOMPtr<nsIURL> aUrl = do_CreateInstance(NS_STANDARDURL_CONTRACTID, &rv); > >+ if (NS_FAILED(rv)) return NS_OK; > > you aren't clearing aResult but you're returning NS_OK. this feels really bad > Comment above the procedure say we must return NS_OK. All callers check the result for validity. Should I do something more? > >+#ifdef DEBUG > > please change this to DEBUG_<you> or use nspr logging. thanks. > > >+ aUrl->GetSpec(spec); > >+ printf("aUrl == %s\n", spec.get()); > >+#endif > >+ InternalFindServerByUri(aUrl, PR_TRUE, aResult); > > return NS_OK; > > } > Done. > > >+nsMsgAccountManager::findServerUrl(nsISupports *aElement, void *data) > >+{ > >+ nsresult rv; > >+ > >+ nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(aElement, &rv); > > since you aren't really using rv here, i'd suggest you save do_QI from setting > it. just nullcheck server. > > >+ if (NS_FAILED(rv)) return PR_TRUE; > Done. + nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(aElement); + if (!server) return PR_TRUE; > >+ // Don't try and get a port for the 'none' service > > service or protocol? > Changed comment to following: + // Don't try and get a port for the 'none' scheme > >Index: mozilla/mailnews/base/util/nsMsgMailNewsUrl.cpp > >@@ -167,53 +167,50 @@ > > NS_IMETHODIMP nsMsgMailNewsUrl::GetServer(nsIMsgIncomingServer ** aIncomingServer) > >+ nsresult rv; > >+ nsCAutoString turl; > > turl? My shorthand for "temp url"; changed to urlstr + nsCAutoString urlstr; > > >Index: mozilla/mailnews/local/src/nsLocalUtils.cpp > >+ // No unescaping of username or hostname done here. > >+ // This is done inside of InternalFindServerByUri > This => It (?) > Changed second line of comment to be clearer. + // The unescaping is done inside of FindServerByUri > > // in an imap hiearchy. look for an imap server. > ^^^^^^^^ (sp) Fixed. > > > nsLocalURI2Server(const char* uriStr, > > nsIMsgIncomingServer ** aResult) > > > *aResult = server; > > NS_IF_ADDREF(*aResult); > > I wonder why you didn't combine those two lines I missed one? Changed now. > > >Index: mozilla/mailnews/local/src/nsLocalMailFolder.cpp > >@@ -3107,81 +3107,63 @@ > > that code looks like it's asking for a loop. > Being a rookie, I didn't have a bright idea of how do do this simply. I came up with the following, but I think that I would have problems with setting mType to the xxxx and then having it disappear off the stack. I'll take a suggestion, otherwise I'll leave it as is. My attempt (extra details cut out) int i; int maxSchemes; nsCAutoString scheme; #ifdef HAVE_MOVEMAIL maxSchemes = 4 #else maxSchemes = 3 #endif /* HAVE_MOVEMAIL */ for(i=1; i <= maxSchemes; i++) { switch (i) { case 1: scheme.Assign("none"); break; case 2: scheme.Assign("pop"); break; case 3: scheme.Assign("rss"); break; case 4: scheme.Assign("movemail"); break; default: break; } url->SetScheme(scheme); rv = accountManager->FindServerByUri(url, PR_FALSE, getter_AddRefs(server)); if (NS_SUCCEEDED(rv) && server) { mType = scheme.get(); return mType; } } Thanks for all of the help from everyone. Kevin

fixes per timeless

Comment on attachment 156069 [details] [diff] [review] v3 Carry forward sr from neil

Comment on attachment 155979 [details] [diff] [review] v2 clear request on obsolete patch

To avoid posting the whitespace version multiple times, once I have reviews finished, I'll post the whitespace version for checkin. Kevin

Comment on attachment 156069 [details] [diff] [review] v3 sr=bienvenu, with one nit - we need a new uid for nsIMsgAccountManager since we added methods.

(In reply to comment #79) > (From update of attachment 156069 [details] [diff] [review]) > sr=bienvenu, with one nit - we need a new uid for nsIMsgAccountManager since we > added methods. > David, Can I just use guidgen on my window box to get this guid? I read that I can get one on irc.mozilla.org, but I don't have access to irc here at work. If I can use guidgen, I'll have the patch shortly. Kevin

yes, I think so - I use uuidgen, but I assume guidgen is equivalent...

Final version for checkin David, I changed the uuid in this patch. I also attached two small changes in the JS to call the new version of findrealserver until the front end work in bug 238583 is done. I just pass 0 so that the port is not checked. I had overlooked this change because I am simultaneously working on the front end. Kevin

*** Bug 55540 has been marked as a duplicate of this bug. ***

Being a rookie, I messed up on changing the case for findserverbyuri and findrealserver. This patch is the same as the previous check-in patch, just with the case of those two procedures corrected so that it will compile. I best learn the case mangling that the idl compiler does for next time.

Looks like your real final version wasn't :-(

timeless checked in a bustage fix to nsLocalMailServer.cpp (missed movemail changes) and I checked on in to nsMsgAccountManager.h (duplicate const)

the main patch introduced some imap problems. The first is a ton of thread-safety assertions. This patch fixes the thread-safety assertions by caching the server key in the url, so we don't have to get the server object. There are still problems - all imap headers are downloaded every time I startup and we don't ever seem to see the .msf files for the imap folders...I'll look into that next.

this fixes imap folder path handling so we won't think we have to hash every folder name, and throw away .msf files.

Comment on attachment 156474 [details] [diff] [review] fix thread safety problems in imap introduced by this patch thanks for cleaning this stuff up david.

mozilla/mailnews/base/prefs/resources/content/AccountWizard.js 1.125 mozilla/mailnews/compose/src/nsSmtpServer.cpp 1.47 mozilla/mailnews/base/util/nsMsgDBFolder.cpp 1.227 mozilla/mailnews/base/util/nsMsgMailNewsUrl.cpp 1.101 mozilla/mailnews/base/src/nsMsgAccountManager.cpp 1.286 mozilla/mailnews/base/src/nsMsgAccountManager.h 1.46 mozilla/mailnews/local/src/nsLocalMailFolder.cpp 1.473 mozilla/mailnews/local/src/nsLocalUtils.cpp 1.53 mozilla/mailnews/base/public/nsIMsgAccountManager.idl 1.62 mozilla/mailnews/news/src/nsNntpService.cpp 1.251 mozilla/mailnews/imap/src/nsImapService.cpp 1.302 mozilla/mailnews/imap/src/nsImapUtils.cpp 1.87 mozilla/mailnews/base/prefs/resources/content/AccountManager.js 1.109 mozilla/layout/xul/base/src/nsMenuBarListener.h 1.20 mozilla/mailnews/base/src/nsMsgAccountManager.h 1.47

if you have an rss account, you get a lot of assertions - this fixes those assertions by returning the same default port as the no incoming server server.

Comment on attachment 156513 [details] [diff] [review] fix assertion with rss servers sr=sspitzer

my fix to fix the thread-safety assertions broke a special case of imap online server directory handling - basically, we weren't adding the hierarchy delimiter because we couldn't find the namespace. This was trunk only

*** Bug 101911 has been marked as a duplicate of this bug. ***

*** Bug 254194 has been marked as a duplicate of this bug. ***

*** Bug 274995 has been marked as a duplicate of this bug. ***

Any news on this bug? I just ran into it with Thunderbird 1.5. Thunderbird complains that I'm trying to add another account with the same server/username, which is not true. But it seems my prefs.js contains much more accounts than are shown in Thunderbird. Like the original reporter I'm using SSH with port forwarding to access mutiple servers through localhost. Any work-around?

Comment on attachment 156474 [details] [diff] [review] fix thread safety problems in imap introduced by this patch bienvenu%nventure.com 2004-09-07 17:25 mozilla/mailnews/imap/src/nsImapUrl.cpp 1.170

Comment on attachment 156478 [details] [diff] [review] fix imap folder path handling neil%parkwaycc.co.uk 2004-08-18 16:11 mozilla/mailnews/imap/src/nsImapUtils.cpp 1.87

Comment on attachment 156513 [details] [diff] [review] fix assertion with rss servers bienvenu%nventure.com 2004-08-19 08:21 mozilla/mailnews/local/src/nsRssService.cpp 1.2

Comment on attachment 158144 [details] [diff] [review] proposed fix for regression introduced by attachment 156748 [details] bienvenu%nventure.com 2004-09-07 17:25 mozilla/mailnews/imap/src/nsImapUrl.cpp 1.170

Comment on attachment 156474 [details] [diff] [review] fix thread safety problems in imap introduced by this patch oops. wrong annotation. this is really: bienvenu%nventure.com 2004-08-19 07:41 mozilla/mailnews/imap/src/nsImapUrl.cpp 1.169 and i'm way too sleepy to be doing this.

Marcel, can you give any more details? The work around may be to clear out the cruft from prefs.js, the account prefs that don't correspond to accounts in the account list - mail.accountmanager.accounts contains a list of accounts, and then there are corresponding prefs like mail.account.account9.identities, and mail.account.account9.server. So, you could remove the prefs for accounts that aren't in the list. And you could remove the servers that aren't hooked up to accounts. But I'm not sure that's really going to help - I think we ignore those unused prefs, for the most part, because we build up the in memory structures from the accountmanager.accounts pref, i.e., we add accounts for each of those, and then load the prefs for each of the servers pointed to by the accounts.

I just tried to add a new POP-account with the same user name and server as already exists in my profile. The wizard give me an error this is not possible. Why is this an error? It shouldn't be an error since sometimes you will have multiple accounts that you access with ssh through port forwarding. Let's say I have 2 accounts with ISP X and ISP Y and both have the same username. I would setup ssh forwarding like this: localhost:8110 --> mail.ispx.com:110 localhost:9110 --> mail.ispy.com:110 In this case I would not be able to setup these both accounts in Thunderbird because it would complain I have two accounts with same server and user name. So either the check would be removed or should include the port number while comparing. Right now it's not possible to provide a port number in the wizard.

Lots of patches and code review... from three years ago. Still broken as of 1.5.0.13. I can create a second IMAP account with the same host and username, but the port setting is ignored. Actually, it's worse than ignored: if I change the port number in the second account, the port number in the first account is also changed!

This is still broken in Thunderbird 2.0.0.0. I used the Account Manager to make a second account at my server. The new account doesn't show up inside the Account Manager and it doesn't show up in the Folders pane. However, if I click on the 1st account at the server, it asks for the password for the (messed up) second account! I'm pretty sure I have to edit the prefs.js file, but I'm not sure exactly how much stuff to take out.

This bug is done, isn't it? UI for it is bug 238583.

You can kind of get around this problem by manipulating the mail.accountmanager.accounts list, putting the account that you want to use first in the list. Then your old working account silently disappears, replaced by the new one. Re-order the list to get the other account back. This works when the server/port number pair is identical as can easily occur in newsgroups. This workaround has not been explicitly tested for the condition actually described in the bug. You could also possibly get sometimes around this (again for newsgroups) by using the multiple identities feature, if you could only always remember to use the correct identity consistently. I was hoping to be able to create different newsgroup "accounts" to access different newsgroups using the same server and the same port. Alas, this ten year old bug/feature gets in the way.

My apologies, but this is insane that it hasn't been fixed in approaching 12 years (it was filed around the time I started working on Mozilla). It look like we've come close to fixing it at least twice. Even if those are horridly bit-rotted, the front-end account creation code SHOULD NOT let you create a known you-will-be-screwed situation. Or at least it should warn you off strenuously! Look at how close I came (bug 709442) to losing 2.5 months of email *forever* because our own admins thought this *should* work, to the point where they didn't even feel the need to test it with Thunderbird (just mail.app on Mac, I assume). I'll morph bug 709442 into a "make the UI block dangerous account creates" bug, but guys, this is just clownshoes - we can't honestly call Thunderbird a solid client if it lets you do stuff like this. One experience with a bug like this and entire organizations can swear off it forever. Leaving priority alone since bug 709442 is already critical, and that will block the dataloss side of things.

Yes, i realy hate this *feature* of TB. Sad thing ... I´m not a programmer, but such bugs *should* be fixed, IMHO. BTW, it is also not OK to just import all mails from each new account added! I have accounts with gigabyte of collected mails - they should NOT be imported automatically! Realy ...

(In reply to Magnus Melin from comment #109) > This bug is done, isn't it? UI for it is bug 238583. So why is it still open?

Well, if the original comment in that bug is correct, that's just the UI front-end to whatever fix is implemented via this bug. However, I haven't reviewed the patches there; the bug may have morphed into covering backend and frontend; if so this bug can probably be dupped against it.

This bug here seems to be only about backend. I ask if that is finished because this bug here is still NEW. Patches in bug 238583 seem to be frontend only. I could work on that bug but need to know if the backend is prepared for this.

Given the amount of patches that did go into the backend (in this bug), it likely works, possibly buggy.

The error message seems to have changed, and the underlying problem does too (perhaps this requires a new bug report). I can set up two POP accounts on the same server, but TB then warns me that ""the directory specified in the local directory setting is already used by the [other account name]". Checking the account settings shows that the local directory in server settings is the same. If I change it, it changes for both accounts.