419592 - Replace news/pop/smtp calls to nsMsgProtocol::OpenNetworkSocketWithInfo with nsMsgProtocol::OpenNetworkSocket

Status: RESOLVED WORKSFORME

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Serge Gautherie (:sgautherie)]

<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
}}

Comment 2

[Serge Gautherie (:sgautherie)]

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

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Replace the callers

[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.

Comment 4

[David :Bienvenu]

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.

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) Replace the callers

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

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

Comment 6

[David :Bienvenu]

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.

Comment 7

[Mark Banner (:standard8)]

(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.

Comment 8

[David :Bienvenu]

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.

Comment 9

[Mark Banner (:standard8)]

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.

Comment 10

[Serge Gautherie (:sgautherie)]

(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|.

Comment 11

[Serge Gautherie (:sgautherie)]

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

Comment 12

[[e-mail]]

Note that NS_ExamineForProxy has been replaced by MsgExamineForProxy.

Comment 13

[Wayne Mery (:wsmwk)]

This has a TM set, but was something even checked in?

Comment 14

[Magnus Melin [:mkmelin]]

I don't know. But nsMsgProtocol::OpenNetworkSocket doesn't exist anymore. So probably not worth keeping open.