84472 - Error: make unsupported ftp server message user friendly

Status: VERIFIED FIXED

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

Description

[Henrik Gemal]

when I go to:
ftp://kuhub.cc.ukans.edu/
I crash with 20010604

talkback ID:
TB31442982E
TB31442893Z
TB31442844H


btw: it's a vms server http://bugzilla.mozilla.org/show_bug.cgi?id=22299

Comment 1

[Doug Turner (:dougt)]

There is code in the ftp list parsing that assumes that the ls format is UNIX if 
the SYST response is unrecongnized.  This assumption causes us to dump core.  

Maybe we should prompt the user about the lack of support for the ftp server?

Comment 2

[Doug Turner (:dougt)]

Attachment: Removes our pretend-that-we-can-handle-it hack

Comment 3

[Doug Turner (:dougt)]

gagan, review please; darin, sr please.

Until we official support a server type, we shouldn't pretend we do...

Comment 4

[Gagan]

Hmmm... how did we handle this in 4.x? If this was also dropped in 4.x then I'd
say r=gagan else I'd rather see us try to load it... 

Comment 5

[Doug Turner (:dougt)]

We loaded it in 4x, iirc.  The bug to support this paticular VMS is futured and 
is orthoganal to this bug.

darin, sr?

Comment 6

[Darin Fisher]

so, with this patch we will fail to load FTP sites from unknown systems?  and,
the problem is that we don't have any way of deciphering the ls format of an
unknown systems?  shouldn't the UNIX ls interpreter also be more robust to 
avoid crashing if the server sends a garbage ls format?

sr=darin, but i think we should make sure there is a bug filed against the UNIX
ls interpreter to avoid such crashes.

also, "unrecongnized" should be "unrecognized" ;-)
 

Comment 7

[Doug Turner (:dougt)]

thanks for pointing out my speelin. :-)

Comment 8

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 9

[Doug Turner (:dougt)]

fixed:
Checking in nsFtpConnectionThread.cpp;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp,v  <--
nThread.cpp
new revision: 1.177; previous revision: 1.176
done

Comment 10

[Henrik Gemal]

now when I go to:
ftp://kuhub.cc.ukans.edu/
I'm getting an alert saying:
"VMS MultiNet V4.3(91)"

That's not very user friendly!

Comment 11

[Doug Turner (:dougt)]

more user friendly than a crash.  ;-)  

Lets reopen this bug, so that we can add a dialog.

Comment 12

[Doug Turner (:dougt)]

adjusting summary

Comment 13

[Matthias Versen [:Matti]]

Dougt: Can you change that alert message ?
If a user gets this message, he don't know what Mozilla mean.
Maybe : "This FTP Server "VMS MultiNet V4.3(91)" is currently unsupported. 
Please report this at bugzilla.mozilla.org." is better.

Comment 14

[Doug Turner (:dougt)]

Attachment: necko.properties patch

Comment 15

[Doug Turner (:dougt)]

Attachment: patch to ftp

Comment 16

[Doug Turner (:dougt)]

the text will read:

The FTP server kuhub.cc.ukans.edu is currently unsupported.

I decided against including the SYST response because it would only confuse the
layperson.  If you are cool with that, I will seek a review, ect.

Comment 17

[Matthias Versen [:Matti]]

Thanks !
I think this is ok !

Comment 18

[Doug Turner (:dougt)]

bulk move to 0.9.4

Comment 19

[Bradley Baetz (:bbaetz)]

r=bbaetz

Comment 20

[Darin Fisher]

sr=darin

Comment 21

[Doug Turner (:dougt)]

*** This bug has been marked as a duplicate of 22299 ***

Comment 22

[benc]

qa to me.
VERIFIED:

Comment 23

[benc]

Was this ever checked in? I still see the old style dialogs in Mozilla 0.9.5.

Comment 24

[benc]

REOPEN: per bbaetz, updated summary to reflect fix.

Comment 25

[benc]

-> default owner + qa...
"everything old is new again..."

Comment 26

[Bradley Baetz (:bbaetz)]

hmm. taking for ->0.9.6. Theres a bit of bitrot, and in spite of my r=, I don't
think that:

+            const PRUnichar *formatStrings[1] = {
NS_ConvertASCIItoUCS2(mResponseMsg).get() };

is safe, because you're accessing the converted string beyond the life of the
temporary.

Comment 27

[Bradley Baetz (:bbaetz)]

Out of time, -> 0.9.7

Comment 28

[Bradley Baetz (:bbaetz)]

Attachment: revised patch

I've updated dougt's patch. I've also fixed the error value stuff, which was
wrong, and causing the uriloader to assert via the string code. (Getting rid of
some proxying code in the process, added in bug 72280 w/o comment. Is this just
vestiges of the threadsafe stuff? removing it didn't affect anything, and the
ptr was the same, afetrwards, so it definately wasn't being proxied)

It turns out that the uriloader asserts in other cases, too - I'll file a bug
on that. The current code is wrong, though, so the fix is still valid.

Comment 29

[Darin Fisher]

Comment on attachment 58636 [details] [diff] [review]
revised patch

probably want to keep the stream listener proxy code unless you can be certain
that it isn't reachable from any of the nsIChannel methods.

Comment 30

[Bradley Baetz (:bbaetz)]

Attachment: fixed patch

OK, readded the proxying.

Comment 31

[Doug Turner (:dougt)]

Comment on attachment 60409 [details] [diff] [review]
fixed patch

remove the debug printf.

why are you setting mStatus (the second change in this patch)?

Comment 32

[Bradley Baetz (:bbaetz)]

I need to set mstatus (and change the first param of the notification) so that
the uriloader can know that we failed.

Comment 33

[Darin Fisher]

Comment on attachment 60409 [details] [diff] [review]
fixed patch

sr=darin (i agree with dougt... either remove the debug printf or replace it
with a NS_WARNING)

Comment 34

[Bradley Baetz (:bbaetz)]

Checked in, with the debug printf removed (since the server type is now in the
dialog)

Comment 35

[Dawn Endico]

removing item for this bug from release notes now that it is fixed.

Comment 36

[Henrik Gemal]

verified fixed in 20020116