474434 - Active ftp sessions are not closed when clearing recent history

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Bjarne (:bjarne)]

I cannot see that established ftp-connections are closed when the user choses "Clear Recent History" -> "Active Logins". To reproduce :

1) log on to an ftp-server which requires authentication
2) do not let FF remember the username and password
3) go to Tools->"Clear Recent History" , enable "Active Logins" , click "Ok"
4) shift-reload the page with the ftp-listing

Expected result : the popup asking for username and password should appear

Actual result : the page with the ftp-listing is refreshed (active connection is re-used)

Comment 1

[Michal Novotny [:michal]]

Attachment: patch v1

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 362903 [details] [diff] [review]
patch v1

This seems ok, but call the variable mSessionId (without the x) and the method GetSessionId()?

Also, it might be good to have the notification be net:clear-active-logins and just have the HTTP code use the same notification, no?

Comment 3

[Michal Novotny [:michal]]

(In reply to comment #2)
> (From update of attachment 362903 [details] [diff] [review])
> This seems ok, but call the variable mSessionId (without the x) and the method
> GetSessionId()?

Idx is shortened "index" but you're right. Id is better.

> Also, it might be good to have the notification be net:clear-active-logins and
> just have the HTTP code use the same notification, no?

Good idea, I'll do it.

Comment 4

[Michal Novotny [:michal]]

Attachment: patch v2

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 363039 [details] [diff] [review]
patch v2

+        // notify listeners, currently nsHttpHandler and nsFtpProtocolHandler
+        // observe this topic

you shouldn't try to have a complete list of listeners here... just mention that at least HTTP and FTP observe it. otherwise the list has a good chance of getting out of date.

+++ b/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp
+    mControlConnection->mSessionId = gFtpHandler->GetSessionId();

I think it would be better to initialize this in the constructor instead, so that the caller doesn't have to worry about it.

+                mControlConnection->mSessionId = gFtpHandler->GetSessionId();

why do you need this? It seems that if the session ID changed since the control connection was established, then the connection was deleted, so you wouldn't get it here.

Comment 6

[Michal Novotny [:michal]]

Attachment: patch v3

(In reply to comment #5)
> +                mControlConnection->mSessionId = gFtpHandler->GetSessionId();
> 
> why do you need this? It seems that if the session ID changed since the control
> connection was established, then the connection was deleted, so you wouldn't
> get it here.

You're right, this is unnecessary.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 363167 [details] [diff] [review]
patch v3

Does not apply:
{
patching file netwerk/protocol/ftp/src/nsFtpProtocolHandler.cpp
Hunk #4 FAILED at 378
1 out of 4 hunks FAILED
patching file netwerk/protocol/ftp/src/nsFtpProtocolHandler.h
Hunk #2 FAILED at 92
1 out of 2 hunks FAILED
}

Comment 8

[Michal Novotny [:michal]]

Attachment: patch against rev. b84ee6f45b08

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 363665 [details] [diff] [review]
patch against rev. b84ee6f45b08

I meant making the constructor something like:
+nsFtpControlConnection::nsFtpControlConnection(const nsCSubstring& host, PRUint32 port)
+    : mServerType(0), mSessionId(gFtpHandler->SesssionId()), mHost(host), mPort(port)

But your way works too. (Is your version of the line really below 80 characters?)

Comment 10

[Michal Novotny [:michal]]

Attachment: patch v5, changed according to comment 9 [Checkin: Comment 11]

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 363688 [details] [diff] [review]
patch v5, changed according to comment 9
[Checkin: Comment 11]


http://hg.mozilla.org/mozilla-central/rev/a99b6912bbe8