124035 - No errors when saving to a full diskette

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Bill Law]

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.

Comment 1

[Bill Law]

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.

Comment 2

[Bill Law]

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

Comment 3

[Adam Lock]

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.

Comment 4

[Adam Lock]

Attachment: Patch

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

Comment 5

[Adam Lock]

Attachment: New patch

New patch also tests return code of MakeOutputStream in SaveDocumentInternal.

Can I have a review of this please?

Comment 6

[Kathleen :Brade]

+        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?

Comment 7

[Adam Lock]

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?

Comment 8

[Adam Lock]

Attachment: New patch

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 9

[Kathleen :Brade]

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...

Comment 10

[Adam Lock]

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.

Comment 11

[Kathleen :Brade]

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 12

[Kathleen :Brade]

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);

Comment 13

[Adam Lock]

Attachment: Patch Mk IV

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

Comment 14

[Kathleen :Brade]

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

r=brade

Comment 15

[rpotts (gone)]

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

sr=[e-mail]

Comment 16

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 17

[Adam Lock]

Fix checked in

Comment 18

[Blake Ross]

marking fixed.

Comment 19

[sairuh (rarely reading bugmail)]

vrfy'd fixed on win2k with 2002.09.16.08.