Closed Bug 252344 Opened 21 years ago Closed 21 years ago

FTP does never show "looking up host" status message

Categories

(Core Graveyard :: Networking: FTP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8alpha3

People

(Reporter: Biesinger, Assigned: Biesinger)

Details

Attachments

(1 file, 2 obsolete files)

when going to an ftp url, the status bar does not show any messages about "looking up host", and not about "connecting to host" initially either.
Attached patch patch (obsolete) — Splinter Review
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8alpha3
Comment on attachment 153832 [details] [diff] [review] patch aren't there cases where you might leak |eventSink|? Perhaps you should wrap the newly created eventSink with a nsRefPtr prior to passing it to nsFtpControlConnection::Connect. also, i don't think you really want to pass NULL as the second parameter to SetEventSink. doing that causes the events to happen on the socket transport thread, and your code appears to then call out to the nsIProgressEventSink impl on the socket thread! call NS_GetCurrentEventQ and pass that as the second parameter. see for example the code that sets a nsITransportEventSink for the data connection.
Attachment #153832 - Flags: review?(darin) → review-
(In reply to comment #2) > (From update of attachment 153832 [details] [diff] [review]) > aren't there cases where you might leak |eventSink|? Perhaps you should wrap > the newly created eventSink with a nsRefPtr prior to passing it to > nsFtpControlConnection::Connect. oh, hm, yeah. will fix. > also, i don't think you really want to pass NULL as the second parameter to > SetEventSink. doing that causes the events to happen on the socket transport > thread, and your code appears to then call out to the nsIProgressEventSink impl > on the socket thread! Yes, but I think that's fine. the nsIProgressEventSink is the nsFTPChannel, and that has a proxy to the "real" event sink: http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/ftp/src/nsFTPChannel.cpp#508
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #153832 - Attachment is obsolete: true
Comment on attachment 154591 [details] [diff] [review] patch v2 oh, ok... i didn't realize mSink was already a proxy. that's probably somewhat leftover from the days when FTP allocated an actual thread for the state logic. ok... looks good to me. though, you should know that the transport event sink proxy object is nice because it avoids clogging the event queue with more than one status/progress event at a time. it can be configured to be lossy like that in cases where some loss of status/progress information is tolerable. doesn't TransportEventForwarder need to support threadsafe AddRef and Release at least?
Comment on attachment 154591 [details] [diff] [review] patch v2 r=darin with NS_IMPL_THREADSAFE for TransportEventForwarder.
Attachment #154591 - Flags: review?(darin) → review+
Comment on attachment 154591 [details] [diff] [review] patch v2 hmm, as mSink is an nsFTPChannel, this patch causes assertions about ftp channel's lack of threadsafe isupports. all in all, it's probably better to specify an eventq.
Attachment #154591 - Attachment is obsolete: true
Attached patch patch v3Splinter Review
now using current thread's eq
Attachment #155211 - Flags: review?(darin) → review+
Attachment #155211 - Flags: superreview?(bzbarsky)
Comment on attachment 155211 [details] [diff] [review] patch v3 sr=bzbarsky
Attachment #155211 - Flags: superreview?(bzbarsky) → superreview+
Checking in netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp; /cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp,v <-- nsFtpConnectionThread.cpp new revision: 1.287; previous revision: 1.286 done Checking in netwerk/protocol/ftp/src/nsFtpControlConnection.cpp; /cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpControlConnection.cpp,v <-- nsFtpControlConnection.cpp new revision: 1.36; previous revision: 1.35 done Checking in netwerk/protocol/ftp/src/nsFtpControlConnection.h; /cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpControlConnection.h,v <-- nsFtpControlConnection.h new revision: 1.23; previous revision: 1.22 done
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: