If you specify an FTP directory URL without including the trailing slash, the browser correctly displays the directory just as if the trailing slash were present. However it does not correct the URL by appending the slash. This used to be OK because it ensured that all the links in the resultant directory listing would work correctly. Unfortunately, now that bug 24867 has implemented the "Upload File" menu choice for FTP directories this does not work quite so well. If the trailing slash is omitted, the File Upload code ends up stripping the last component of the path when determining the base URI. This results in the file being stored one directory level higher that what the user intended. This could result in data loss by inadvertently overwriting a file in the higher level directory, or a security issue if the file being uploaded is intended to be private, and is intended to be stored in a directory with proper access controls, but ends up in a publicly accessible area. Bug 37102 has a lot of discussion about why the slash cannot be added in all cases, but I would second the question in this comment. https://bugzilla.mozilla.org/show_bug.cgi?id=37102#c13 Bug 37102 also seems to be trying to blame this on the server, but that makes little sense, because for some reason, clicking on Reload results in a correct URL including the trailing slash. Clicking on reload should not be sending anything different than the original request did to the server and it seems to work in the reload case. So, it would appear that Mozilla handles this condition correctly on a reload, but fails to on the initial load.

It would appear this code http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp#824 is intended to handle this case, but it is never reached during the initial load.

Wanted, but don't have an owner yet. Bill, are you willing to take it?

If i can narrow down any where the issue actually occurs I will probably take it. Right now I am not sure I know enough and taking the bug probably means no one else will look at it, although I am not sure who else would at this point.

There are 2 issues here. This patch only fixes the first one. The code here http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/nsIndexedToHTML.cpp#167 appends the '/' to baseUri and to uri, but fails to do so to titleUri unless there a password is specified in the URI. This patch fixes that issue. The second issue seems to be that although uri is updated, when the File Upload code does a GetCurrentURI, it does not get the updated value. It would seem that this code needs to call set the current URI to uri after it is updated if it is too work as expected. I really don't know how to accomplish this from this routine, nor am I at all sure doing this from a stream converter is really desirable, however since this is where the code seems to be that is attempting to fix the URI by appending the slash, it would appear to be the only place this can be done without rewriting a lot of other code.

I had previously determined that forcing a page reload before processing the file upload "fixed" this issue, but did not really think that approach was even wallpaper patchworthy. So, I looked into that further and determined that although the URI in currentURI is missing the final '/', the URI in the sessionHistory entry is not. This patch works around the issue in the front-end code by trying the URI from session history if the current URI does not end with a '/' character. This is obviously not the ideal fix, but I think something needs to be done as the current behavior of string files one level up in the directory hierarchy than intended on the FTP server can result in accidental permanent data loss, and I don't see a back-end fix coming before 1.9 beta.

Comment on attachment 280277 [details] [diff] [review] workaround I am not a reviewer for this code, nor knowledgable about it.

I'm not familiar with this code, sorry.

The fact that a different URI is going into session history is suspicious...

