128927 - ftp hang when CR and LF are split in OnDataAvailable

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect, P2)

Description

[[not reading bugmail]]

see bug 118719 for fixing this problem with http://ftp links and others, but
links here on this page are of type:

ftp://ftp.. ie: ftp://ftp.3com.com/pub/nic/3c90x/3c90x1.exe

These types of links take a long delay time from 'beginning FTP transaction'
status line text, ie after clicking, to the point of dialog showing is really
long, if you hit 'stop'  the dialog will come up, and you can hit the 'save
file' within the dialog.. and the ftp transaction takes some time before
downloading, but sometimes if you do this with a file you already have saved in
the directory before, you get a 0 byte file.  It maybe hanging inside the
network code as well.


testing build 2002-3-1-03, 3-2-08, 3-3, 3-4.. w2k all seem to have this problem.

netscape 6.2.1 doesn't exhibit this slowness, didn't have an opportunity to
check milestones yet.  I'll report back later to see where this maybe have started.

searched bugzilla couldn't find this yet..

Comment 1

[Bradley Baetz (:bbaetz)]

The only way we can know whether its a directory or not is by talking to the
server, and (usually) doing content sniffing, since ftp doesn't provide a mime type.

bz, any ideas on shortcuts?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Dennis, are you using shift-click or just clicking on the links?  That is, are
you actually doing "save as" or clicking through to the content?

Comment 3

[[not reading bugmail]]

opps, sorry about that, I have just been clicking on the links, I have not done
nor tried any other method.

Comment 4

[Boris Zbarsky [:bzbarsky]]

If you just click on the link, then we have to contact the server, get some
data, detect the type before we can show that dialog.

Comment 5

[[not reading bugmail]]

ok, thanks, then even if that is invalid, is there a network issue then with the
delay?  As I noted, Netscape 6.2.1 pops up much quicker.  using the same link and 

I tried the 'save link as'  context menu item but it didn't contact the server
to fetch the file, like it is broken.  I was using the URL which had those links
on there.

I'm currently writing and viewing this bug using NS 6.2.1 to download the FTP
file below in the description, if I click on it, it takes 2 seconds count to go
from Status line text 'Beginning FTP transaction' to 'Connecting to
ftp.3com.com' in the status line, then the 1 second after to bring up the dialog
window for the saving the filename.. before actually downloading the file.  

Boris, 

Current Mozilla builds delay counts are in the 10's of seconds, not 1 or 2
seconds to do this.  NS 6.2.1 takes about 4 or 5 seconds from a click on the ftp
link below to bringing up the dialog.  Mozilla is about 30+ .. so Then there is
some large delay, because as a user, I don't like waiting that amount of time..
so I used both Ns 6.2.1 or IE 5. to grab these files, Mozilla's current builds
are way too slow for ftp://ftp links, I dont think any other user of a
distribution build would like to wait that long, if it was on the order of 5
seconds in 6.2.1. :)

If you dont mind, I'd like to punt it over to networking and have darin take a
look at it, for a network issue.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yep.  we take _forever_ to contact that server.  If I hit "stop" while we're
doing that, I immediately get the "what do I do" dialog.

Note that this is not a problem with the ftp.mozilla.org server, just the 3com
one.  Reopening.

Comment 7

[Boris Zbarsky [:bzbarsky]]

to ftp networking

Comment 8

[Bradley Baetz (:bbaetz)]

It takes 5 seconds for me on a fast connection. How long does it take for you?
Theres a lot more round tripping with ftp than http, so it will take longer.

Comment 9

[Boris Zbarsky [:bzbarsky]]

On that 3com page?  I gave up after 40 seconds and hit "escape".  This was from
a computer on MIT's campus.  Same thing from here (MediaOne cablemodem).  Again,
irc.mozilla.org is quite fast.

Comment 10

[Bradley Baetz (:bbaetz)]

Cable modem from Australia took 5 seconds. If you can reproduce this, how long
does a command line ftp program take?

Comment 11

[Boris Zbarsky [:bzbarsky]]

wget takes 8 seconds to contact the server and get the file.  See also bug 129364

Comment 12

