Starting with the 2008-02-21-03 Thunderbird trunk nightly, sending email through an SMTP server that doesn't use a username/password fails, claiming that it was "unable to log in to SMTP server smtp.example.com" (though it's clearly something else, since it doesn't require any network activity to be unable to log in). In the same builds, sending through a server that *does* support SMTP-AUTH works just fine. Unfortunately, I'm going to be gone all weekend so I won't be able to experiment with backing things out, but by far the most likely looking thing between 2008-02-20-03 and 2008-02-21-03 is bug 415034, making it look like somewhere (maybe http://mxr.mozilla.org/seamonkey/source/mailnews/compose/src/nsSmtpDelegateFactory.cpp#79, and why are we stomping on the SetSpec rv rather than checking it?), we are creating a URI with an empty user:pass, rather than with none.

Stole enough time to back out bug 415034, which does indeed make things work again.

And since bug 415034 already has approval1.8.1.13, it would be super-extra-nice if someone who's home this weekend could build branch Tbird with that patch applied, and see whether we're going to break there, too.

I've just tried this on trunk seamonkey (pulled earlier today), I get this on the console WARNING: NS_ENSURE_TRUE(port >= 0 && port <= 0xFFFF) failed: file /opt/mozillaextra/master/mozilla/netwerk/base/src/nsSocketTransportService2.cpp, line 465 Here nsMsgProtocol::OpenNetworkSocket calls GetPort() on a URI and gets -1 (i.e. default port), but doesn't do anything with it, e.g. translate it to be non-default. I'm fairly sure this first part is due to bug 403480. I tried manually hacking that to set it to 25 (for smtp), but still got the error message as described, but with the following warning this time: WARNING: NS_ENSURE_TRUE(host && *host) failed: file /opt/mozillaextra/master/mozilla/netwerk/dns/src/nsHostResolver.cpp, line 413 Turns out in nsMsgProtocol::OpenNetworkSocket, GetHost also returns empty for the hostname. This is due to incorrect mailnews code (I believe), but is shown up by bug 415034. See http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/compose/src/nsSmtpService.cpp&rev=1.148&mark=161-163,210-216#159 - when aSmtpUserName is empty, .get() returns "" (the first marked blocked), the second marked block then does !aSmtpUserName (this time its a char*) so in the empty username case we get a url of the form: smtp://@localhost:25/... hence this triggers the protection in bug 415034. I've a patch for this second part, but not for the first yet. SMTP isn't currently registered as a protocol (though it will need to be for password manager), fixing it that way might be the best way to go, but I haven't assessed how long that will take. The other question, is would the nsMsgProtocol::OpenNetworkSocket function affect any of our other mailnews protocols with default ports?

This fixes the second part of the bug that I just described in comment 3. To test this, add "if (port == -1) port = 25;" in nsMsgProtocol::OpenNetworkSocket. It seemed best to just pass aSmtpHostName and aSmtpUserName as nsCString, thus we could use !IsEmpty() in the check. Additionally I dropped the horrible abuse of getter_Copies for nsEscape and replaced it with the (frozen API compatible) mailnews version. The urlSpec.get() check was also just useless (urlSpec would never have been empty, and if it was, it'd returned "" which will always return true), so that meant re-indenting half the function, so I decided to tidy up the whole function at the same time. Attaching diff -u, normal patch in a mo.

Comment on attachment 305379 [details] [diff] [review] The fix (diff -w) I'm a little worried about the port issue - what happens with SSL? >+ return smtpUrl->QueryInterface(NS_GET_IID(nsIURI), (void **) aUrl); return CallQueryInterface(smtpUrl, aUrl);

So I wen to edit the smtp and told it to use a user name and smtp over ssl and it will not send. What now?

I'm pretty sure Standard8's dead right that the correct answer is to make smtp URLs real, but while we wait for that, I think it would be nice to be able to send mail, and this plus attachment 305379 [details] [diff] [review] seems to give us that.

Comment on attachment 305379 [details] [diff] [review] The fix (diff -w) r=philringnalda in a "for what little that's worth, since I'm not actually a mailnews peer" sort of way. dmose, though, is eagerly awaiting an r? mail :)

Comment on attachment 305664 [details] [diff] [review] The additional hack, v.1 Looks good; r=dmose.

Comment on attachment 305379 [details] [diff] [review] The fix (diff -w) r=dmose