(In reply to comment #8) > The fact that a different URI is going into session history is suspicious... > Kind of what I thought. I would have expected the URI going to session history to be copied from the one in currentURI. Obviously that is no the case.

So... When you're in the code you're modifying, are the channel's mURI and mOriginalURI the same object? The current URI comes from mOriginalURI in a lot of cases.

(In reply to comment #10) > So... When you're in the code you're modifying, are the channel's mURI and > mOriginalURI the same object? The current URI comes from mOriginalURI in a lot > of cases. > The existing trunk code ries to fix this by appending the '/' here: http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/nsIndexedToHTML.cpp#167 At this point, the channels mURI and mOriginalURI are the same object. So, it would appear that currentURI must be set before the stream converter is run and the session history mst be saved after the streamconverter modifies the URI path. So, I guess the fix would be to add code here to modify currentURI as well. I have no idea how to get a pointer to currentURI from within the stream converter though.

> At this point, the channels mURI and mOriginalURI are the same object. OK. That makes sense. > it would appear that currentURI must be set before the stream converter is run That doesn't make sense, though. This code is running way before anyone in Gecko ever hears about this load, except via progress notifications, perhaps. And those shouldn't affect the currentURI... Just to make sure. You're talking about the currentURI of the nsIWebNavigation? > the session history must be saved after the streamconverter modifies the > URI path That sounds right, yes.

(In reply to comment #12) > And those shouldn't affect the currentURI... Just to make sure. You're > talking about the currentURI of the nsIWebNavigation? Yes. That is what is referenced here which ends up not having the trailing slash http://lxr.mozilla.org/seamonkey/source/suite/browser/navigator.js#2520

Want to breakpoint in nsDocShell::SetCurrentURI and see who's setting it and where they get it?

OK, I added some debug printfs and currentURI is definitely being set before the stream converter adds the slash. Here is the traceback from when the URI is set. #9 0xf6696a7f in nsDocShell::SetCurrentURI (this=0x8cd5998, aURI=0x95b8a30, aRequest=0x9513b38, aFireOnLocationChange=0) at /home/wag/mozilla/trunk/docshell/base/nsDocShell.cpp:1184 #10 0xf6697431 in nsDocShell::OnNewURI (this=0x8cd5998, aURI=0x95b8a30, aChannel=0x9513b38, aLoadType=1, aFireOnLocationChange=0, aAddToGlobalHistory=1) at /home/wag/mozilla/trunk/docshell/base/nsDocShell.cpp:7702 #11 0xf66975cb in nsDocShell::OnLoadingSite (this=0x8cd5998, aChannel=0x9513b38, aFireOnLocationChange=0, aAddToGlobalHistory=1) at /home/wag/mozilla/trunk/docshell/base/nsDocShell.cpp:7726 #12 0xf6697850 in nsDocShell::CreateContentViewer (this=0x8cd5998, aContentType=0x9513bd8 "application/http-index-format", request=0x9513b38, aContentHandler=0x9513e08) at /home/wag/mozilla/trunk/docshell/base/nsDocShell.cpp:5849 #13 0xf66c2da3 in nsDSURIContentListener::DoContent (this=0x8cd2798, aContentType=0x9513bd8 "application/http-index-format", aIsContentPreferred=0, request=0x9513b38, aContentHandler=0x9513e08, aAbortProcess=0xffa7b410) at /home/wag/mozilla/trunk/docshell/base/nsDSURIContentListener.cpp:138 #14 0xf66cc810 in nsDocumentOpenInfo::TryContentListener (this=0x9513df8, aListener=0x8cd2798, aChannel=0x9513b38) at /home/wag/mozilla/trunk/uriloader/base/nsURILoader.cpp:735 #15 0xf66cd054 in nsDocumentOpenInfo::DispatchContent (this=0x9513df8, request=0x9513b38, aCtxt=0x0) at /home/wag/mozilla/trunk/uriloader/base/nsURILoader.cpp:434 #16 0xf66cde41 in nsDocumentOpenInfo::OnStartRequest (this=0x9513df8, request=0x9513b38, aCtxt=0x0) at /home/wag/mozilla/trunk/uriloader/base/nsURILoader.cpp:280 #17 0xf76d58d4 in nsFTPDirListingConv::OnStartRequest (this=0x95eed50, request=0x9513b38, ctxt=0x0) at /home/wag/mozilla/trunk/netwerk/streamconv/converters/nsFTPDirListingConv.cpp:209 #18 0xf76a137a in nsStreamListenerTee::OnStartRequest (this=0x9673978, request=0x9513b38, context=0x0) at /home/wag/mozilla/trunk/netwerk/base/src/nsStreamListenerTee.cpp:50 #19 0xf76597b9 in nsBaseChannel::OnStartRequest (this=0x9513b08, request=0x958ba80, ctxt=0x0) at /home/wag/mozilla/trunk/netwerk/base/src/nsBaseChannel.cpp:604 #20 0xf766c6df in nsInputStreamPump::OnStateStart (this=0x958ba80) at /home/wag/mozilla/trunk/netwerk/base/src/nsInputStreamPump.cpp:439 #21 0xf766d4bb in nsInputStreamPump::OnInputStreamReady (this=0x958ba80, stream=0x958b948) at /home/wag/mozilla/trunk/netwerk/base/src/nsInputStreamPump.cpp:395 #22 0xf765ee2c in nsBaseContentStream::DispatchCallback (this=0x958b948, async=0) at /home/wag/mozilla/trunk/netwerk/base/src/nsBaseContentStream.cpp:64 #23 0xf77168a5 in nsBaseContentStream::DispatchCallbackSync (this=0x958b948) at /home/wag/mozilla/trunk/netwerk/protocol/gopher/src/../../../base/src/nsBaseContentStream.h:97 #24 0xf7713976 in nsFtpState::OnInputStreamReady (this=0x958b948, aInStream=0x95f06fc) at /home/wag/mozilla/trunk/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp:126 #25 0xf7dace92 in nsInputStreamReadyEvent::Run (this=0x9615c98) at /home/wag/mozilla/trunk/xpcom/io/nsStreamUtils.cpp:111 #26 0xf7ddd6ab in nsThread::ProcessNextEvent (this=0x8081fc8, mayWait=1, result=0xffa7b9c0) at /home/wag/mozilla/trunk/xpcom/threads/nsThread.cpp:490 #27 0xf7d6850b in NS_ProcessNextEvent_P (thread=0x8081fc8, mayWait=1) at nsThreadUtils.cpp:227 #28 0xf71c6d88 in nsBaseAppShell::Run (this=0x84882f0) at /home/wag/mozilla/trunk/widget/src/xpwidgets/nsBaseAppShell.cpp:154 #29 0xf62f115d in nsAppStartup::Run (this=0x84b7d90) at /home/wag/mozilla/trunk/toolkit/components/startup/src/nsAppStartup.cpp:170 #30 0xf7fa407b in XRE_main (argc=1, argv=0xffa7c024, aAppData=0x804c988) at /home/wag/mozilla/trunk/toolkit/xre/nsAppRunner.cpp:3069 #31 0x08048de1 in main (argc=1, argv=0xffa7c024) at /home/wag/mozilla/trunk/browser/app/nsBrowserApp.cpp:153

Oh, man. The directory viewer is actually a document factory? That would do it. I think the right fix is to only have the xpfe/components/build/nsModule.cpp munging of Gecko-Content-Viewers happen when the XUL viewer is enabled (with bonus points for observing the pref and changing the munging stuff). That would mean that if it's disabled we just handle things via the normal stream converter codepath, and then we'll be hitting nsIndexedToHTML::OnStartRequest before we get to nsDSURIContentListener::DoContent. Then we can simplify the code in xpfe/components/directory/nsDirectoryViewer.cpp a bit: if it's being called, then the pref is for the XUL viewer. Not sure about the view-source stuff there, but still.

Well, I tried doing this but ran into an issue almost immediately. I'm not sure what you mean by not doing the munging of Gecko-Content_viewers. Everything I tried in that are resulted in the "The file "<directoryName>" is of type application/http-index-format ..." message pop-up. It would appear there is something else I need to do somewhere to make this mime-type take the normal stream converter codepath. Another think I noticed is that if I try to enable the XUL viewer in Firefox, FTP directory listings are not displayed at all. Is this a bug, or is the XUL directory viewer a seamonkey only thing? My last question is that if I fix this as suggested, that would seem to leave this issue unresolved for the xul directory viewer case. Should I just disable the file upload menu choice if the XUL viewer is enabled?

> It would appear there is something else I need to do somewhere to make this > mime-type take the normal stream converter codepath. Ah, yes. You need to add something like: #define INDEX_TO_UNKNOWN "?from=application/http-index-format&to=*/*" where we define INDEX_TO_HTML and then add 812 { "Indexed to Unknown Converter", 813 NS_NSINDEXEDTOHTMLCONVERTER_CID, 814 NS_ISTREAMCONVERTER_KEY INDEX_TO_UNKNOWN, 815 nsIndexedToHTML::Create 816 }, down where we currently set up the "Indexed to HTML Converter". That should do it, I think. > Another think I noticed is that if I try to enable the XUL viewer in Firefox, I don't think Firefox builds all the right pieces for that viewer. So yes, effectively it's a Seamonkey only thing. > Should I just disable the file upload menu choice if the XUL viewer is > enabled? Given that it's off by default, I think that would be just fine.

Good news :-) Bad news :-( First the good news. The suggestion from comment #18 works great! Now for the bad news. It seems that the code in nsModule.cpp is only done on the first launch of seamonkey following a new install. Also, at the point it is run, not all the preferences files have been processed so all you get is the default preference. So, I can make this work using this method, but only at the expense of dropping support for XUL directory listings.

> Also, at the point it is run, not all the preferences files have been > processed That's where installing a pref observer would help. As those files got processed, you would get notified about the preference changing and could remove or add the category entry as needed. That would also make dynamic changes to the preference work correctly.

And silly me thought this would be a simple bug :-). OK so, what I need to do is to find a good place in some initialization routine that runs after all the preferences are loaded and have that set the proper initial state as well as set up a preference observer to watch the network.dir.format preference and do the right thing if it changes form XUL to non-XUL or vice versa. I could use some advice as to the correct place to put this code.

> OK so, what I need to do is to find a good place in some initialization routine No, you don't. If you just install the pref observer on module load there (and remove it on XPCOM shutdown), it'll get notified as we load the rest of the pref files.

(In reply to comment #17) >My last question is that if I fix this as suggested, that would seem to leave >this issue unresolved for the xul directory viewer case. Should I just disable >the file upload menu choice if the XUL viewer is enabled? Can't you enable the file upload menu only if the ftp URL ends with a slash? (In reply to comment #20) >That would also make dynamic changes to the preference work correctly. They currently already do; would that change break it somehow?

> They currently already do; Because the pref is checked in the document factory every time it's invoked. What we want to do, however, is to not even invoke the document factory when not using the XUL viewer. And since that decision involves changing the category manager to pick the mode, it needs an explicit pref observer.

you're aware that the RegisterProc function doesn't usually get called, right? (Only when component registration is triggered, so basically on extension install/uninstall and upgrade)

Then that code needs to move into the module ctor/dtor. Or maybe we should just ditch the xul viewer...

(In reply to comment #23) > (In reply to comment #17) > >My last question is that if I fix this as suggested, that would seem to leave > >this issue unresolved for the xul directory viewer case. Should I just disable > >the file upload menu choice if the XUL viewer is enabled? > Can't you enable the file upload menu only if the ftp URL ends with a slash? Well, that would just make the file upload never work. The issue is that the trailing slash is not present in the URL when it should be. That is why the files end up being stored in the wrong place on the server. If you just don;t enable the menu choice to avoid that then you can't upload at all. I suppose you could manually add the '/' yourself, but the folder links on the index page don't contain the trailing '/' in the target.

(In reply to comment #25) > you're aware that the RegisterProc function doesn't usually get called, right? > (Only when component registration is triggered, so basically on extension > install/uninstall and upgrade) > Yes. I mentioned that in comment #19.

(In reply to comment #27) >>Can't you enable the file upload menu only if the ftp URL ends with a slash? >Well, that would just make the file upload never work. The issue is that the >trailing slash is not present in the URL when it should be. I thought that the point of fiddling around with stream converters is that it would fix up the ftp URL so that it did end with a slash? bz, is it not possible for the FTP channel that produces the http index format to correct the URL?

I have no idea. I try not to look at the FTP code, to be entirely honest. But yes, that seems like a much better idea than messing with the URL in the stream converter, in general.

(In reply to comment #29) > bz, is it not possible for the FTP channel that produces the http index format > to correct the URL? > Actually that is the approach I was taking on this bug initially. But in researching that, I found this code that seemed to be already trying to fix the slash issue, So I thought it would be easier to try to fix the existing code than to try to invent a new solution. It appears that I may have been wrong. I will go back to where I was in investigating fixing the FTP code. I hope it is as simple as trying a CWD before trying the RETR and appending a slash to the URL if the CWD succeeds and there isn't one already there.

nsFtpState::SetContentType looks like a good place to me.

(In reply to comment #32) > nsFtpState::SetContentType looks like a good place to me. > Thanks. I'll try there next. I think I have some time to work on this tonight.

Actually I had a patch tested and ready when Neil suggested nsFtpState::SetContentType, but I liked his suggestion better because mine was still in a streamconverter, and I think a streamconverter should be like a filter. It takes the import stream and preforms some transform on it to produce the output stream. It really should not be changing some other random data someplace else just because it seems a convenient place to do it. In any event, I have a build in progress with a patch that should fix this. I will attach it once i have completed testing on it.

This moves the code to append missing slash to FTP URIs form the streamconverter to nsFtpState::SetContentType.

Comment on attachment 282204 [details] [diff] [review] This fixes the issue dougt is probably a much better reviewer than I for this code...

Comment on attachment 282204 [details] [diff] [review] This fixes the issue >+ nsCAutoString baseUri; >+ rv = uri->GetAsciiSpec(baseUri); >+ if (NS_FAILED(rv)) return rv; The old codepath needed the baseUri elsewhere but you can probably stick to using the path throughout.

Good point! I am re-nominating as a blocker now that the bug has an owner and a patch.

Comment on attachment 282337 [details] [diff] [review] addresses Neil's comments this code can be simplified how about: if (mPath.Last() != '/') { mPath.Append('/'); nsCOMPtr<nsIURL> url(do_QueryInterface(mChannel->URI())); url->SetFilePath(mPath); } (note: mPath is unescaped, so you don't have to deal with %2f)

(In reply to comment #39) > nsCOMPtr<nsIURL> url(do_QueryInterface(mChannel->URI())); I guess this should replace mChannel->GetURI above? nsIURI* or nsCOMPtr? > url->SetFilePath(mPath); What's wrong with uri->SetPath(path); ? >(note: mPath is unescaped, so you don't have to deal with %2f) Any idea why the old code did? ftp://me@host// or ftp://me@host/%2F perhaps?

(In reply to comment #40) > (In reply to comment #39) > > nsCOMPtr<nsIURL> url(do_QueryInterface(mChannel->URI())); > I guess this should replace mChannel->GetURI above? nsIURI* or nsCOMPtr? As that uses do_QueryInterface it has to use a nsCOMPtr... > > url->SetFilePath(mPath); > What's wrong with uri->SetPath(path); ? The code in nsFtpState::Init that initializes mPath uses GetFilePath (for some reason it has code to handle URIs not implementing nsIURL, and I don't know why...) > >(note: mPath is unescaped, so you don't have to deal with %2f) > Any idea why the old code did? ftp://me@host// or ftp://me@host/%2F perhaps? Yeah, I guess the latter

(In reply to comment #39) > (From update of attachment 282337 [details] [diff] [review]) > this code can be simplified > > how about: > > if (mPath.Last() != '/') { > mPath.Append('/'); > nsCOMPtr<nsIURL> url(do_QueryInterface(mChannel->URI())); > url->SetFilePath(mPath); > } This is simpler, but it does not work. 1. mPath is not a valid URI path. It is missing the leading '/'. I guess that would be ok if setPath fixes that somehow. I have not checked. 2. You do need to do something about "/%2f" case. This has to do with the case you mentioned about the url containing a username. ftp://user@host/ gets the login directory for the user. ftp://user@host/%2f starts at the root of the filesystem. In the first case mpath == "" in the second, mapath == "/". So, in order to use mPath you would need to check the first character and if it is a '/' insert a "%2f" following the slash, otherwise insert a '/' at the beginning of mPath. This hardly seems simpler. That said, the code does appear like it might be possible to simplify it (I mostly copied it the way it was out of the stream converter). I will look at it and post a new patch.

(In reply to comment #41) > The code in nsFtpState::Init that initializes mPath uses GetFilePath (for some > reason it has code to handle URIs not implementing nsIURL, and I don't know > why...) Because mPath is what is sent to the FTP server. The FTP server wants UNIX type file paths, not a URI path. What is needed for this code to all work correctly downstream is for the URI path to have the trailing slash appended if one is required. It might be simpler to use a path other than the URI path as the source because it is already lying around in some variable, but all of these paths exist because of minor differences in path specification for different purposes. I am not sure all of these transformations are always symmetric and it is therefore safer to just use the URI path as the source for what you are putting back into the URI path. For FTP URI's, alot of that has to do with the "%2f" special case.

I was able to use mPath to simplify the test to see if appending the '/' was necessary at least.

Comment on attachment 282394 [details] [diff] [review] simplified patch I'm not sure why you differentiate between "unix-style paths" and "URI paths" here. The only difference is that URI paths can have escaped characters, right? Do you have any reason to believe that my suggested code doesn't work? No, you don't have to insert a slash prefix if you use SetFilePath. But, sorry for not realizing that mPath can be empty. You can't call Last() on a string that's possibly empty; you need to check IsEmpty() first.

(In reply to comment #45) > (From update of attachment 282394 [details] [diff] [review]) > I'm not sure why you differentiate between "unix-style paths" and "URI paths" > here. The only difference is that URI paths can have escaped characters, right? Yes I guess so. I guess I was starting at this code too long. I had thought that in some cases mPath we altered based on responses form the server, but I was mis-remembering. It is mPwd that is sometimes set from server responses. I was concerned that symlinks or Windows long/short name issues might make the URI path end up changing drastically from what the user originally entered. > Do you have any reason to believe that my suggested code doesn't work? Your suggested code would not work for the case where the original URI was: ftp://user@host.domain/%2f This would return the directory listing for / However the URI displayed and saved in history would be: ftp://user@host.domain/ So, clicking on reload would return a directory listing for ~/user > > No, you don't have to insert a slash prefix if you use SetFilePath. > > But, sorry for not realizing that mPath can be empty. You can't call Last() on > a string that's possibly empty; you need to check IsEmpty() first. > So, is there something else you still want me to change, or it just the missing IsEmpty check?

(In reply to comment #47) > So, clicking on reload would return a directory listing for ~/user ^^^^^^ Sorry, meant ~user

(In reply to comment #45) > Do you have any reason to believe that my suggested code doesn't work? Actually a better example than what I gave before is required because my previous example of ftp://user@host.domain/%2f could just be handled by an additional condition on the if and still using the rest of ou suggestion of just using mPath as the source for the fixed URI path. You really run into trouble with something like: ftp://user@host.domain/%2fusr/local/bin This results in mPath == "/usr/local/bin which will result in your code setting the URI path to "/usr/local/bin/" and the directory that would be displayed would be /usr/local/bin on the FTP server. However if you click reload, then the directory that would be displayed would be ~user/usr/local/bin. because the URI would be "ftp://user@host.domain/usr/local/bin" instead of "ftp://user@host.domain/&2fusr/local/bin". I think the code to account for this situation is more complicated than the code to load the path from the uri.

1. Sorry for BugSPAM. 2. Bugzilla needs a feature so I can edit my last comment. 3. The last to URIs in the last comment should have had the '/' appended.

Examples are always good. These all assume both you and my code are corrected for the mpath = "" case. Case InputPath mPath MyCode YourCode 1 "/" "" "/" "/" 2 "/pub/" "pub/" "/pub/" "/pub/" 3 "/pub" "pub" "/pub/" "/pub/" 4 "/%2f" "/" "/%2f" "/%2f" 5 "/%2fpub/" "/pub/" "/%2fpub/" "/%2fpub/" 6 "/%2fpub" "/pub" "/%2fpub/" "/pub/" So, you code gets them all right except for case 6.

(In reply to comment #41) >(In reply to comment #40) >>(In reply to comment #39) >>> url->SetFilePath(mPath); >>What's wrong with uri->SetPath(path); ? >The code in nsFtpState::Init that initializes mPath Sorry, I didn't spot that you switched to mPath throughout, which is why you had eliminated the mChannel->GetURI call completely (c.f. first question).

Comment on attachment 282527 [details] [diff] [review] With the IsEmpty test for mPath >+ nsCOMPtr<nsIURI> uri(do_QueryInterface(mChannel->URI())); So, I don't think this needs a do_QueryInterface, but I'm still waiting for biesi to answer whether it needs an nsCOMPtr or not :-)

(In reply to comment #54) > (From update of attachment 282527 [details] [diff] [review]) > >+ nsCOMPtr<nsIURI> uri(do_QueryInterface(mChannel->URI())); > So, I don't think this needs a do_QueryInterface, but I'm still waiting for > biesi to answer whether it needs an nsCOMPtr or not :-) > You seem pretty comfortable with this area, should I change the r= to you?

(In reply to comment #54) > (From update of attachment 282527 [details] [diff] [review]) > >+ nsCOMPtr<nsIURI> uri(do_QueryInterface(mChannel->URI())); > So, I don't think this needs a do_QueryInterface, but I'm still waiting for > biesi to answer whether it needs an nsCOMPtr or not :-) > So more like mChannel->URI->getPath(path); ?

(In reply to comment #55) >should I change the r= to you? I'm not a peer, I was just suggesting code variations. I could technically do the sr though, if biesi is happy to do the review.

well, dougt does not seem to be a peer either, but BZ who is listed as a peer for this did a hand-off.

if you use GetPath it doesn't need an nsCOMPtr nor a QueryInterface, indeed what does the code do for ftp://ftp.mozilla.org/pub?foo ?

(In reply to comment #59) > if you use GetPath it doesn't need an nsCOMPtr nor a QueryInterface, indeed > > what does the code do for ftp://ftp.mozilla.org/pub?foo ? > Well, if that ended up returning a directory listing i guess it would put a slash at the end of it. Now i wonder what a '?' character at that point of an FTP URI would actually man. Does the '?' indicate the beginning of a query string? a re those even supported in FTP URIs or is is it a wildcard character. Hmm back to more testing. if it is a query string then the slash would need to be inserted before the '?'. That would make the code a lot trickier. I am canceling the review requests and will do more testing and come up with a new patch.

yes, it's a query string. nsIURL::filePath handles that correctly.

Thanks for answering comment #40...

> well, dougt does not seem to be a peer either, He's a special case: he wrote this code and used to own it for a good long while, before he stopped being networking peer.

(In reply to comment #61) > yes, it's a query string. nsIURL::filePath handles that correctly. > OK so i need to figure out how to fix that too. BTW the current code botches that case also. It also appends the / following the query string. The fact that you can use a query string on an FTP URI must not be well known. SQUID ends up doing a RETR pub?foo for that URL.

(In reply to comment #61) > yes, it's a query string. nsIURL::filePath handles that correctly. > Well, it is interesting that we treat it as a query string. So my testing has shown that neither IE, Opera or squid proxy do. They all consider the ? to be just like any other filename character in an FTP URL. Therefore, If it is not easy to put the slash before the ? I might just fix this so that if there is a ? in the URL it does not append the slash, so at least it makes it no worse.

RFC1738 mentions the <searchpart> only in section 3.3 which is specific to HTTP URLs. Further, in section 5 it lists '?' as a valid character in an <fsegment> (which is a path segment) in an FTP URL. Therefore, it would appear that a '?' should not be treated any differently than an alpha character in this context. Therefore putting the slash at the end of the URI is really the correct thing to be doing and the code that is special casing the '?' character in FTP URLs is wrong. However, given the current behavior, I will make sure not to append the slash if there is a '?' in the URL and leave fixing FTP urls with '?' to be parsed according to the spec for another bug.

What about ftp://ftp.mozilla.org/pub#foo or does the channel not see the #?

According to RFC 1738 again 3.2.2. FTP url-path The url-path of a FTP URL has the following syntax: <cwd1>/<cwd2>/.../<cwdN>/<name>;type=<typecode> so once you get tot he path component the only characters that seem to be treated differently would be '/' ';' and '='. and '=' only has special meaning after the ';' and ';' is only valid for a filename URL and not for a directory. Therefore I think my code currently works correctly for any FTP URL that is valid according to the spec.

Note that RFC 1738 (dated 1994) has been updated twice: RFC 2396 in 1998 and RFC 3986 in 2005. You might want to look at those instead (though it may not change anything). I believe 2396 is what browsers (including us) tend to implement.

Google failed me. I guess I should have just searched the rfc index.

You probably searched for "url rfc" and not "uri rfc"?

(In reply to comment #65) > Therefore, If it is not > easy to put the slash before the ? Just use filePath instead of path? (In reply to comment #67) > What about ftp://ftp.mozilla.org/pub#foo or does the channel not see the #? The channel sees the #; it probably has the same issue.

(In reply to comment #72) > (In reply to comment #65) > > Therefore, If it is not > > easy to put the slash before the ? > > Just use filePath instead of path? > > > (In reply to comment #67) > > What about ftp://ftp.mozilla.org/pub#foo or does the channel not see the #? > > The channel sees the #; it probably has the same issue. > Ah! I get it! IF I use filePath instead of path then I place the slash based on the way the URI was parsed. That way if we decide the current parser interpretation of the spec is wrong and change it this code can stay the same. Also, if the spec changes and some other character is added as a delimiter between the file path part of the URI and whatever follows, this code will still be able to remain unchanged I like it. I will try to get a new patch out either tonight or tomorrow.

This seems to work. Unfortunately changing to Get/Set File Path seems to make the nsCOMPtr and QueryInterface necessary.

don't capitolized "Don't" in the comment. check for the filepath in SetContentType seams wrong. Isn't there a better place for this check -- maybe in S_LIST handling? have you tested the case where we are fetching a file via FTP, but it comes from the cache? I am sorta worried that this change will modify the filepath to append a '/' for a file. also in nsIndexedToHTML, move the |rv = uri->GetPath(path);| stuff into the block where path is used.

(In reply to comment #75) >have you tested the case where we are fetching a file via FTP, but it comes >from the cache? I am sorta worried that this change will modify the filepath >to append a '/' for a file. The special FTP cache only caches directories. See bug 122548. >also in nsIndexedToHTML, move the |rv = uri->GetPath(path);| stuff into the >block where path is used. You mean down within the same block to where path is used, right?

carrying sr+ forward

(In reply to comment #75) > don't capitolized "Don't" in the comment. > Fixed in new patch. > > check for the filepath in SetContentType seams wrong. Isn't there a better > place for this check -- maybe in S_LIST handling? > This is not addressed in the new patch I just attached. I wanted to get a working baseline patch that works and addresses the other review comments before trying the code at a different place. > have you tested the case where we are fetching a file via FTP, but it comes > from the cache? I am sorta worried that this change will modify the filepath > to append a '/' for a file. > I have done a great deal of testing on this and have not been able to produce a problem. > also in nsIndexedToHTML, move the |rv = uri->GetPath(path);| stuff into the > block where path is used. > The new patch addresses this.

(In reply to comment #75) > check for the filepath in SetContentType seams wrong. Isn't there a better > place for this check -- maybe in S_LIST handling? I tried moving the check there, but ran into the reverse of the issue you were worried about with downloads and the cache. Moving this into the protocol handling means the URL only ends up with the slash in the case where the directory is NOT in the cache. So I went back to doing it in SetContentType unless you have a suggestion of a better place which does not end up with the code being bypassed in the cache case.

Comment on attachment 286228 [details] [diff] [review] addressing comments please comment in SetContentType why this check for the slash is best where it is.

Requesting approval for post M9 check-in. Patch is cbiesinger: sr+ dougt: r+ I have been running with this code for about a month now and done extensive testing of edge cases under both Windows and Linux without finding any issues.

Comment on attachment 286307 [details] [diff] [review] patch for check-in a=endgame drivers for M9

Checking in netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp; /cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp,v <-- nsFtpConnectionThread.cpp new revision: 1.317; previous revision: 1.316 done Checking in netwerk/streamconv/converters/nsIndexedToHTML.cpp; /cvsroot/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp,v <-- nsIndexedToHTML.cpp new revision: 1.87; previous revision: 1.86 done