Closed Bug 23096 Opened 21 years ago Closed 21 years ago

Specifying pop server with the port number appended crashes

Categories

(MailNews Core :: Networking, defect, P5)

Other
Linux
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: stuart, Assigned: alecf)

Details

(Keywords: crash, Whiteboard: fix in hand)

When I specify a pop server or smtp server as localhost:port mozilla doesn't
accept it and later crashes.

Is there another way to specify the port number?
Priority: P3 → P5
QA Contact: lchiang → esther
Assignee: mscott → alecf
stuart, there is a separate preference for setting the port on a server. Try
doing that. Look in your prefs.js and for the server in question add a pref that
looks like the following:
user_pref("mail.server.server1.port", 143);

where 143 is your port # and server1 is the name of the server in your
preferences (you should see lots of server prefs already there with that name).

It looks like the account manager needs to provide a slot for the port number so
users can enter the port in the UI. Re-assiging to alecf.
I believe this is a dupe of something I already have, but I'm not sure.
Status: NEW → ASSIGNED
stuart, can you get a stack trace of the crash? I don't know why it would be
crashing, but I'd like to see where...
Target Milestone: M15
well, since migration doesn't make this crash, and users aren't likely going to
do this, I'm holding off until m15.
Adding "crash" keyword to all known open crasher bugs.
Keywords: crash
I think average users would be confused to have an additional port slot with the 
POP host address (even if the default port is already pre-specified).
This would include the need to explain what they are for and that they usually 
shouldn't be touched.

It is better to have just one textfield in "host[:port]" format, so experienced 
users can choose different ports, while "grandma" wouldn't even notice that 
there's something complicated like ports.
there's kind of an unwritten rule in software development: it's better to 
possibly confuse advanced users than to definately confuse novice users...  

imagine tech support on the phone to a user, "Ok, now where it has your 
hostname, yes isp.blah.com, put a colon, and then 394. no, I meant "after" as in 
in that same text field, not the next text field.. no don't put a space. yes a 
colon is the one that looks like a two periods, not a period and a comma, 
that's a semicolon... it doesn't work? read to me exactly character for 
character what the "hostname" field says..." etc etc...

not only that, but it's not at ALL obvious that putting :port at the end of a 
hostname means to use that port, whether your an advanced user or a novice 
user.... nowhere in the UI does it say ":port" or "put your port after the 
hostname with a colon" or anything wierd like that... and so I am against this 
idea.



the "port" setting will be in some kind of "advanced" dialog somewhere so gramma 
wouldn't even see it.
David, could you try to reproduce this, figure out what's going on, and
hopefully fix it? thanks.
Assignee: alecf → davidmc
Status: ASSIGNED → NEW
Sure, I'll try to work this one out.  Sounds like fun.  But maybe someone can 
coach me on what is supposed to happen when I pick a random port number in order 
to reproduce the crashing?  I don't have any easy means to setup a server to 
actually use an arbitrary port number (never did it before).

However, the part about putting the port number somewhere in the prefs is easier, 
and I can have fun following the code path to see it actually gets used by the 
socket layer wherever that is.
I think we crash before we attempt to make a connection to the bogus port.  
Thus, you don't need to configure any server. That's my guess, anyway, but 
actually trying it out should tell you.
The following change in nsMsgIncomingServer.cpp seems to fix the problem; it 
replaces the macro which generated both GetHostName() and SetHostName(), with
explicit implementations which  qualify the hostname when getting.  If a colon is 
detected anywhere in the name, a zero is slammed over that byte and the result is 
written back to prefs.  Note this is clearly hacky, but less complex than going 
for some general scheme to qualify hostnames in a more general sense.

Maybe alecf can review this change and consider any complications.  My simple 
runtime test removes the bad behavior; but that's not very conclusive.