(In reply to comment #9) > Created an attachment (id=305664) [details] > The additional hack, v.1 > > I'm pretty sure Standard8's dead right that the correct answer is to make smtp > URLs real, but while we wait for that, I think it would be nice to be able to > send mail, and this plus attachment 305379 [details] [diff] [review] seems to give us that. I agree this is the right hack for now. I've raised bug 419591 and a series of others to cover the work we need to do for removing this hack once it is committed.

Comment on attachment 305380 [details] [diff] [review] [checked in] The fix (normal patch) I've checked this in now. It'll change the symptoms a little, but hopefully we can follow it up soon with the second patch.

OK, so with attachment 305380 [details] [diff] [review] I can't actually reproduce comment #3...

Comment on attachment 305664 [details] [diff] [review] The additional hack, v.1 This is wrong. Attachment 305380 [details] [diff] would work were it not for a typo.

Comment on attachment 305380 [details] [diff] [review] [checked in] The fix (normal patch) >- if (!PL_strchr(aSmtpHostName, ':')) >- { >- urlSpec.Append(':'); >- urlSpec.AppendInt(aSmtpPort); >- } >+ if (aSmtpHostName.FindChar(':') != -1) >+ { >+ urlSpec.Append(':'); >+ urlSpec.AppendInt(aSmtpPort); >+ } So, this is the bit everyone overlooked: PL_strchr(aSmtpHostName, ':') is null when there is no colon, so the condition needs to be aSmtpHostName.FindChar(':') == -1 The reason I didn't notice before is that all I did for my sr was to use the debugger to change aSmtpUserName to null and verify that mail was sent.

(In reply to comment #17) > So, this is the bit everyone overlooked: > PL_strchr(aSmtpHostName, ':') is null when there is no colon, > so the condition needs to be aSmtpHostName.FindChar(':') == -1 I've now checked in a bustage fix for this and sending SMTP now works correctly. Note that because we don't have an smtp protocol registered, the smtp url still returns a valid port number even in the default case - which is why this bug now works (I'll have to alter the follow-up bugs). So marking as fixed. Note that TB builds have been respun to incorporate the new fix, the buggy bug fix missed the SM builds.

Just attempted to use Thunderbird 3.01pre and it added some updates but will not send. I am not a programmer. I am a tester. I have gone back to Thunderbird version 2.0.0.12 (20080213) which does send with the smtp mail box I have been using for over a year.

Works fine over here, make sure you are using the 20080227 build (See Help | About.)

I got the update to 20080227 and it works! Thanks !!

How ever I can not delete messages.

Quit and re started and now it does work.

Hello ... This bug is occuring also on 1.8 branch since TB 2.0.0.13pre 20080304(at least) ... and is still there in the last 2.0.0.13pre 2008030603 See thread in forum: http://forums.mozillazine.org/viewtopic.php?t=632380&start=15&postdays=0&postorder=asc&highlight= My smtp server is Sendmail 8.13.4/8.13.4 It is really the same bug, because, if i switch to the gmail smtp (with authentification and TSL), it is working. This bug was for trunk Reopen and change version to all ???

(In reply to comment #26) > Hello ... > This bug is occuring also on 1.8 branch since TB 2.0.0.13pre 20080304(at least) > ... and is still there in the last 2.0.0.13pre 2008030603 Thanks for bringing this up. I'll get onto it later today. > This bug was for trunk > Reopen and change version to all ??? I've changed the version, but we don't reopen because the status normally refers to trunk where it affects more than one version.

this has made Tb unusable in 1.8 branch, lately. Please, fix it! See here: http://forums.mozillazine.org/viewtopic.php?t=634434

We can allow empty userinfo, and there might be other extensions which always use "@" with or without userinfo. Shouldn't encounter it in web content since IE doesn't allow it, but Opera does.

Comment on attachment 308037 [details] [diff] [review] Allow http://@example.com >- if (userinfoLen == 0) >- return NS_ERROR_MALFORMED_URI; I think it might possibly make sense to leave this special case in instead of merging it into the passwordless case below.

Neil suggested keeping the "no userinfo at all" case separate for clarity, since that's the case that's broken here.

(In reply to comment #6) > (From update of attachment 305379 [details] [diff] [review]) > I'm a little worried about the port issue - what happens with SSL? > > >+ return smtpUrl->QueryInterface(NS_GET_IID(nsIURI), (void **) aUrl); > return CallQueryInterface(smtpUrl, aUrl); Do you even need the QI?

Comment on attachment 308078 [details] [diff] [review] Keep empty userinfo check separate this patch seems better

Comment on attachment 308037 [details] [diff] [review] Allow http://@example.com Not using this one, the other patch makes the logic clearer than using the more compact ?: operator

Comment on attachment 308078 [details] [diff] [review] Keep empty userinfo check separate Approved for 1.8.1.13. a=ss

Comment on attachment 308078 [details] [diff] [review] Keep empty userinfo check separate Requesting approval1.9 to land this fix on the trunk as well in case other code/addons are doing the same thing as mailnews.

fix checked in. I doubt the trunk triagers will see the request in a closed bug so I'll open a new bug about landing on trunk

Verified 1.8.1.13 (was not working in TB 2.0.0.13pre 2008030903 ... and working after update to 2008031003) Don't mark bug as verified because i don't know what is the status for trunk (Comment #36 request aproval but on bug #37 Daniel told that he have open bug 421840 to signal aproval request for 1.9 ...)

You're right, they won't see the request. Open the bug, and they will, though.

Comment on attachment 308078 [details] [diff] [review] Keep empty userinfo check separate a1.9+=damons

Fix checked into trunk

blocking 1.8.0.15

Comment on attachment 308078 [details] [diff] [review] Keep empty userinfo check separate caillon, please sign off for 1.8.0.15. it fixes regression from bug 415034 and should apply cleanly.

Comment on attachment 308078 [details] [diff] [review] Keep empty userinfo check separate Removing long-obsolete approval request as it is no longer needed any more.