Error: make unsupported ftp server message user friendly

VERIFIED FIXED in mozilla0.9.7

Status

()

Core
Networking: FTP
P3
normal
VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: Henrik Gemal, Assigned: bbaetz)

Tracking

({testcase})

Trunk
mozilla0.9.7
x86
Windows 2000
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

17 years ago
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
(Reporter)

Updated

17 years ago
Keywords: crash

Updated

17 years ago
Severity: normal → critical

Updated

17 years ago
Target Milestone: --- → mozilla0.9.2

Comment 1

17 years ago
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?

Updated

17 years ago
Summary: crash when going to ftp://kuhub.cc.ukans.edu/ → crash when going to unsupported ftp sites.

Comment 2

17 years ago
Created attachment 37668 [details] [diff] [review]
Removes our pretend-that-we-can-handle-it hack

Comment 3

17 years ago
gagan, review please; darin, sr please.

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

Comment 4

17 years ago
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

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

darin, sr?
Status: NEW → ASSIGNED

Comment 6

17 years ago
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

17 years ago
thanks for pointing out my speelin. :-)

Updated

17 years ago
Blocks: 83989

Comment 8

17 years ago
a= asa@mozilla.org for checkin to the trunk.
(on behalf of drivers)

Updated

17 years ago
Whiteboard: ready to checkin

Comment 9

17 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Reporter)

Comment 10

17 years ago
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

17 years ago
more user friendly than a crash.  ;-)  

Lets reopen this bug, so that we can add a dialog.
Severity: critical → normal
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: crash when going to unsupported ftp sites. → unsupported ftp sites should alert
Target Milestone: mozilla0.9.2 → ---

Updated

17 years ago
Whiteboard: ready to checkin
Target Milestone: --- → mozilla0.9.3

Updated

17 years ago
Priority: -- → P3

Updated

17 years ago
Keywords: crash
Blocks: 91019

Comment 12

17 years ago
adjusting summary
Summary: unsupported ftp sites should alert → VMS unsupported
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

17 years ago
Created attachment 43530 [details] [diff] [review]
necko.properties patch

Comment 16

17 years ago
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.

Thanks !
I think this is ok !

Comment 18

17 years ago
bulk move to 0.9.4
Target Milestone: mozilla0.9.3 → mozilla0.9.4
(Assignee)

Comment 19

17 years ago
r=bbaetz

Comment 20

17 years ago
sr=darin

Updated

17 years ago
Target Milestone: mozilla0.9.4 → mozilla1.0

Comment 21

17 years ago

*** This bug has been marked as a duplicate of 22299 ***
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → DUPLICATE

Comment 22

17 years ago
qa to me.
VERIFIED:
Status: RESOLVED → VERIFIED
QA Contact: tever → benc

Comment 23

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

Comment 24

17 years ago
REOPEN: per bbaetz, updated summary to reflect fix.
Status: VERIFIED → REOPENED
Resolution: DUPLICATE → ---
Summary: VMS unsupported → Error: make unsupported server message user friendly

Comment 25

17 years ago
-> default owner + qa...
"everything old is new again..."
Assignee: dougt → bbaetz
Status: REOPENED → NEW
Summary: Error: make unsupported server message user friendly → Error: make unsupported ftp server message user friendly
(Assignee)

Comment 26

17 years ago
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.
Status: NEW → ASSIGNED
Target Milestone: mozilla1.0 → mozilla0.9.6
(Assignee)

Comment 27

17 years ago
Out of time, -> 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
(Assignee)

Comment 28

17 years ago
Created attachment 58636 [details] [diff] [review]
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.
Attachment #37668 - Attachment is obsolete: true
Attachment #43530 - Attachment is obsolete: true
Attachment #43531 - Attachment is obsolete: true

Comment 29

17 years ago
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.
Attachment #58636 - Flags: needs-work+
(Assignee)

Comment 30

17 years ago
Created attachment 60409 [details] [diff] [review]
fixed patch

OK, readded the proxying.
Attachment #58636 - Attachment is obsolete: true

Comment 31

17 years ago
Comment on attachment 60409 [details] [diff] [review]
fixed patch

remove the debug printf.

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

Comment 32

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

Comment 33

17 years ago
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)
Attachment #60409 - Flags: superreview+
(Assignee)

Comment 34

17 years ago
Checked in, with the debug printf removed (since the server type is now in the
dialog)
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 35

17 years ago
removing item for this bug from release notes now that it is fixed.
(Reporter)

Comment 36

17 years ago
verified fixed in 20020116
Status: RESOLVED → VERIFIED

Updated

16 years ago
Keywords: testcase
You need to log in before you can comment on or make changes to this bug.