Closed Bug 27609 Opened 25 years ago Closed 23 years ago

Need to have alerts for all file save failures


(Core Graveyard :: File Handling, defect, P1)



(Not tracked)



(Reporter: rzach, Assigned: law)



(Keywords: dataloss)


(6 files, 12 obsolete files)

22.63 KB, text/html
11.20 KB, patch
Details | Diff | Splinter Review
17.64 KB, patch
: review+
Details | Diff | Splinter Review
5.86 KB, patch
: review+
Details | Diff | Splinter Review
1.51 KB, patch
: review+
Details | Diff | Splinter Review
3.83 KB, text/plain
Left over from bug 23821.

Need more informative alerts when file save fails due to permission violation. 
Right now it just says "Unknown error".  Same thing for similar errors (eg, no
space left on device).
Sorry, forgot to assign to law.
Assignee: don → law
QA Contact: paulmac → sairuh
Target Milestone: --- → M15
*** Bug 33320 has been marked as a duplicate of this bug. ***
Move to M16 for now ...
Target Milestone: M15 → M16
OS: Linux → All
see it on windows too.
*** Bug 32461 has been marked as a duplicate of this bug. ***
Target Milestone: M16 → M18
Move to M21 target milestone.
Target Milestone: M18 → M21
Component: XP Apps → XP Apps: GUI Features
The file `KingFred.html' could not be saved to the folder `Documents', because 


( Save As ... )           ( Cancel ) (( Retry ))  [Mac, X]
           (( Retry )) ( Save As ...) ( Cancel )  [Windows]