[[not reading bugmail]]

brad.. I'm on dialup and this is painfully slow, I cant test the mac or anything
else but win98.. but took a look at the other bug 129364 comment 6 and it seems
like the same :) 

thanks guys.

Comment 13

[[not reading bugmail]]

ok, found the problem build.. its fine in build 2002-02-01 and a problem in
build 2002-02-02 w2k platform.

several checkins to networking code:general and one for networking:FTP.

Checkins between 02/01/2002 08:00 and 02/02/2002 12:00:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=02%2F01%2F2002+08%3A00&maxdate=02%2F02%2F2002+12%3A00&cvsroot=%2Fcvsroot

bug 86917: socket transport should try all ip addresses returned by DNS service
Trunk M098 [@ PR_GetIdentitiesLayer]

bug 122675: socket transport doesn't cancel properly if busy resolving or connecting

bug 122787: (nsIURI) nsStandardURL::SetFileName doesn't recalculate mPath

bug 120959: Use of ".." in address causes links to relative URL's to fail

darin & dougt checked in these bugs..

nominating and bumping severity.

Comment 14

[Bradley Baetz (:bbaetz)]

OK, got it. This could be a regression from bug 110241 - we get the login banner
and then appear to hang

Updating url, and taking for 1.0. Removing nsCatfood keyword, which is only for
netscape employees.

The dialog never appears for me, and I suspect that if it did, you'd just be
saving the 0 byte file we love so much.

Comment 15

[[not reading bugmail]]

Bradley, I must of forgot to put that bug # on my last comment when I noted:
networking:FTP, oh well :).  


Also, I thought nsCatFood was general user dissatisfaction and nsDogFood is
netscape employees.  This line is why I had added nsCatFood: This indicates a
bug nominated as a serious user satisfaction issue for Netscape releases.

I didn't wanna add nsDogFood as that seems to be for internal use that is not
that bad of a bug.

As long as its good for now, thats all that matters now.  Thanks for working on
this.

Comment 16

[Bradley Baetz (:bbaetz)]

Attachment: patch

The problem here was that the CR was sent in a different ODA to the LF. The fix
is to concatnate the buffer before hand, and do the required fixup to deal with
that. I tried moving this to the new string apis, but all I got was a mess of
unreadable code which didn't work correctly.

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 73386 [details] [diff] [review]
patch

not +3? :-)
+	 line.Assign(currLine, (eol - currLine + 1) + 2);

Comment 18

[Darin Fisher]

Comment on attachment 73386 [details] [diff] [review]
patch

>Index: nsFtpConnectionThread.cpp

>+    nsCString lines(mControlReadCarryOverBuf);

any point to using a nsCAutoString here?  or are we always talking about data
greater than 64 bytes?

>+    lines.Append(buffer, aCount);

how about using SetCapacity here since we know how much space to eliminate an
extra call into the heap allocator?  or maybe a dependent concatenation??


>+        // Append the current segment, including the CRLF
>+        nsCAutoString line;
>+        line.Assign(currLine, (eol - currLine + 1) + 2);

can |line| be a dependent single fragment string instead?

   const nsDependentSingleFragmentCString &line = Substring(currLine, eol + 3);

other than these nits, sr=darin

Comment 19

[Bradley Baetz (:bbaetz)]

If I change to use no-flat strings, I have to use iterators, and thats gets into
the ugly code.

Why would setcapacity help? I pass in an explicit length.

+1 +2 for readability. +1 for the length calc + crlf

Comment 20

[Bradley Baetz (:bbaetz)]

Oh, and we're usually talking about a line < 64 bytes (the main exception being
login banners with # or * in the first and last column), but the main reason is
that I need a flat string.

Let me talk to people about a better way to do that.

Updating summary, too.

Comment 21

[Bradley Baetz (:bbaetz)]

bz: Feel like giving me some string help before I check this in?

Comment 22

[Peter Trudelle]

nsbeta1+ per Nav triage team

Comment 23

[Bradley Baetz (:bbaetz)]

Actually, this whole file needs to be redone for new string APIs. I've filed bug
131361 on doing that - it shouldn't block this bug.

