Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Sending mail through SMTP server that doesn't require user:pass fails

RESOLVED FIXED in mozilla1.9beta4

Status

MailNews Core
Composition
--
major
RESOLVED FIXED
10 years ago
4 years ago

People

(Reporter: philor, Assigned: standard8)

Tracking

({regression, verified1.8.1.13})

unspecified
mozilla1.9beta4
regression, verified1.8.1.13
Dependency tree / graph
Bug Flags:
blocking1.8.1.13 +
wanted1.8.1.x +
blocking1.8.0.next +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
Blocks: 415034
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?
Created attachment 305379 [details] [diff] [review]
The fix (diff -w)

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.
Attachment #305379 - Flags: superreview?(neil)
Attachment #305379 - Flags: review?(philringnalda)
Created attachment 305380 [details] [diff] [review]
[checked in] The fix (normal patch)

Comment 6

10 years ago
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);
Attachment #305379 - Flags: superreview?(neil) → superreview+

Updated

10 years ago
Duplicate of this bug: 419434

Comment 8

10 years ago
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?
Created attachment 305664 [details] [diff] [review]
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.
Attachment #305664 - Flags: superreview?(neil)
Attachment #305664 - Flags: review?(dmose)
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 :)
Attachment #305379 - Flags: review?(philringnalda)
Attachment #305379 - Flags: review?(dmose)
Attachment #305379 - Flags: review+
Comment on attachment 305664 [details] [diff] [review]
The additional hack, v.1

Looks good; r=dmose.
Attachment #305664 - Flags: review?(dmose) → review+
Comment on attachment 305379 [details] [diff] [review]
The fix (diff -w)

r=dmose
Attachment #305379 - Flags: review?(dmose) → review+
Blocks: 419591
(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.
Attachment #305380 - Attachment description: The fix (normal patch) → [checked in] The fix (normal patch)

Comment 15

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

Comment 16

10 years ago
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.
Attachment #305664 - Flags: superreview?(neil) → superreview-

Comment 17

10 years ago
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.
Attachment #305664 - Attachment is obsolete: true
(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.
Assignee: nobody → bugzilla
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Updated

10 years ago
Duplicate of this bug: 419672

Updated

10 years ago
Duplicate of this bug: 419150

Comment 21

10 years ago
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. 

Comment 22

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

Comment 23

10 years ago
I got the update to 20080227 and it works! Thanks !!

Comment 24

10 years ago
How ever I can not delete messages.

Comment 25

10 years ago
Quit and re started and now it does work.
Target Milestone: --- → mozilla1.9beta4

Comment 26

10 years ago
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 ???

Updated

10 years ago
Flags: blocking1.8.1.13?
(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.
Version: Trunk → unspecified

Comment 28

10 years ago
this has made Tb unusable in 1.8 branch, lately. Please, fix it! See here:
http://forums.mozillazine.org/viewtopic.php?t=634434
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.13?
Flags: blocking1.8.1.13+
Created attachment 308037 [details] [diff] [review]
Allow http://@example.com

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.
Attachment #308037 - Flags: superreview?
Attachment #308037 - Flags: review?(neil)
Attachment #308037 - Flags: approval1.8.1.13?
Attachment #308037 - Flags: superreview? → superreview?(cbiesinger)

Comment 30

10 years ago
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.
Attachment #308037 - Flags: review?(neil) → review+
Created attachment 308078 [details] [diff] [review]
Keep empty userinfo check separate

Neil suggested keeping the "no userinfo at all" case separate for clarity, since that's the case that's broken here.
Attachment #308078 - Flags: superreview?(neil)
Attachment #308078 - Flags: review?(neil)
Attachment #308078 - Flags: approval1.8.1.13?

Updated

10 years ago
Attachment #308078 - Flags: superreview?(neil)
Attachment #308078 - Flags: superreview+
Attachment #308078 - Flags: review?(neil)
Attachment #308078 - Flags: review?(cbiesinger)
(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?
Attachment #308037 - Flags: superreview?(cbiesinger) → superreview+
Comment on attachment 308078 [details] [diff] [review]
Keep empty userinfo check separate

this patch seems better
Attachment #308078 - Flags: review?(cbiesinger) → review+
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
Attachment #308037 - Attachment is obsolete: true
Attachment #308037 - Flags: approval1.8.1.13?
Comment on attachment 308078 [details] [diff] [review]
Keep empty userinfo check separate

Approved for 1.8.1.13. a=ss
Attachment #308078 - Flags: approval1.8.1.13? → approval1.8.1.13+
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.
Attachment #308078 - Flags: approval1.9?
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
Keywords: fixed1.8.1.13
Depends on: 421840

Comment 38

10 years ago
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 ...)
Keywords: fixed1.8.1.13 → verified1.8.1.13
You're right, they won't see the request. Open the bug, and they will, though.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 308078 [details] [diff] [review]
Keep empty userinfo check separate

a1.9+=damons
Attachment #308078 - Flags: approval1.9? → approval1.9+
Fix checked into trunk
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 42

9 years ago
blocking 1.8.0.15
Flags: blocking1.8.0.15+

Comment 43

9 years ago
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.
Attachment #308078 - Flags: approval1.8.0.15?
No longer blocks: 419591
Blocks: 419591
Product: Core → MailNews Core
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.
Attachment #308078 - Flags: approval1.8.0.next?
You need to log in before you can comment on or make changes to this bug.