Mark Banner (:standard8) Reporter
	Description • 17 years ago

In our news, pop and smtp implementations we have various calls to nsMsgProtocol::OpenNetworkSocketWithInfo, these can be replaced by nsMsgProtocol::OpenNetworkSocket.

This replacement will reduce duplicated code (getting host/ports etc), and also means that the calls to NS_ExamineForProxy can be dropped which will help the move to the frozen API.

Depends on bug 419591 as we need to ensure OpenNetworkSocket is implemented  before we do this replacement.

	Serge Gautherie (:sgautherie) Assignee
	Comment 1 • 16 years ago

<http://mxr.mozilla.org/seamonkey/search?string=OpenNetworkSocketWithInfo&case=on&find=%2Fmailnews%2F&filter=%5E%5B%5E%5C0%5D*%24&tree=seamonkey>
{{
Found 6 matching lines in 4 files
}}

	Serge Gautherie (:sgautherie) Assignee
	Comment 2 • 16 years ago

Updated (to Hg) search:
http://mxr.mozilla.org/comm-central/search?string=OpenNetworkSocketWithInfo&case=on&find=%2Fmailnews%2F.*%2Fns

	Serge Gautherie (:sgautherie) Assignee
	Comment 3 • 16 years ago

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20081127 SeaMonkey/2.0a2pre] (home, debug default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/28a7fa014b03
+http://hg.mozilla.org/comm-central/rev/3c516d1c1248)

Looks rather straightforward.

	David :Bienvenu
	Comment 4 • 16 years ago

Comment on attachment 350360 [details] [diff] [review]
(Av1) Replace the callers

this patch has bit-rotted in news, but more importantly, it broke pop3 download for me for one of my accounts, so I'm going to have to minus it. With the patch, I get a cert error, and I don't get the option to override it. w/o the patch, I can read mail fine. So this needs some investigation.

	Serge Gautherie (:sgautherie) Assignee
	Comment 5 • 16 years ago

Av1, with un-bit-rotted News part after bug 463096.

Do you have steps or a test(case) for the Pop3 issue ?

	David :Bienvenu
	Comment 6 • 16 years ago

my att&t pop account no longer gets new mail with your patch, when configured to use ssl. So I really can't approve that.

	Mark Banner (:standard8) Reporter
	Comment 7 • 16 years ago

(In reply to comment #5)
> Do you have steps or a test(case) for the Pop3 issue ?

I've not tried it but I may be able to guess. Set up a new pop account with a
hostname that's incorrect or something. Then, set the hostname to the correct
address. Then with the patch you will fail - the reason being the RealHostName
"translation".

The news one will fail in the same way as well.

	David :Bienvenu
	Comment 8 • 16 years ago

Comment on attachment 354311 [details] [diff] [review]
(Av1a) Replace the callers

if this is simply un-bitrotting the old patch, I have to minus it...standard8's idea sounds right, though - realhostname is different than hostname for the account that's having the problem.

	Mark Banner (:standard8) Reporter
	Comment 9 • 16 years ago

I've just attached a couple of unit tests to bug 470973 (that I wanted for password manager), but should help check against regressions with this bug.

This bug also doesn't need to block bug 433316.

	Serge Gautherie (:sgautherie) Assignee
	Comment 10 • 16 years ago

(In reply to comment #7)

I confirm these steps (with a Newsgroup account).
prefs.js ends up with
{
user_pref("mail.server.server2.hostname", "initialHostName");
user_pref("mail.server.server2.realhostname", "currentHostName");
}
Though the error dialog displays |.realhostname|, 'Subscribe' actually tried to connect to |.hostname|.

	Serge Gautherie (:sgautherie) Assignee
	Comment 11 • 16 years ago

So, it seems I trusted comment 0 and removed host, port and proxy, from the callers, without checking _all_ the details.

1 - Port

NNTP used |m_url|; Pop3 and |OpenNetworkSocket()| use |aURL|.
Does it matter ?

http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgProtocol.cpp#165

2 - (Real)HostName

I would need to re-add something like
{
    nsCAutoString realHostName;
    server->GetRealHostName(realHostName);
    NS_ENSURE_SUCCESS(rv, rv);
    aURL->SetHost(realHostName);
    NS_ENSURE_SUCCESS(rv, rv);
}
Maybe duplicating |aURL|, if it is not safe to modify its |Host| ?

http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/src/nsStandardURL.cpp#1355

3 - Proxy

It looks like I should not remove the |NS_ExamineForProxy()| call,
but instead "move" it to |OpenNetworkSocket()|, which currently handles "smtp" only ?

http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/public/nsNetUtil.h#772

	neil@parkwaycc.co.uk
	Comment 12 • 13 years ago

Note that NS_ExamineForProxy has been replaced by MsgExamineForProxy.