//NS_IMPL_SERVERPREF_STR(nsMsgIncomingServer, HostName, "hostname");
NS_IMETHODIMP
nsMsgIncomingServer::GetHostName(char **retval)
{
	nsresult rv = GetCharValue("hostname", retval);
	if ( NS_SUCCEEDED(rv) && retval && *retval)
	{
		char* name = *retval;
		int c = (unsigned char) *name;
		while ( c )
		{
			if ( c == ':' )
			{
				*name = 0; // clobber the first colon character
				SetCharValue("hostname", *retval); // write back to prefs
				break; // end while loop
			}
			c = *++name;
		}
	}
  return rv;
}
NS_IMETHODIMP
nsMsgIncomingServer::SetHostName(const char *chvalue)
{
  nsresult rv = SetCharValue("hostname", chvalue);
  return rv;
}
Looks like another cast is needed for systems using signed chars:
c = (unsigned char) *++name;
this looks great...
minor comments:
1) why use make c an integer and then cast? how about just making it a char,
thus making it match whatever signedness the platform is using (also allowing
the compiler to make extra optimizations since you're no longer changing the
storage class of the value in *name when you transfer it's value into c)
2) we should try interpreting the port number after the colon, and calling
SetPort() with it.
3) in SetHostName, I think similar logic should be used to what you wrote in
GetHostName - so that if I call SetHostName("hostname:40") that only "hostname"
actually gets written to the pref, (and SetPort() is also called)
1) the 'int' is just long habit; char seems to do fine.
2) David Bienvenu agreed if a port number was present, it should be a default 
which gets overridden by a separate port number pref; so automatically calling 
SetPort() here would instead give precedence to this port number, right?
3) I was going to do something similar in SetHostName(), but then I noticed the 
argument was const, and I didn't want to cast away const or allocate another 
string (we do that way too much by the way).  I figured if getting side managed 
to put things to rights eventually, that might be enough.
2) Yeah, that's true. I can go either way on this.
3) you are very right about the const char thing... but it does seem like at
this point, setting the hostname to "abc:40" _should_ override the port
preference, so we should just go ahead and set it. (otherwise, setting the port
to abc:40 would pretty much be exactly like setting it to abc)
2) maybe handle the port number in 3) below instead
3) if the string contains a colon, then make a stack-based copy if small enough, 
or go to heap if too big; is there a limit on how long server names get?  I know 
there's probably a perfect string class for dividing the stack vs. heap aspect, 
but I don't pay any attention to the changing string architectures. Anything 
shoulds fine to me, and I can easily do it by hand in several lines of code.
nsCAutoString is a stack-based class that goes to the heap with strings over 64
bytes, without you having to think about it. 
Usage is trivial
nsCAutoString myStr(someConstCharString);
myStr.Truncate(somePosition);

I don't believe there is a limit, or at least that we should impose one.
Let's use nsCAutoString, then. Tomorrow I'll try a new version in my tree.
these are mine now, I guess.
Assignee: davidmc → bienvenu
Alec, do you have this patch in your tree to check in? If not, please assign 
back to me.
Assignee: bienvenu → alecf
I don't....
Assignee: alecf → bienvenu
moving to m17
Target Milestone: M15 → M17
accepting
Status: NEW → ASSIGNED
Mail Review recommends this bug as beta2 stopper.  Adding nsbeta2 keyword.
Keywords: nsbeta2
I'm just going to fix this and get it over with :)
reassigning to me, I have a fix.
Assignee: bienvenu → alecf
Status: ASSIGNED → NEW
Whiteboard: fix in hand
Status: NEW → ASSIGNED
ok, fix is in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Alec, how do I test this.  
1.) Should I create a New account and give a bogus server name w/port like 
"nsmail:40" in the field for POP server name?   
2.) OR should I edit the prefs.js file and change the port to "40" (40 is just a 
random number)?

I did both of these and nothing bad happened, with the 1st one, the prefs.js 
file has the port string (which is a bogus port), but it got my mail anyway as 
did scenario 2.

If the port is bogus do we just ignore the bogus port and go to the default.
If the port is not bogus do we use the entered port number?  

Let me know.
sorry for the delay. Talked to esther in the hallway:
- it should not crash
- if you put "hostname:9999" in your prefs as the hostname for a particular
server, you should be able to start mail and your prefs should now have
seperated the hostname and port out into "hostname" and 9999
- if you enter "hostname:9999" in the account wizard, your prefs should have
similar results.
Verified.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.