72280 - Bulk FTP Changes

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Doug Turner (:dougt)]

I have been working of resolving some ftp problems which include:

53002
53429 (?)  Now should alert
67852
57763
32307
60509
71178
70580
71734
66330

Comment 1

[Doug Turner (:dougt)]

Attachment: Text description of changes.

Comment 2

[Doug Turner (:dougt)]

Attachment: Patch v.1

Comment 3

[Judson Valeski]

"
+    if (mPrompter && NS_FAILED(mInternalError) && +        (mInternalError !=
NS_ERROR_UNKNOWN_HOST ||
+         mInternalError != NS_ERROR_NET_TIMEOUT ))
+    {  
+        nsAutoString    title;
+        title.AssignWithConversion("Error");        // localization
+        nsAutoString    text;
+        text.AssignWithConversion(mResponseMsg);
+        
+        (void) mPrompter->Alert(title.GetUnicode(), text.GetUnicode());
+    }
+
"

The docshell throws timeout and dns failure alerts, why are you doing this here?


"
 nsFtpControlConnection::Connect()
 {
@@ -137,6 +150,11 @@
     nsresult rv;
     nsCOMPtr<nsIInputStream> inStream;
 
+    // nsCOMPtr<nsISocketTransport> sTrans = do_QueryInterface(mCPipe);
+    //if (!sTrans) return NS_ERROR_FAILURE;
+    //rv = sTrans->SetSocketTimeout(25);  
+    //if (NS_FAILED(rv)) return rv;
+
"

"25" who? what? when? where? why? :-). At least a #define please... perhaps a pref?

I'm sure you've already gone through this excersize, but just in case...
regarding OnStart/Stop pairs. It's required that a protocol handler spew *both*,
in the following order, OnStart, OnStop, in *all* cases. Please do a mental
check on the following. 

Will an OnStart|OnStop pair be propegated to the consumer (the one who initiated
the request for "ftp://foo") after the request is initiated via AsyncOpen() when:

1. The user hits the cancel button before the server has even been resolved?
2. The user hits cancel after the server has been resolved, but the connection
has not been established? 1 and 2 used to be considered the same from FTP's
standpoint, that may still be the case.
3. The user hits cancel during control connection activity?
4. The user hits cancel during data connection activity (middle of a download:
directory listing download as well as file)?
5. The DNS lookup fails?
6. The socket transport connection fails (timeout, interrupt, whatever)?
7. The user or password command fails?
8. Some other control connection command fails?
9. The data connection fails (timeout, close(), whatever, just some socket error)?

Same 9 above but for a synchronous read (if supported).

Comment 4

[Doug Turner (:dougt)]

> The docshell throws timeout and dns failure alerts, why are you doing this here?


I am capturing error should could be created by the ftp protocol (eg. Could not
create data connection, Ratio requirements, Dir/file does not exists).  The two
errors that I am checking against are handled above FTP by the webshell.. The
web shell will throw this dialogs.  

yes, it sucks that this happens in to places.   It would be great if the clients
could know about all of the different errors that FTP could genereate, but after
talking with conrad and gagan, we believe that serious errors, such as these,
should either be Alert()'ed or passed to an event sink.  I took the less work
approach and used Alerts, just like it was done in the other protocols.  The
more I think about this, the more I think that we should just pass this error
via the nsIProgressEventSink and let the clients deal with hit.  We should
probably file a bug on this.  Would this be required for embedding??




+    // nsCOMPtr<nsISocketTransport> sTrans = do_QueryInterface(mCPipe);
+    //if (!sTrans) return NS_ERROR_FAILURE;
+    //rv = sTrans->SetSocketTimeout(25);  
+    //if (NS_FAILED(rv)) return rv;
+
"

"25" who? what? when? where? why? :-). At least a #define please... perhaps a pref?


Heh.  It should have been:  #define DOUGT_STRESS_TEST  I will fix this.


Jud, thanks for this list.  It is a pretty good breakdown.  To answer this more
directly, here is what ftp does.  Before I open up a data connection, if any
error occurs, a onStart and onStop will be fired to the consumer.  Once the data
connection has been established, the socket transport will fire these
notifications (though a "forwarder" wrapper).  

I will verify that these cases are all met today.  If any are not, you will hear
back from me.

Comment 5

[Doug Turner (:dougt)]

Attachment: Patch v.2 - Includes uriloader OnStart workarounds.

Comment 6

[Doug Turner (:dougt)]

The last patch includes a workaround to shield the URILoader from OnStartRequest
notifications until the data connection has something to give it.  Without this
workaround, errors prior to any data being recieved resulted in a page
transition.

Also the patch includes the suggests from Jud.

Comment 7

[Judson Valeski]

r=valeski on patch v.2

Comment 8

[Doug Turner (:dougt)]

thanks

Comment 9

[benc]

VERIFYING:
Each bug will be verified/has been for correct behavior.