Comment 24

[Brendan Eich [:brendan]]

Comment on attachment 73386 [details] [diff] [review]
patch

>+    const char* currLine = lines.get();
>+    while (*currLine) {
>         char* eol = strstr(currLine, CRLF);
>         if (!eol) {
>-            mControlReadCarryOverBuf += currLine;
>+            mControlReadCarryOverBuf.Assign(currLine);
>             break;
>         }
> 
>-        // Append the current segment
>-        mControlReadCarryOverBuf.Append(currLine, eol - currLine + 1);
>-
>+        // Append the current segment, including the CRLF
>+        nsCAutoString line;
>+        line.Assign(currLine, (eol - currLine + 1) + 2);

Why the + 1?  Isn't (eol + 2 - currLine) the right length?

/be

Comment 25

[Bradley Baetz (:bbaetz)]

Err, yeah (As discussed on irc with brendan, I did this thinko in the previous
patch)

I'll fix this when I get a moment free, hopefully before the weekend.

Comment 26

[Bradley Baetz (:bbaetz)]

Attachment: v2

Comment 27

[Darin Fisher]

Comment on attachment 75875 [details] [diff] [review]
v2

>Index: nsFtpConnectionThread.cpp

>+    nsCString lines(mControlReadCarryOverBuf);

so why isn't this a nsCAutoString?


>-                           isdigit(mControlReadCarryOverBuf.get()[0]) &&
>-                           isdigit(mControlReadCarryOverBuf.get()[1]) &&
>-                           isdigit(mControlReadCarryOverBuf.get()[2]));
>+        PRBool startNum = (line.Length() >= 3 &&
>+                           isdigit(line.get()[0]) &&
>+                           isdigit(line.get()[1]) &&
>+                           isdigit(line.get()[2]));

why .get()[i]?	you can use nsAFlatCString::operator[] instead.

>+        if (startNum && line.get()[3] == ' ') {

ditto


otherwise, sr=darin

Comment 28

[Bradley Baetz (:bbaetz)]

The entire data per ODA is often > 64 bytes, but each individual line is usually
only about 20-30.

Comment 29

[Vinay Badami]

r=badami for patch v2

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 75875 [details] [diff] [review]
v2

a=dbaron for trunk checkin.  I think nsAFlatCString::operator[] should work on
everything except nsXPIDLCString, where the implicit conversion to char* causes
an ambiguity because of the signedness/unsignedness of the parameter to
nsAFlatCString::operator[].

Comment 31

[Akkana Peck]

I got confused reading the discussion -- will this help the delays between
choosing "Save as" (or shift-clicking) and seeing the dialog, or only the delays
when clicking the link?

Comment 32

[Bradley Baetz (:bbaetz)]

This will only help this particular site, which hung until the connection failed.

ftp will take longer than http to set up the conneciton - we cna't get out of that.

Comment 33

[Bradley Baetz (:bbaetz)]

Checked in.

Comment 34

[[not reading bugmail]]

works great in build 3-31-09 w2k.  Thanks bradley.

Comment 35

[[not reading bugmail]]

turnaround time for this file is back to 5 seconds so that is fixed.

but, I think there is a new bug in the code that follows this as it gets passed
to the progress dialog and download manager.. after click save file.. 
build 3-31-09 w2k.  

The download manager and the progress dialogs now for this type of file, I tried
downloading http://ftp.<blah>/mozilla-win32-installer.exe again to see if this
was not related.. cause it spits that info out correctly.

For the links again on this bug/page: it looks like it not getting the correct
values passed for the file size, it shows 0 K, cause it spits out ok on
latest/builds page file downloads.

The time elapsed shows: 00:0-'insert a number here that grows' as it downloads...  

dont know if its related to the bug checkin for bug 130299 though.

Boris,
do you see this on linux too?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Yep.  This bug is fixed on Linux (build 2002-04-01-08).  The time elapsed
problem is a download manager/file handling issue, not an FTP issue.

Comment 37

[[not reading bugmail]]

noting comment #35 is bug 134901.

Comment 38

[[not reading bugmail]]

v

Comment 39

[benc]

+making test for FTP.