For lack of write permission:
* reason = `you do not have permission to change the contents of that folder'
* suggestion = `Change the folder properties and try again, or try saving in a
  different location.'
  (Do you have an XP way of telling whether it's a floppy which is

Lack of space:
* reason = `there is not enough room on the disk'
  (Ideally this would be followed by `(42 kb needed, 15 kb available)', but
  that's getting into RFE territory.)
* suggestion = `Make more space on the disk and try again, or try saving in a
  different location.'

Unknown error:
* reason = `an unknown error occurred'
* suggestion = nothing

Any other causes?
*** Bug 60266 has been marked as a duplicate of this bug. ***
i'm bumping up the sev on this a bit, due to lorca's observations in bug 60266.

bill/mscott, shouldn't we throw a dialog after the hits OK in the filepicker
dialog, saying something to the effect of, "cannot save since you don't have the
proper permissions for this directory"? since the download progress dialog does
now appear, this gives the perception of dataloss.
Severity: minor → major
Keywords: correctness
Yes, we should inform the user about what happens.

I had a fix for at least one aspect of this general problem but it got nixed for
rtm.  We need to merge the file download logic and make sure that all paths
properly handle errors.  There's lots of bugs relating to that; if there weren't
so many, maybe I could sort them all out :-(.
Raising Severity due to data loss status.  Adding myself to cc: list.

From DUPLICATE 60266 (that I wrote up):
Happens in: Linux 11-08-09
Does not happen in: Mac/Win->N/A

Reason for Severity: Data loss/silent data loss/perceived data loss

Reproducible: Always.

Steps to Reproduce: 
1. Open up a download box
2. Select directory to save in-make sure its not normally accessible to your 
particular account
3. Watch download/save progress
4. Confirm "Download/Save Complete" dialog box.
5. Check directory for contents

Actual Results: Nothing there.
Expected Results: File downloaded to be there.
Severity: major → critical
Nominating for nsbeta1.  This overlaps with other bug(s) but I want to flag it
Keywords: nsbeta1
Netscape Nav triage team: this is a Netscape beta stopper.  Changed priority to 
Priority: P3 → P2
Target Milestone: --- → mozilla0.8
Did a fix for this get checked in last night?
Almost.  I added error handling code in the xfer code.  That isn't used all the 
time, yet.  I'm using bug 67216 to track that issue.

So, I'm marking this one as dependent on that bug.
Depends on: 67216
Target Milestone: mozilla0.8 → mozilla0.9
Adjusting keywords; dependent bug 67216 should be the one marked nsbeta1.
Keywords: nsbeta1
Marking nsbeta1+ since we should fix this
Keywords: nsbeta1+
nav triage team:

Yes, this sucks, but push has come to shove. Bumping from mozilla0.9 train,
marking nsbeta1-, resetting target milestone
Keywords: nsbeta1+nsbeta1-
Target Milestone: mozilla0.9 → ---
Marking nsbeta1+, mozilla0.9
Keywords: nsbeta1-nsbeta1+
Target Milestone: --- → mozilla0.9
I'm not getting to this dialog till next round...
Target Milestone: mozilla0.9 → mozilla0.9.1
nav triage team:

Marking mozilla0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Depends on: 80242
No longer depends on: 80242
*** Bug 80242 has been marked as a duplicate of this bug. ***
<q src="mpt" date="2000-07-26 20:38"> Any other causes?</q>
3. Enter a pathname which doesn't exist (I had a c:\download directory, but
accidently typed in the plural c:\downloads instead)

Actual Results:  "There was an error writing to the target location"

Expected Results:  "This directory does not exist.  Should it be created?"
bill/bryner, are there even hooks for creating directories from the file
nav triage: frequency of this problem and severity are low enough to defer to 
Target Milestone: mozilla0.9.2 → mozilla0.9.3
nav triage team:

Unfortunately, the work involved to fix this problem the right way is too much to 
take for mozilla0.9.3. Pushing out to mozilla1.0
Target Milestone: mozilla0.9.3 → mozilla1.0
Keywords: nsenterprise
Removing nsenterprise nomination.
Keywords: nsenterprise
spam: over to File Handling.
Component: XP Apps: GUI Features → File Handling
->098, actual text used is no longer the issue in this bug, we need to ensure
that users are alerted to failed saves.  updating summary to reflect bug morphage.
Summary: Need more informative alert for file save failure → Need to have alerts for all file save failures
Target Milestone: mozilla1.0 → mozilla0.9.8
related: bug 65770 - not getting an alert when disk full for file downloads.
*** Bug 87438 has been marked as a duplicate of this bug. ***
*** Bug 108669 has been marked as a duplicate of this bug. ***
*** Bug 109069 has been marked as a duplicate of this bug. ***
*** Bug 105718 has been marked as a duplicate of this bug. ***
*** Bug 112175 has been marked as a duplicate of this bug. ***
*** Bug 113062 has been marked as a duplicate of this bug. ***
*** Bug 116753 has been marked as a duplicate of this bug. ***
I think my bug (116753) should be blocked by this, rather then duped, as it is a
specific issue, on only one type of OS, which warrants seperate verification
after this fix is in. Unless there is contention, I'll undupe it tomarrow.
*** Bug 115197 has been marked as a duplicate of this bug. ***
Depends on: 117228
This document describes how I plan to fix this problem (once and for all). 
Anybody who cares about how we do this (or has an interest in the components
that are modified) should have a look and offer feedback, ASAP.
No longer depends on: 67216
Blocks: 67216
Blocks: 73106
All these download related items are moving out; delayed due to overhaul of 
downloading and many regressions in this area.
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Attachment #64387 - Attachment is obsolete: true
Priority: P2 → P1
Blocks: 72982
Blocks: 117258
Blocks: 91828
Blocks: 92508
Blocks: 98394
Blocks: 90074
Keywords: helpwantednsbeta1
This patch makes the uriloader use the new nsIProgressDialog.

It does not try to reuse nsIWebBrowserPersist as its stream listener.  The main
drawback is that there are still two places where we have network I/O that has
to check for errors, etc.  I will try to see if I can re-use the
nsWebBrowserPersist as the uriloader's stream listener, but I'm worried that
that might be hard.

So, this is an interim fix to add error handling and use the new and improved
progress dialog.
OK, I need some reviews, approvals, and super-reviews now.

There are 4 separate items:

1. The new nsIProgressDialog interface and implementation in 
mozilla/embedding/components/ui/progressDlg.  The files of interest are all 
named nsProgressDialog.* (the ones named nsProgressDlg.* are obsolescent).  I 
need a volunteer to review it; I'd like either or both of ben and mscott to 

2. The nsWebBrowserPersist changes, attached to this bug.  Adam Lock needs to 
review/approve these changes.  I need a volunteer to super-review (ben or 

3. The contentAreaUtils.js changes, attached to this bug.  I need a volunteer 
to review this (Boris? I know you've been hacking code real near this); Ben 
should super-review/approve.

4. The uriloader/exthandler changes, attached to this bug.  I need a volunteer 
to review (Boris?); mscott should super-review/approve.

The only open issues are:
a. Some problems related to handling errors returned on OnStopRequest.  E.g., 
if I abort a download from my ftp server console, I get an alert saying "The 
operation was abortted by the system administrator" (or something like that) 
but the progress dialog then jumps to 100% and indicates the download is 
completed.  Neither nsWebBrowserPersist nor the uriloader seem to look for 
errors and pass them to the dialog.  I've marked where they should do that with 
comments but some code still needs to be written.
b. There is some flakiness in the networking code (I think) when saving to a 
diskette.  I've opened bug 124035 and bug 124036 for those problems.  The one 
may be due to an error occuring when we create the file transport, which we're 
not handling.  The timing one requires some investigation.
I forgot this file in the nsWebBrowserPersist patch.  This needs to be added in
Attachment #65500 - Attachment is obsolete: true
This patch doesn't build as-is on Linux because
embedding/components/ui/progressDlg isn't hooked up to the build.
Attached patch unix build changes (obsolete) — Splinter Review
brian or bill, have you had a chance to test these patches on Mac [either 9.x or
10.1.x]? just curious. :)
Thanks, Brian; I had those unix makefile changes on my to-do list.

Sarah, no Mac testing, yet.  But it's all xp code so it should work fine :-).

There does need to be comparable tweaks to the Mac build scripts, though.  I'll
work on that.
Comment on attachment 68248 [details] [diff] [review]
Patch to nsWebBrowserPersist to work with new nsIProgressDialog


A couple of nitpicks. First (and this is a holdover from the old code) we
should	use PRUint32 instead of unsigned int for bytesRead/written/remaining

And can you unindent the switch statements so the case line up underneath the
Attachment #68248 - Flags: review+
Comment on attachment 68268 [details] [diff] [review]
Patch to uriloader to make it work with the new nsIProgressDialog


can we pull the string bundle #define properties line out and up at the top of
the file instead of nested in the if clause just for clarity? 

Just for kicks can you make sure you can still open a large mail attachment
after these changes?
Attachment #68268 - Flags: superreview+
Comment on attachment 68248 [details] [diff] [review]
Patch to nsWebBrowserPersist to work with new nsIProgressDialog

sr=mscott modulo Adam's comments about using PRUint32s instead of "unsinged
Attachment #68248 - Flags: superreview+
re: comment 54

I've made the changes suggested; thanks.
Attachment #68248 - Attachment is obsolete: true
Minor grouch: Progress should never be shown in a dialog (not minimizable, not
closable). Normally it will be in a progress window (minimizable, not closable);
the exception is the download manager, where it will be in a document window
(minimizable, closable). So calling anything `nsIProgressDialog' is just going
to confuse people who use the API ... s/ProgressDialog/ProgressWindow/g would be
nice. :-)
Comment on attachment 68251 [details] [diff] [review]
Patch to contentAreaUtils.js to make it work with new nsIProgressDialog

>+      var filesFolderLeafName = getStringBundle().GetStringFromName("filesFolder");
>+      filesFolderLeafName = filesFolderLeafName.replace(/\^BASE\^/, nameWithoutExtension);

Any reason not to use FormatStringFromName here? 

>+      if (!filesFolder.exists())
>+        filesFolder.create(lfIID.DIRECTORY_TYPE, 0755);
>+    }

I'd suggest not doing this.  This is done in any case by the WebBrowserPersist
code (see nsWebBrowserPersist::SaveDocumentInternal) and that code actually
has the smarts to not create this dir if there are no linked resources to
persist. Let's not ruin that over here.  :)

>+dump("onStateChange exception=" + e + "\n" );

This is going away before checkin, right?  :)
Comment on attachment 68268 [details] [diff] [review]
Patch to uriloader to make it work with the new nsIProgressDialog

> 		  helperAppDlg \
>+          progressDlg \
> 		  plugin \

Fix the indentation (

>+NS_IMETHODIMP nsExternalAppHandler::Observe(nsISupports *aSubject, const char *aTopic, const PRUnichar *aData )
>+  // User pressed cancel button on dialog.
>+  return this->Cancel();

Should we check aTopic?  If it's always going to be "oncancel" (as the
interface comments for nsIProgressDialog suggest), then the "observer"
attr of nsIProgressDialog should perhaps be renamed "cancelObserver" for
clarity....  As it stands people may be tempted to make things other
than cancelling observable and then this code could fail.

>       mWebProgressListener = aWebProgressListener;
>-      webProgress->AddProgressListener(mWebProgressListener);

>+  mTimeDownloadStarted = PR_Now();

Why move this?	It seems more correct to call that right at the end of
OnStartRequest as before....

> while (count > 0) // while we still have bytes to copy...

Shouldn't this be | while (NS_SUCCEEDED(rv) && count > 0) | ?
Otherwise we'll go into an infinite loop on a read error....

>+          rv = mOutStream->Write(mDataBuffer, numBytesRead, &numBytesWritten);

That should be bufPtr instead of mDataBuffer, no?

>+    if (action!=nsIMIMEInfo::saveToDisk)

add spaces around the "!="

>+      if(openWith.IsEmpty())

Space after the "if" as elsewhere in this file?  Same for the "if"s inside
this one....

>+      // Tell progress dialog what we're opening with.
>+      progressDlg->SetOpeningWith(openWith);

I'd say that we should either have a default value for "openWith" in case it's
_still_ empty after the nsILocalFile dance or assert that it's not empty.
I'm not sure how it can be empty at this point without something being
deeply wrong...  Also, you want openWith.get() to get a wstring, no?

>+  PRInt32 mProgress;

Could we add a brief comment explaining what this integer is?
Comment on attachment 68270 [details] [diff] [review]
Additional nsWebBrowserPersist file

>diskFull=There is not enough free space to store the file.  Free up space and try again, or, choose a different target location.

There should be no comma after the "or".
I'm not sure we want to have the "try again" text in there.

Also, I think we should have two different error messages:

1)  Disk full when writing to our temp file in the uriloader.  Since the user
    has no way to easily configure where we put this file (short of shutting
    down mozilla, changing some environment variables, and restarting) we
    just say that the user should free up some space.  Bonus points for adding
    the name of the directory or file we were saving to in this alert so the
    user knows _where_ to free up space (this would replace "the file" in the
2)  Disk full when writing to the file/dir the user chose to save to when doing
    "Save As" (error in nsWebBrowserPersist).  In this case the error should
    be similar to what we have now, again with "the file" replaced by the
Attachment #68270 - Flags: needs-work+
Apparently the %1, %2, etc in that attachment of mine should be something else, 
but no-one in #mozilla seems to know precisely what. :-)
I didn't say that. bz said to use %s, he's right. I merely noted that if the 
order didn't match (eg if you localized to spanish or something) and needed to 
use the second param first, you could.
Re: Scott's comments (in comment 55)

I've removed the #define for |properties| and just put the chrome url in the
call as a literal.  Likewise modified in nsWebBrowserPersist.

Re: Boris's comments (in comment 60)

I've made all those changes, except for:
* the moving of the setting of mTimeDownloadStarted; this value is needed in
ShowProgressDialog, which gets called before the previous setting of this field
occurred.  That could be worked around somehow, but it was easier to just move
this up a few lines.  The data is coming in, so it's not like we're lying.
* SetOpenWith(openWith) not using |.get()|.  It doesn't seem necessary and I
prefer to let the compiler figure out the best way of passing the argument.  Is
that politically incorrect for some reason?

Updated patches coming soon.
re: comment 59

>Any reason not to use FormatStringFromName here? 

No, but I just copied this code from nsProgressDlg.js as is.  I suppose I can
fix it now.

>>+      if (!filesFolder.exists())
>>+        filesFolder.create(lfIID.DIRECTORY_TYPE, 0755);
>>+    }
>I'd suggest not doing this

Ditto for this.  I'll remove these lines now, based on your say so.  Ben/Adam,
you guys seemed to want this before; please holler if removing this is a mistake.

>+dump("onStateChange exception=" + e + "\n" );

Deleted (was temporary debug code).
re: comment 61...comment 64

I'm not sure what to make of the "suggestion" entries in mpt's new

We are a bit constrained here in that the code can only return a single string
(from nsWebBrowserPersist.cpp and nsExternalHelperAppService.cpp) to the
progress dialog code.

mpt described his proposal as "ideal," but I'm stuck here in the real world.

I can add code to nsProgressDialog.js to look for some specific return values
(the NS_ERROR_FILE_* ones), at which point I could fetch additional "suggestion"
strings for use in the alert.  But, given that there seems to be only one
suggestion for each error, I'd rather just combine the two sentences in the same
property string (I'm ignoring the implied different suggestion in the case of
the user running out of space on the same volume as the trash/recycle-bin
because that's too hard to check for anyway and users will just have to figure
that out, I say).

I think the suggestions for putting the file names and amount of space still
needed are useful.  I'm going to work on mashing together mpt's error text and
suggestion strings and getting the file name and size inserted, as he and Boris

I may not get to that real soon, though.  Can I get OK to check in what I have
and deal with improving the messages via another bug?
> SetOpenWith(openWith) not using |.get()|.

There was a push at some point to remove operator char* from all the string
classes (bug 53057).  I guess nsXPIDLC?String is safe from that for now, so do
whichever you prefer here.  :)

Blake commented (in another bug) that the title bar should read something like:

   37% - Saving

i.e., move the percentage to the beginning so it appears in the task bar.

I like the idea of getting as much info as possible into the task bar, but I'm 
not sure this doesn't make the full title look a little awkward.

I think we should only show the percentage in the titlebar when the window is
minimized, a la IE.  This is very easy now that minimized/maximized/restored
state flags have been implemented on nsIDOMChromeWindow.

Someone commented that they want it in the titlebar all the time in case they
have something obscuring the rest of that window, but that hardly seems like the
common case to me.  I think always displaying it in the titlebar as well as the
window is overkill, and awkward as you point out.
I'd like to ditto Blake's comments.
A couple comments on the nsWebBrowserPersist changes:
 * could the block of new code (switch) in OnDataAvailable be moved to a
separate method?  (I worry this block may need to grow to accommodate new errors.)

 * I'm concerned about the string bundle dependency.  Wouldn't it be better if
the persist code returned a specific set of error codes which the client/caller
could interpret/describe as it saw fit?
Hardware: PC → All
re: moving the switch statement

Sounds reasonable.  It wouldn't need to be a method (member function), though. 
Is a static utility function OK?  That would make it easier for me to cut/paste
it into the uriloader code.

re: property file dependency

This was suggested by Jud Valeski so that the code could be shared by clients
that didn't care to embellish the alert text at all.  Also, it kind of matches
the strategy deployed by the networking code.

A more pragmatic reason is that it is difficult, in general, to pass nsresult
values to JavaScript code because they're only defined by C++ macros (although
some, including all that we'd want to examine now, are also available to JS via

I wasn't thrilled with this scheme, either, but it's what we decided to do when
the design was being debated.
As a note, the patches in this bug do not solve the problem of MoveTo() failing
in the uriloader due to lack of space or some such.  This is covered by bug
Your note is correct.  Nor does it handle some cases where the error
notifications come in via OnStopRequest.

If I get to those things soon, then I'll incorporate them; otherwise, that will
have to wait a bit longer.
I've updated this patch to refactor the error notification code into a separate
routine as suggested by Kathy Brade.

I've also slightly tweaked the code she added recently to call StartUpload. 
That should only happen if no error has occurred.
Attachment #68684 - Attachment is obsolete: true
Updated patch to uriloader/exthandler stuff.

This refactors the code that sends the error notifications as was done in

I also added some error checking to handle errors when moving the temp file. 
This is less that ideal still, because the return value is always
NS_ERROR_FAILURE so the message ends up as the generic "There was an error
writing to the target location."  nsILocalFile implementationsof MoveFile are
going to have to be fixed to give better NS_ERROR_FILE_* messages.

Likewise, I added error checking after calling LauchAppWithTempFile.  That
doesn't quite work though, because it seems the implementors of that function
don't return error codes.
Attachment #68268 - Attachment is obsolete: true
Updated patch; no material changes.
Attachment #68251 - Attachment is obsolete: true
re: progress window title

OK, I'm trying to wrap this part up and I see where IE (v5.5) uses
  "37% of Completed"
and that's the title all the time.

It would be easier if I didn't have two distinct titles so I wonder if I can 
just use the same technique?  That would also permit me to ignore differences 
when saving versus opening-with.


Also, noboby has come up with a solid proposal for the error alert text.  I'm 
going to proceed with my best shot at merging the comments that have been 
received.  I don't think we need to embellish the "out of space" message beyond 
inserting the target file path (the about downloaded so far is in the progress 
window and if users are counting their bytes that closely, they can do the 
Comment on attachment 69774 [details] [diff] [review]
Revised patch for uriloader/exthandler code

r=bzbarsky.  Make sure to make the mac build changes corresponding to those
REQUIRES changes.
Attachment #69774 - Flags: review+
Comment on attachment 69776 [details] [diff] [review]
Updated patch to contentAreaUtils

>+    var encodingFlags = flags;
>+    if (persistArgs.contentType == "text/plain") {
>+      encodingFlags |= nsIWBP.ENCODE_FLAGS_FORMATTED;
>+      encodingFlags |= nsIWBP.ENCODE_FLAGS_ABSOLUTE_LINKS;
>+      encodingFlags |= nsIWBP.ENCODE_FLAGS_NOFRAMES_CONTENT;        
>+    }

Do we really want to have "PERSIST_FLAGS_NO_CONVERSION" and
in the encoding flags?	That seems odd....  For one thing as encoding flags
that would
just make it explicit.

Also, could we always pass nsIWBP.ENCODE_FLAGS_ENCODE_ENTITIES?  that will fix
bugs we have on entities being saved as characters in some charset instead...

The rest looks good.
I suspect that encodingFlags stuff is wrong.  I just cribbed that code from 
elsewhere and may have garbled it in the translation.  The code I copied it 
from was messed up, too, as I recall.

I'll straighten that out.
I've rolled into this patch the new .properties file, in which I've
incorporated mpt's wording (for the most part).  Each message incorporates the
target file path as an insert.
Attachment #68270 - Attachment is obsolete: true
Attachment #68301 - Attachment is obsolete: true
Attachment #68790 - Attachment is obsolete: true
Attachment #69769 - Attachment is obsolete: true
I've enhanced the error handling to insert the file name (temporary vs. final)
into the messages.
Attachment #69774 - Attachment is obsolete: true
Fixes the encodingFlags snafu and changes the title text to put the percentage
Attachment #69776 - Attachment is obsolete: true
Blocks: 126258
Comment on attachment 70175 [details] [diff] [review]
Final(?) contentAReaUtils.js patch

Attachment #70175 - Flags: review+
Comment on attachment 70173 [details] [diff] [review]
Final(?) uriloader/exthandler patch

Attachment #70173 - Flags: review+
nsbeta1+ per nav triage team
Keywords: nsbeta1nsbeta1+
fix checked in
Closed: 23 years ago
Resolution: --- → FIXED
No longer blocks: 91828
The nsWebBrowserPersist changes created a new compiler warning:

 `PRBool readError' might be used uninitialized in this function

I am not absolutely sure if this can really happen, or it's just a compiler
being stuping, but it seems that if function is called with 0 aLenght, something
indeed might happen...

Resolution: FIXED → ---
Another warning caused by this check-in:

 `PRBool readError' might be used uninitialized in this function

I looked at that when I was reviewing.. I don't think onDataAvailable is ever
called with a 0 data length (what would be the point?).

We can add an initializer just for kicks, I suppose....
FYI, I've had bugs in the past where the length in an ODA call was 0 and I had
code which wasn't coping with that properly. So it does happen.
OK, looking over the code again nsExternalHelperAppService will never hit the
code that uses readError with readError unset.	WebBrowserPersist could do it
if mCancel is true....

Attaching simple patch to fix warnings.
Comment on attachment 70686 [details] [diff] [review]
Patch to fix warnings

Attachment #70686 - Flags: review+
Mozilla lets you open new bugs, too.  I suggest we do that in this case.

And by the way, the reporting of errors while saving files has been totally
fucked up from the git go.  If we find some residual problem in that area, it
would not surprise me.  In fact, if we don't, that would shock me.  But when it
happens. let's please open new bugs specific to the remaining problems rather
than keep re-opening this one. 
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
Am I right assuming that marking thius bug as "fixed" would not stop attachment
70686 [details] [diff] [review] from going in? Or should I open a new bug for the warnings?

I'd open a separate bug.  Just assign it to me.
tested using 2002.03.06 commercial verif builds on linux rh7.2, mac OS X v10.1.3
and win2k. seems that there are four basic test cases used to verify this bug:

a. write permissions
b. out of space in target directory
c. check that target directory exists
d. out of space in temp directory

detailed results are below. since issues still remain here, i'm not sure if i
should verify this particular bug. however, i have spun off or reopened bugs as
needed, so maybe this should get closed...?

in any case, there still are related bugs which might be part of the dependency
tree. will verify those seperately (when fixed) since they haven't been dup'd
here. i wish to verify this particular bug in a finite manner. :)

a. write permissions. primarily Linux and prolly mac OS X. make sure to display
an alert when the user lacks the priv to write to targetted directory. many dups.

example: create a dir that's owned by root (or another user) and give it 700
mode. the try to save file to that dir.


* linux: PASS. get an alert dlg saying "Cannot create file. Directory [path] is
not writable."

* mac: PASS. in the file picker, folders that don't have the proper privs are
simply inaccessible, eg, greyed out or have a lock icon.

* win2k: tested with a locked floppy. PASS, sort of. got the alert dlg "The disk
cannot be written to because it is write protected. Please remove the write
protection from the volume in drive A:." but the download progress remained and
appeared completed --even though no saving occurred.

b. out of space. All platforms. should get an alert where download cannot
complete due to lack of space. many dups. cleanup issues remain: see bug 129332.

linux example: try to save to full/limited harddisk --different partition than /tmp.
mac example: try to save to full/limited Zip disk.
win2k example: try to save to full/limited ramdisk.


* linux: FAIL. see bug 129614. seemed to download successfully, but it was
incomplete. no error dlg came up --and the salted file persisted in /tmp even
after quitting. will try w/floppy later on...

* mac: PASS. got an alert dlg "There is not enough room on the disk to save to
Zip 100:[file]. Remove unnecessary files from that disk and try again, or try
saving in a different location."

* win2k: FAIL, also see bug 129614.

one-time occurrence, though, on win2k: got an alert dlg "A:\[file] could not be
saved, because an unknown error occurred. Sorry about that. Try saving to a
different location."

c. make sure target directory exists. All platforms. was bug 80242.

example: in file picker for saving, enter a non-existent directory name.


* linux: PASS. get an alert dlg saying " Path [path] doesn't exist, can't save

* mac: PASS. in the file picker for saving, i'm prevented from entering a bogus
directory --unlike the file picker for opening, which has a "go to" field for
such a purpose.

* win2k: FAIL. reopened bug 80242. bogus dir is created anyway, and file is
saved there.

d. since we first download to a temp location, then move to user-determined
target, if there's not enough room in the temp location, we should display an
alert. All platforms. was bug 105718 and bug 112175.

linux example: need to fill up /tmp a lot, then test saving.
mac example: need to fill up Desktop folder, then test saving.
win2k example: need to fill up c:\WINNT\Temp, then test saving.


* linux: FAIL. see bug 129604.

* mac: overall FAIL when the default temp location is the Desktop --same results
as linux. this occurred with a new profile. also see bug 129604. one-time
"success": got an alert dlg "There is not enough room on the disk to save to
[non-Desktop-remembered location]:[salted filename]. Remove unnecessary file
from the disk and try again, or try saving to a different location." this.
however, was for an existing profile with that pref set.

* win2k: FAIL, also see bug 129604.
Hmmm.  Maybe 8 test cases, because clicking on a link and going through the 
helper app dialog (open-with vs. save-to-disk) is completely distinct from 
right clicking and choosing save-link-as.

I'm guessing all your tests were using the former, which complicates things in 
a number of ways (especially prone to error when the downloaded copy in temp is 
moved/copied to the final destination).

I think things work more reliably if you use the save-link-as technique.  Not 
that the problems you cite aren't problems, too.  They're just more likely 
problems in the nsExternalAppHandlerService code.
Sairuh--when you are testing "write permissions" be sure to test not only
directory permissions but also file permissions.  On a Mac, you can lock a file
by selecting it and choosing "File | Get Info | General Information" and then
checking the box near the bottom.  I'm sure you know how to change the
permissions on Linux files.  I think you can lock Windows-files but I don't
recall how at the moment.
Bug 131108 was just opened, which tells us that we are not dealing well with
saves to files on which we do not have write-permissions.
that bug is a duplicate of a separate bug which was recently fixed
hokay, finally ran through the more common scenarios, but this time using "save
link target as" from the link context menu.

this will be a partial rubberstamp verification, since i didn't repeat the cases
of a full temp location or full linux partition. if anyone would like to test
those scenarios, pls do add your observations/methods here!

anyhow, the detail tests and results (based on 2002.05.02.0x-1.0.0 branch comm
bits) are at --will post as
an attachment soon, since it's on an internal server.
Keywords: qawanted
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.