No errors when saving to a full diskette

VERIFIED FIXED in mozilla1.0

Status

Core Graveyard
File Handling
--
major
VERIFIED FIXED
17 years ago
2 years ago

People

(Reporter: Bill Law, Assigned: Adam Lock)

Tracking

({topembed+})

Trunk
mozilla1.0
x86
Windows 2000
topembed+

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
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.
Keywords: nsbeta1
Target Milestone: --- → mozilla1.0
(Reporter)

Comment 2

17 years ago
Adam, can you track this down and fix it, please?  I have too many mozilla1.0 
bugs and need some help.
Assignee: law → adamlock
Target Milestone: mozilla1.0 → ---

Updated

17 years ago
Keywords: nsbeta1 → nsbeta1+, topembed+
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 3

17 years ago
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.
(Assignee)

Comment 4

17 years ago
Created attachment 73902 [details] [diff] [review]
Patch

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

Comment 5

17 years ago
Created attachment 73907 [details] [diff] [review]
New patch

New patch also tests return code of MakeOutputStream in SaveDocumentInternal.

Can I have a review of this please?
Attachment #73902 - Attachment is obsolete: true

Comment 6

17 years ago
+        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?
(Assignee)

Comment 7

17 years ago
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?
(Assignee)

Comment 8

17 years ago
Created attachment 74073 [details] [diff] [review]
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?
Attachment #73907 - Attachment is obsolete: true

Comment 9

17 years ago
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...
(Assignee)

Comment 10

17 years ago
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

17 years ago
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

17 years ago
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);
(Assignee)

Comment 13

17 years ago
Created attachment 74315 [details] [diff] [review]
Patch Mk IV

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

Updated

17 years ago
Attachment #74315 - Flags: review+

Comment 14

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

r=brade

Comment 15

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

sr=rpotts@netscape.com
Attachment #74315 - Flags: superreview+

Comment 16

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74315 - Flags: approval+
(Assignee)

Comment 17

17 years ago
Fix checked in

Comment 18

17 years ago
marking fixed.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Updated

16 years ago
Component: Networking → File Handling
QA Contact: benc → sairuh
vrfy'd fixed on win2k with 2002.09.16.08.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.