473371 - Make FTP also honor LOAD_ANONYMOUS flag

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Bjarne (:bjarne)]

This is a spin-off from bug #466080 to address the ftp-case.

Comment 1

[Bjarne (:bjarne)]

I've spent some time looking at this and know some places where the code needs changes. However, I cannot come up with any reasonable use of LOAD_ANONYMOUS for ftp. Probably just me being stupid, but could someone suggest a reasonable way to apply LOAD_ANONYMOUS to a ftp-request (or maybe even supply a testcase)? Moreover - what would be reasons for doing marking an ftp-request with LOAD_ANONYMOUS?

Anyway, even though I don't really see the use of LOAD_ANONYMOUS for ftp, here are some results from experimenting with this and current thinking :

- Using XHR to load over ftp seems to always abort with a CheckSameOriginError. This is reasonable, even though I don't really know if the abort is intentional or just a side-effect of something else. Maybe it doesn't matter...  I've loaded html/JS over http and tried XHR/ftp back to the same server, and I've loaded html/JS from a local file and tried XHR/ftp to some server. Both attempts fail. Current thinking is to leave this behaviour as it is.

- An ftp-request with LOAD_ANONYMOUS set should not use a cached result from a request with LOAD_ANONYMOUS not set. Moreover, it looks like ftp results are cached rather randomly (e.g. bug #437515), and afaik there is no cache-specification for ftp. Current thinking is to cache anonymous requests and non-anonymous requests separately like it's done for XHR and for ssl-sessions (and deal with remaining coherence-issues in other bugs). A tempting alternative is of-course to drop caching of ftp-requests completely.

- Clearing active/authenticated sessions do not seem to work for ftp. Even when clearing BOTH active/authenticated sessions as well as stored passwords, then doing shift-reload, the browser reloads from the server with no questions asked. Current thinking is that this must be fixed independently of LOAD_ANONYMOUS, moreover that a request with LOAD_ANONYMOUS set should never re-use existing sessions.

- FireFTP implements secure ftp and we probably need to handle LOAD_ANONYMOUS at the SSL-level also for ftp connections. Current thinking is that this gets me back to the issue mentioned first in this comment : I don't see a reasonable use of LOAD_ANONYMOUS, and I have trouble whipping up a testcase.

- Finally, there is the possibility to encode ftp username and password into the URL. Current thinking is that I have no clue how we should deal with this when LOAD_ANONYMOUS is set...

Comment 2

[Bjarne (:bjarne)]

(In reply to comment #1)
> - Using XHR to load over ftp seems to always abort with a CheckSameOriginError.
> This is reasonable, even though I don't really know if the abort is intentional
> or just a side-effect of something else. Maybe it doesn't matter...  I've
> loaded html/JS over http and tried XHR/ftp back to the same server, and I've
> loaded html/JS from a local file and tried XHR/ftp to some server. Both
> attempts fail. Current thinking is to leave this behaviour as it is.

Hmmm...  just realized that I should repeat this with proper chrome-privileges. Will do that asap.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This wouldn't be for use with XHR. As you say XS-XHR will always abort with other errors before we get to even open an FTP request.

This is solely for extensions to use if they want to pull down data and be sure they aren't getting user private data. It's always hard to imagine what extensions will want to do, the best we can do is to provide solid APIs and then see what turns out

Comment 4

[Bjarne (:bjarne)]

Ok. So, if I understand this correctly : Expected usage is that chrome-scripts establish ftp connections using Components.classes("@mozilla.org/network/io-service;1") , setting LOAD_ANONYMOUS in order to not leak user-credentials?

Do we automatically set LOAD_ANONYMOUS if the connection is set up like this, or is this the responsibility of the extension? (Not pertinent from the perspective of honoring the flag, but I'm just curious...)

Also, do you have comments to the other items from comment #0? In particular about caching, active sessions and username/pw encoded in the url?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Heh, was already writing a longer reply :)

> - Using XHR to load over ftp seems to always abort with a CheckSameOriginError.
> This is reasonable, even though I don't really know if the abort is intentional
> or just a side-effect of something else. Maybe it doesn't matter...  I've
> loaded html/JS over http and tried XHR/ftp back to the same server, and I've
> loaded html/JS from a local file and tried XHR/ftp to some server. Both
> attempts fail. Current thinking is to leave this behaviour as it is.

Yes, the Access-Control spec that XHR is using for cross-site requests is defined only to work over http/https at this point. Unless the spec changes we should leave things as is.

> - An ftp-request with LOAD_ANONYMOUS set should not use a cached result from a
> request with LOAD_ANONYMOUS not set. Moreover, it looks like ftp results are
> cached rather randomly (e.g. bug #437515), and afaik there is no
> cache-specification for ftp. Current thinking is to cache anonymous requests
> and non-anonymous requests separately like it's done for XHR and for
> ssl-sessions (and deal with remaining coherence-issues in other bugs).

Yup, that's what I did for http requests as well, see
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#1776

> A
> tempting alternative is of-course to drop caching of ftp-requests completely.

I don't have an opinion on this. I do know that Jason Duell is looking into doing some serious rework on our caches though, not sure if that includes the ftp cache or not.

> - Clearing active/authenticated sessions do not seem to work for ftp. Even when
> clearing BOTH active/authenticated sessions as well as stored passwords, then
> doing shift-reload, the browser reloads from the server with no questions
> asked. Current thinking is that this must be fixed independently of
> LOAD_ANONYMOUS, moreover that a request with LOAD_ANONYMOUS set should never
> re-use existing sessions.

What do you mean by "with no questions asked"? But yes, if there is a connection which we have already established credentials on then I assume we can't use it, right? Since ftp credentials are per connection, not per request.

> - FireFTP implements secure ftp and we probably need to handle LOAD_ANONYMOUS
> at the SSL-level also for ftp connections. Current thinking is that this gets
> me back to the issue mentioned first in this comment : I don't see a reasonable
> use of LOAD_ANONYMOUS, and I have trouble whipping up a testcase.

Not follow you here. If FireFTP is the one that has to be responsible for forwarding the LOAD_ANONYMOUS flag to the ssl code, I'm fine with that.

> - Finally, there is the possibility to encode ftp username and password into
> the URL. Current thinking is that I have no clue how we should deal with this
> when LOAD_ANONYMOUS is set...

That's fine. As long as necko doesn't automatically add user credentials to the request then extensions can safely use LOAD_ANONYMOUS to get anonymous requests if they want. Or they could add some other random credentials safely knowing that we won't add the users credentials on top of them or some such.

(In reply to comment #4)
> Ok. So, if I understand this correctly : Expected usage is that chrome-scripts
> establish ftp connections using
> Components.classes("@mozilla.org/network/io-service;1") , setting
> LOAD_ANONYMOUS in order to not leak user-credentials?

Yup.

> Do we automatically set LOAD_ANONYMOUS if the connection is set up like this,
> or is this the responsibility of the extension? (Not pertinent from the
> perspective of honoring the flag, but I'm just curious...)

For now it'll be the responsibility of the extension. I know there has been discussions about changing the default to be anonymous, but I haven't been part of those discussions.


All that said, supporting FTP isn't super important IMHO, given how rarely FTP is used these days. We could simply make the FTP code refuse to open a connection if the LOAD_ANONYMOUS flag is set, and instead return an NS_ERROR_NOT_IMPLEMENTED error. I actually just realized a much more critical bug in the http implementation of LOAD_ANONYMOUS, I'll file a separate bug on that.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Basically I'd say that if it's easy to make FTP channels support LOAD_ANONYMOUS we should. If it requires a lot of changes then I don't think it's worth it, at least until someone requests it.

Comment 7

[Bjarne (:bjarne)]

(In reply to comment #5)
> Yup, that's what I did for http requests as well, see
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#1776

... and it's done for the ssl-sessions in the patch for bug #466080. :)

> I do know that Jason Duell is looking into
> doing some serious rework on our caches though, not sure if that includes the
> ftp cache or not.

It would be interesting to be in the loop for this one, in particular since I have bug #462243 on my plate. Any particular bug # to be CC'd on?

> What do you mean by "with no questions asked"?

Just "no questions asked" :)

The browser does not ask for username/password. It should definitely ask when reloading the ftp-request after clearing stored passwords *and* active sessions. My guess is that it finds an active session and reuses it, hence clearing active sessions does not seem to have an effect on ftp.

> But yes, if there is a
> connection which we have already established credentials on then I assume we
> can't use it, right? Since ftp credentials are per connection, not per request.

Yup. I think we should make it work like persistent connections for http : A anonymous request should not use a connection made by a non-anonymous request (and probably vice-versa).

> Not follow you here. If FireFTP is the one that has to be responsible for
> forwarding the LOAD_ANONYMOUS flag to the ssl code, I'm fine with that.

You're probably right, assuming FireFTP uses necho for ssl. I'll trace a run with FireFTP next week to see where the ssl comes from. API pass flag must be clear, though, and that would be our responsibility.

> > - Finally, there is the possibility to encode ftp username and password into
> > the URL. Current thinking is that I have no clue how we should deal with this
> > when LOAD_ANONYMOUS is set...
> 
> That's fine. As long as necko doesn't automatically add user credentials to the
> request then extensions can safely use LOAD_ANONYMOUS to get anonymous requests
> if they want. Or they could add some other random credentials safely knowing
> that we won't add the users credentials on top of them or some such.

Well, I was thinking about urls like (not 100% about the syntax - it's almost 2am around here)
"ftp:username:secretpassword//ftp.server.com" where credentials are stored as part of the url (see nsFtpState::Init() in file nsFtpConnectionThread.cpp). The question is whether we should allow these to be used for anonymous ftp-requests. I tend to think we should, but I'm not sure.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #7)
> > I do know that Jason Duell is looking into
> > doing some serious rework on our caches though, not sure if that includes the
> > ftp cache or not.
> 
> It would be interesting to be in the loop for this one, in particular since I
> have bug #462243 on my plate. Any particular bug # to be CC'd on?

Don't think there is one yet. Jason?

> > What do you mean by "with no questions asked"?
> 
> Just "no questions asked" :)
> 
> The browser does not ask for username/password. It should definitely ask when
> reloading the ftp-request after clearing stored passwords *and* active
> sessions. My guess is that it finds an active session and reuses it, hence
> clearing active sessions does not seem to have an effect on ftp.

Ah, I'm with you.


> > Not follow you here. If FireFTP is the one that has to be responsible for
> > forwarding the LOAD_ANONYMOUS flag to the ssl code, I'm fine with that.
> 
> You're probably right, assuming FireFTP uses necho for ssl. I'll trace a run
> with FireFTP next week to see where the ssl comes from. API pass flag must be
> clear, though, and that would be our responsibility.

Sounds good.

> > > - Finally, there is the possibility to encode ftp username and password into
> > > the URL. Current thinking is that I have no clue how we should deal with this
> > > when LOAD_ANONYMOUS is set...
> > 
> > That's fine. As long as necko doesn't automatically add user credentials to the
> > request then extensions can safely use LOAD_ANONYMOUS to get anonymous requests
> > if they want. Or they could add some other random credentials safely knowing
> > that we won't add the users credentials on top of them or some such.
> 
> Well, I was thinking about urls like (not 100% about the syntax - it's almost
> 2am around here)
> "ftp:username:secretpassword//ftp.server.com" where credentials are stored as
> part of the url (see nsFtpState::Init() in file nsFtpConnectionThread.cpp). The
> question is whether we should allow these to be used for anonymous
> ftp-requests. I tend to think we should, but I'm not sure.

Yes, that's the syntax I was thinking about too. I allow such syntax to be used for the equivalent code in the httpchannel code, but don't allow necko to automatically add username/passwd to the channel. That's what we want for FTP channels too.

Comment 9

[Bjarne (:bjarne)]

> Yes, that's the syntax I was thinking about too. I allow such syntax to be used
> for the equivalent code in the httpchannel code, but don't allow necko to
> automatically add username/passwd to the channel. That's what we want for FTP
> channels too.

Do you think we should let the dialogue pop up in this case?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #9)
> > Yes, that's the syntax I was thinking about too. I allow such syntax to be 
> > used for the equivalent code in the httpchannel code, but don't allow necko
> > to automatically add username/passwd to the channel. That's what we want for
> > FTP channels too.
> 
> Do you think we should let the dialogue pop up in this case?

No, we should use the username/password given in the URL, and if that doesn't work we should fail the connection. If we were to bring up a dialog and ask the user, that would be the browser adding user-specific credentials to the channel, which is what we want to avoid.

Comment 11

[Bjarne (:bjarne)]

Attachment: Initial version

This patch should handle the following differences between anonymous and non-anonymous ftp-requests

1) Content should be cached separately
2) Authenticated sessions should be cached separately
3) There should be no pop-up asking for username/password for anonymous requests

4) Combination of (2) and (3) should ensure that anonymous requests never gets associated un/pw and hence Necko should never be able to apply un/pw automatically

Bug #474434 has been registered to address the issue of active ftp-sessions not being closed when user clears recent history.

Ftp-caching in general seems to be improved a lot since comment #1 (or maybe my understanding has improved). I see no reason for pursuing that issue further.

Remaining :

- Tests. This point needs some more thinking. In particular I want to test (4) above, but as many aspects as possible should be tested.
- I have not yet done anything to the SSL-layer. Need to see what FireFTP does.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 357800 [details] [diff] [review]
Initial version

>From: [e-mail]
>
>Bug 473371 - Make FTP also honor LOAD_ANONYMOUS flag
>
>diff -r 3eb79df3bcb0 netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp
>--- a/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp	Tue Jan 20 00:26:31 2009 +1300
>+++ b/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp	Tue Jan 20 16:31:53 2009 +0100
>@@ -254,17 +254,17 @@ nsFtpState::EstablishControlConnection()
>     NS_ASSERTION(!mControlConnection, "we already have a control connection");
>             
>     nsresult rv;
> 
>     LOG(("FTP:(%x) trying cached control\n", this));
>         
>     // Look to see if we can use a cached control connection:
>     nsFtpControlConnection *connection = nsnull;
>-    gFtpHandler->RemoveConnection(mChannel->URI(), &connection);
>+    gFtpHandler->RemoveConnection(GenerateCacheKey(mChannel), &connection);
> 
>     if (connection) {
>         mControlConnection.swap(connection);
>         if (mControlConnection->IsAlive())
>         {
>             // set stream listener of the control connection to be us.        
>             mControlConnection->WaitData(this);
>             
>@@ -656,17 +656,18 @@ nsFtpState::S_user() {
>         return NS_ERROR_FAILURE;
> 
>     nsresult rv;
>     nsCAutoString usernameStr("USER ");
> 
>     if (mAnonymous) {
>         usernameStr.AppendLiteral("anonymous");
>     } else {
>-        if (mUsername.IsEmpty()) {
>+        // User-dialogue to get username, avoid for anonymous requests (bug #473371)
>+        if (mUsername.IsEmpty() && !mChannel->HasLoadFlag(nsIRequest::LOAD_ANONYMOUS)) {

If the username is empty we should with NS_ERROR_FAILURE. That's what the current code does if the user doesn't supply a username.

>+    /**
>+     * Generates a cache-key from the URI in combination with the
>+     * LOAD_ANONYMOUD flag from the channel. See also bug #473371.
>+     */
>+    nsCAutoString GenerateCacheKey(nsFtpChannel *mChannel);

Don't return by value for strings, that's pretty expensive. Use the following signature intead:

void GenerateCacheKey(nsFTPChannel* mChannel, nsACString& aKey);

though you might event want to return an nsresult and return an error if GetPrePath fails.

>+nsFtpProtocolHandler::RemoveConnection(nsCAutoString aKey, nsFtpControlConnection* *_retval)
> {
>     NS_ASSERTION(_retval, "null pointer");
>-    NS_ASSERTION(aKey, "null pointer");
>+//    NS_ASSERTION(aKey, "null pointer");

Assert that it's non-empty instead?

>+nsFtpProtocolHandler::InsertConnection(nsCAutoString aKey, nsFtpControlConnection *aConn)
> {
>     NS_ASSERTION(aConn, "null pointer");
>-    NS_ASSERTION(aKey, "null pointer");
>+//    NS_ASSERTION(aKey, "null pointer");

Same here.


>     // FTP Connection list access
>-    nsresult InsertConnection(nsIURI *aKey, nsFtpControlConnection *aConn);
>-    nsresult RemoveConnection(nsIURI *aKey, nsFtpControlConnection **aConn);
>+    nsresult InsertConnection(nsCAutoString aKey, nsFtpControlConnection *aConn);
>+    nsresult RemoveConnection(nsCAutoString aKey, nsFtpControlConnection **aConn);

Don't pass by value here either. Use nsACString& or nsCString&.

looks good otherwise.

/ Jonas

Comment 13

[Bjarne (:bjarne)]

Attachment: New version honoring comments from review

Here we go again. I'll try to add a test tomorrow.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 357836 [details] [diff] [review]
New version honoring comments from review

Is it expected that GenerateCacheKey can return an error that we really should ignore? Don't you want to keep 'throwing' by adding an NS_ENSURE_SUCCESS(rv, rv) after each call?

Comment 15

[Bjarne (:bjarne)]

Attachment: Now even with proper error-handling...

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 357868 [details] [diff] [review]
Now even with proper error-handling...

Looks great

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 357868 [details] [diff] [review]
Now even with proper error-handling...

Ugh, sorry, should have been more thorough in my review.

Don't you need to make a similar fix to S_pass as you've done to S_user?

Also, I was looking for code that saves the username/password when once the connection is successful, since that part should not happen for anonymous requests. But I couldn't find where that happens, if it happens.

Comment 18

[Bjarne (:bjarne)]

Attachment: Taking latest comments into account

(In reply to comment #17)
> Don't you need to make a similar fix to S_pass as you've done to S_user?

Uhmm...  just making sure you're paying attention...  :}

(fixed)

> Also, I was looking for code that saves the username/password when once the
> connection is successful, since that part should not happen for anonymous
> requests. But I couldn't find where that happens, if it happens.

The un/pw pair is kept in private fields nsFtpState::mUsername and nsFtpState::mPassword resp. These are sent to the server in S_user() and S_pass(), and they can be set in 3 ways (as far as I can see)

1) From a prompt in S_user() and/or S_pass() connected to a nsAuthInformationHolder-object. As far as I can understand, it's the nsAuthInformationHolder-object that takes care of storing un/pw. The current patch exits before any of this is done in the LOAD_ANONYMOUS case, preventing both store and restore of un/pw.

2) From nsFtpState::EstablishControlConnection() if a cached, active session is found. nsFtpState::KillControlConnection() stores mPassword in a nsFtpControlConnection-object which represents the active session being cached and reused. Oddly enough, mUsername is not cached by this mechanism. I'm pretty sure this whole mechanism is redundant and can be removed, but the patch just prevents storing and restoring mPassword in the LOAD_ANONYMOUS case (probably it would be sufficient to prevent storing, but it doesn't hurt to paranoid in this situation...).

3) From un/pw encoded in the URL. (This seems to be the only valid way to log onto an ftp-server with credentials and LOAD_ANONYMOUS set in fact.)

Comment 19

[Bjarne (:bjarne)]

Something just occurred to me...

Consider the statement from previous comment (3) that the only valid ftp-requests with LOAD_ANONYMOUS set are those with un/pw embedded into the URL : What if we say that ftp-requests with LOAD_ANONYMOUS set never re-use active connections? This would work transparently since un/pw has to be embedded into the URL in any case. The penalty is extra traffic and load on the server, but the patch would be a lot simpler. Should I give that approach a shot?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That sounds like a good idea to me.

Comment 21

[Bjarne (:bjarne)]

Attachment: WIP for latest approach

This is definitely a log simpler and I think it does what we want. Comments are appreciated.

Comment 22

[Bjarne (:bjarne)]

Attachment: V1.0 (now even compiling!) applying latest strategy

The patch prevents user-prompts and re-use of cached/active sessions for requests with LOAD_ANONYMOUS set. Thus, username/password can never be applied by necko for such requests. (We're not considering anonymous-ftp here, but ftp-accounts which requires username and password.)

A consequence of doing this is that the only LOAD_ANONYMOUS requests which makes sense are those with username/password embedded into the URL. Or put the other way : Ftp requests with LOAD_ANONYMOUS set must embed username/password in the URL to succeed. Such requests will do a full ftp login on the server every time, since active sessions are not re-used, but this performance-loss can be dealt with later if necessary.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 358224 [details] [diff] [review]
V1.0 (now even compiling!) applying latest strategy

Jason should look at this one too.

Comment 24

[Jason Duell]

Comment on attachment 358224 [details] [diff] [review]
V1.0 (now even compiling!) applying latest strategy

I know very little about how the FTP stuff works, but this patch looks very straightforward, so I'm approving it.   Have you tested it?

Comment 25

[Bjarne (:bjarne)]

(In reply to comment #24)
> (From update of attachment 358224 [details] [diff] [review])
> I know very little about how the FTP stuff works, but this patch looks very
> straightforward, so I'm approving it.   Have you tested it?

It has been tested manually, yes. We have no ftp-server (afaik) that can be used with mochitests or xpcshell, so there is no automated test.

Comment 26

[Jeff Walden [:Waldo]]

(In reply to comment #25)
> We have no ftp-server (afaik) that can be used with mochitests or xpcshell,
> so there is no automated test.

Correctomundo.  Maybe one of these days maybe I'll write one to have a chance to read up on FTP, but I have a number of other projects I want to get to first, so it's a back-burner project.

Comment 27

[Bjarne (:bjarne)]

I was playing with the idea of using pyftpdlib for this. Do you think it's worth it to have automated tests for ftp? I.e. is native ftp-support in FF considered important enough to spend time and efforts to set up automated testing for ftp?

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 358224 [details] [diff] [review]
V1.0 (now even compiling!) applying latest strategy

>@@ -2142,16 +2158,19 @@ nsFtpState::CheckCache()
>     }
> 
>     // Generate cache key (remove trailing #ref if any):
>     nsCAutoString key;
>     mChannel->URI()->GetAsciiSpec(key);
>     PRInt32 pos = key.RFindChar('#');
>     if (pos != kNotFound)
>         key.Truncate(pos);
>+    if (mChannel->HasLoadFlag(nsIRequest::LOAD_ANONYMOUS))
>+        key.Insert("anon&", 0);
>+

What is the purpose of these lines? It looks like we're never attempting to cache anonymous channels, so is this needed at all?

Alternatively, if we do this change to nsFtpState::CheckCache, could we remove the changes not to insert the connection into the cache? So that anonymous channels would in fact get cached, just separately from non-anonymous channels?

Comment 29

[Bjarne (:bjarne)]

(In reply to comment #28)
> (From update of attachment 358224 [details] [diff] [review])
> >@@ -2142,16 +2158,19 @@ nsFtpState::CheckCache()
> >     }
> > 
> >     // Generate cache key (remove trailing #ref if any):
> >     nsCAutoString key;
> >     mChannel->URI()->GetAsciiSpec(key);
> >     PRInt32 pos = key.RFindChar('#');
> >     if (pos != kNotFound)
> >         key.Truncate(pos);
> >+    if (mChannel->HasLoadFlag(nsIRequest::LOAD_ANONYMOUS))
> >+        key.Insert("anon&", 0);
> >+
> 
> What is the purpose of these lines? It looks like we're never attempting to
> cache anonymous channels, so is this needed at all?

Looks like leftovers from the first approach where we cached anonymous channels as well as the "normal" ones. See comment #19. The lines can be removed.

> Alternatively, if we do this change to nsFtpState::CheckCache, could we remove
> the changes not to insert the connection into the cache? So that anonymous
> channels would in fact get cached, just separately from non-anonymous channels?

This sounds like the first approach. See comment #19 where the current approach is advocated. On the rare occasions where I can imagine anonymous ftp-requests being used, username/password have to be embedded in the URL in any case (see comment #19), so there is very little to gain from caching the channel. I think this reasoning is sound and it allows the code to be *a lot* simpler.

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 358224 [details] [diff] [review]
V1.0 (now even compiling!) applying latest strategy

Right. Technically we could reuse LOAD_ANONYMOUS connections that used ftp-anonymous username/passwd, i.e. has mAnonymous set (probably the most common case).

However if anyone cares we can always do that later.

sr=me with the change in nsFtpState::CheckCache removed.

Comment 31

[Bjarne (:bjarne)]

Attachment: V1.1 honoring last comment from reviewer

Comment 32

[Bjarne (:bjarne)]

Requesting check-in

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 364778 [details] [diff] [review]
V1.1 honoring last comment from reviewer

Does not apply
{
patching file netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp
Hunk #2 FAILED at 658
1 out of 4 hunks FAILED
}

Comment 34

[Bjarne (:bjarne)]

Attachment: Updated V1.1 which applies to latest code from trunk [Checkin: Comment 35]

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 364793 [details] [diff] [review]
Updated V1.1 which applies to latest code from trunk
[Checkin: Comment 35]


http://hg.mozilla.org/mozilla-central/rev/00bae8a117f6

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 364793 [details] [diff] [review]
Updated V1.1 which applies to latest code from trunk
[Checkin: Comment 35]

Very low risk here since none of our code will trigger this code. However it's something that extension authors may need.

Comment 37

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2257314efb13