389394 - FTP uploads using File -> Upload File menu fail silently on errors

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Bill Gianopoulos [:WG9s]]

If you use the "File -> Upload File" menu to put files to an FTP server, and the upload gets a server-side error (for example if the file already exists and is read-only) the upload appears to proceed normally yet does nothing.  No error indication is returned to the user.

Comment 1

[Bill Gianopoulos [:WG9s]]

It turns out this only occurs on short files.  It could just be it is not checking status on the final flush.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Wanted, not blocking.

Comment 3

[Michal Novotny [:michal]]

Attachment: fix

The problem with small files is in that data connection finishes too early. Method nsFtpState::OnStopRequest() calls Close() which causes calling nsFtpChannel::OnStopRequest() (inherited from nsBaseChannel) where mCallbacks is cleared. When error from ftp server is received, there is no callback to notify: http://mxr.mozilla.org/mozilla/source/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp#1738

AsyncStreamCopier that uploads data is created and started immediately after response to PASV is received. So nsFtpState::OnStopRequest() can be called in following nsFtpState's states:

mState=FTP_READ_BUF mNextState=FTP_R_CWD
mState=FTP_R_CWD
mState=FTP_READ_BUF mNextState=FTP_R_STOR
mState=FTP_R_STOR
... and of course some error states

The patch fixes the problem by delaying the call to Close() if response to STOR command hasn't been received yet. Instead of checking all possible states I've created new member variable mStorReplyReceived that holds the information.

Comment 4

[Bill Gianopoulos [:WG9s]]

I should probably explain why I nominated this as wanted.  THis is not strictly required for Firefox, but Seamonkey and I beleive Camino do have a file upload option.  I found this in early testing of my ftpupload extension for Firefox.  I Hope to get drag-and-drop added to my extension and then would like to get it possibly included in Firefox version 4.  So I was kind of hoping for early inclusion of the backend fixes required for it to work properly so that I could continue my work on the extension. 

Comment 5

[Boris Zbarsky [:bzbarsky]]

Michal, why don't we need to set mStorReplyReceived in the mResponseCode == 1xx case?

Also, it's probably a good idea to document the member, put it next to one of the other PRPackedBools, and init it in the constructor.

Comment 6

[Bill Gianopoulos [:WG9s]]

(In reply to comment #5)
> Michal, why don't we need to set mStorReplyReceived in the mResponseCode == 1xx
> case?

The 1XX replies are intermediate status messages and do not indicate completion of the STOR operation.  IF the server is working according to specifications they should always be followed by either a successful completion or error completion status message.

Perhaps mStorReplyReceived should be changed to mStorCompleteReceived or something similar.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yeah, with that rename and the other parts of comment 5 addressed, I can review this, I think.

Comment 8

[Bill Gianopoulos [:WG9s]]

Attachment: v2

This is based on Michal's patch taking Bz's comments into account.  I changed the name of mStorReplyReceived to mStorNotComplete which changed the state of the flag and so inverted the sets and tests.  This seemed to make it more clear to me.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm not sure why reversing the sense of the member makes sense, but if you want to do that way please call it mStorRequestInProgress or something.  It looks odd to initialize mStorNotComplete to false in the constructor (since it's clearly not complete at that point).

I do think that the original mStorReplyReceived made more sense.

Comment 10

[Bill Gianopoulos [:WG9s]]

(In reply to comment #9)
> I'm not sure why reversing the sense of the member makes sense, but if you want
> to do that way please call it mStorRequestInProgress or something.  It looks
> odd to initialize mStorNotComplete to false in the constructor (since it's
> clearly not complete at that point).
> 
> I do think that the original mStorReplyReceived made more sense.

OK I will change it back to the old name.  I was originally thinking mStoreRequestINProgress. I don;t remember why I changed to this name, which I think you are right makes less sense.  I was trying to come up with a name to make it clearere why it is not set to true on a 1XX response code.  I had asked in comment 6 if you thought changing from mStorReplyReceived to mStorCompleteReceived but I really didn;t like that name either.

Comment 11

[Bill Gianopoulos [:WG9s]]

Attachment: v3

Comment 12

[Bill Gianopoulos [:WG9s]]

Attachment: v3a

This one actually applies cleanly.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 337850 [details] [diff] [review]
v3a

>+    PRPackedBool        mStorReplyReceived; // FALSE if waiting for STOR
>+                                            //   completion status from server

Take out those extra spaces before "completion", please.

And for future, 8 lines of context is good.

r+sr=bzbarsky

Please make sure to credit Michal when checking in!

Comment 14

[Bill Gianopoulos [:WG9s]]

(In reply to comment #13)

> And for future, 8 lines of context is good.

Yes, I need to either update to a later version of hg or figure out how to configure it to use and external diff command.

I don't have check-in rights so will have to attach a new patch and set the keyword.  Not sure how to make sure whoever checks it in gives Michal credit.  I will make sure to note that on the comment I add with the updated patch.

Comment 15

[Bill Gianopoulos [:WG9s]]

Attachment: patch for checkin [Checkin: Comment 16]

Please credit Michal Novotny <[e-mail]> in check-in comment.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed changeset c0796f196ac9.  Thanks, all!