I tried saving a file (either by clicking on a .exe or something, and choosing save to disk, or, by right-clicking and choosing "save link as...") to a diskette that is full (0 bytes free). In all cases, the download is reported as completing, without error notification of any kind. I'm assigning this to myself for now because the problem doesn't really become clear until you try the new code I'm working on that adds error handling logic to our downloading code. When that code lands, and one can readily reproduce the problem, then I'll dig deeper and see what the problem is, and then reassign as necessary. I just don't want to forget about this, nor do I want it to block the landing of my error-handling code.

Similar symptom seen in other cases (e.g., when there's write permission problems; basically, anytime we get an error on creation of the target file, rather than an I/O error writing to it). Requires a line of code or two in nsWebBrowserPersist.cpp to check for errors on the target file creation.

Adam, can you track this down and fix it, please? I have too many mozilla1.0 bugs and need some help.

OnDataAvailable exits early if it gets an error code from MakeOutputStream. Fix is to set cancel when a creation error occurs and let the error drop through to be handled in the same way as an I/O error. Patch follows.

Patch tests return code from MakeOutputStream and lets it be handled like an I/O error.

New patch also tests return code of MakeOutputStream in SaveDocumentInternal. Can I have a review of this please?

+ if (file) { + file->GetUnicodePath(getter_Copies(path)); + } The above block doesn't match the format used elsewhere (curly brace on same line as if). Also, perhaps avoid the step of getting the file and just grab the uri's spec instead?

That code snippet is a straight cut and paste from the code Bill put into OnDataAvailable to display an error. Bill, is there a reason you're using GetUnicodePath here?

I'll answer my own question. The error message wants the report file path, not the spec of the URI to the user. Having said that, this behaviour doesn't make sense for uploading, so I have created this new patch. This patch is a bit bigger but does the following: 1. Moves SendStatusChange in the source and makes it into a member of nsWebBrowserPersist called SendErrorStatusChange. It used to be a static and looked weird. 2. Puts the code that gets the GetUnicodePath from the URI inside SendErrorStatusChange to avoid duplicating it in the two places the method is called. 3. Adds an else branch so that if the URI does not have path (i.e. an upload uri with no associated nsILocalFile), it calls GetSpec and reports that instead. 4. General consistency cleanup around this code. Reviews?

Comment on attachment 74073 [details] [diff] [review] New patch I know Bill wrote it this way originally, but I don't understand why we want to pass in nsnull instead of the aRequest given to us; what's the harm? + mProgressListener->OnStatusChange(nsnull, + aIsReadError ? aRequest : nsnull, aResult, msgText); (I'd rather just see us always pass in aRequest) For this change, I think you should remove nsresult (why have another "rv" variable?) + nsresult rv = MakeOutputStreamFromURI(aURI, aOutputStream); I'd prefer just: rv=MakeOutputStream...

Bill can you explain why nsnull is passed instead of the nsIRequest for a write error? I don't mind either way but I'd like to clarify and agree on the behaviour so we can get this thing reviewed.

I spoke with Bill on the phone. Here is the short summary: The "aIsReadError ? aRequest : nsnull" code should be removed from within the function but inside OnDataAvailable, we need to maintain that (pass in a null aRequest if it's a write error) so that the progress listener never gets a request/channel that is wrong.

Comment on attachment 74073 [details] [diff] [review] New patch + mProgressListener->OnStatusChange(nsnull, + aRequest, aResult, msgText); the call in OnDataAvailable could be something like: + SendErrorStatusChange(readError, rv, readError ? request : nsnull, data->mFile);

This should be ready for review now. The "readError ? request : nsnull" has been moved to where SendErrorStatusChange is called in OnDataAvailable.

Comment on attachment 74315 [details] [diff] [review] Patch Mk IV r=brade

Comment on attachment 74315 [details] [diff] [review] Patch Mk IV sr=rpotts@netscape.com

Comment on attachment 74315 [details] [diff] [review] Patch Mk IV a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Fix checked in

marking fixed.

vrfy'd fixed on win2k with 2002.09.16.08.