27609 - Need to have alerts for all file save failures

Status: VERIFIED FIXED

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

Description

[Richard Zach]

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

Comment 1

[Richard Zach]

Sorry, forgot to assign to law.

Comment 2

[Bill Law]

Definitely.

Comment 3

[Sean Richardson]

*** Bug 33320 has been marked as a duplicate of this bug. ***

Comment 4

[don]

Move to M16 for now ...

Comment 5

[shrirang khanzode]

see it on windows too.

Comment 6

[R.K.Aa.]

*** Bug 32461 has been marked as a duplicate of this bug. ***

Comment 7

[don]

Move to M21 target milestone.

Comment 8

[Matthew T (active 1999-2002)]

The file `KingFred.html' could not be saved to the folder `Documents', because 
{reason}.

{suggestion}

( 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
  write-protected?)

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?

Comment 9

[sairuh (rarely reading bugmail)]

*** Bug 60266 has been marked as a duplicate of this bug. ***

Comment 10

[sairuh (rarely reading bugmail)]

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.

Comment 11

[Bill Law]

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

Comment 12

[Loco]

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.

Comment 13

[Bill Law]

Nominating for nsbeta1.  This overlaps with other bug(s) but I want to flag it
anyway.

Comment 14

[Marcia Knous [:marcia]]

Netscape Nav triage team: this is a Netscape beta stopper.  Changed priority to 
P2.

Comment 15

[Jason Eager]

Did a fix for this get checked in last night?

Comment 16

[Bill Law]

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.

Comment 17

[Bill Law]

Adjusting keywords; dependent bug 67216 should be the one marked nsbeta1.

Comment 18

[Paul Chen]

Marking nsbeta1+ since we should fix this

Comment 19

[Paul Chen]

nav triage team:

Yes, this sucks, but push has come to shove. Bumping from mozilla0.9 train,
marking nsbeta1-, resetting target milestone

Comment 20

[Paul Chen]

Marking nsbeta1+, mozilla0.9

Comment 21

[Bill Law]

I'm not getting to this dialog till next round...

Comment 22

[Paul Chen]

nav triage team:

Marking mozilla0.9.2

Comment 23

[sairuh (rarely reading bugmail)]

*** Bug 80242 has been marked as a duplicate of this bug. ***

Comment 24

[timeless]

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

Comment 25

[sairuh (rarely reading bugmail)]

bill/bryner, are there even hooks for creating directories from the file
pickers?

Comment 26

[Viswanath Ramachandran]

nav triage: frequency of this problem and severity are low enough to defer to 
m0.9.3. 

Comment 27

[Paul Chen]

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

Comment 28

[Jussi-Pekka Mantere]

Removing nsenterprise nomination.

Comment 29

[sairuh (rarely reading bugmail)]

spam: over to File Handling.

Comment 30

[Peter Trudelle]

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

Comment 31

[sairuh (rarely reading bugmail)]

related: bug 65770 - not getting an alert when disk full for file downloads.

Comment 32

[Bill Law]

*** Bug 87438 has been marked as a duplicate of this bug. ***

Comment 33

[timeless]

*** Bug 108669 has been marked as a duplicate of this bug. ***

Comment 34

[R.K.Aa.]

*** Bug 109069 has been marked as a duplicate of this bug. ***

Comment 35

[Bill Law]

*** Bug 105718 has been marked as a duplicate of this bug. ***

Comment 36

[timeless]

*** Bug 112175 has been marked as a duplicate of this bug. ***

Comment 37

[R.K.Aa.]

*** Bug 113062 has been marked as a duplicate of this bug. ***

Comment 38

[Boris Zbarsky [:bzbarsky]]

*** Bug 116753 has been marked as a duplicate of this bug. ***

Comment 39

[Jeremy M. Dolan]

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.

Comment 40

[Jeremy M. Dolan]

*** Bug 115197 has been marked as a duplicate of this bug. ***

Comment 41

[Bill Law]

Attachment: Proposal describing plan for fixing this; requires changes to nsIWebBrowserPersist, the uriloader, navigator.js, etc. so soliciting input before proceeding.

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.

Comment 42

[Bill Law]

All these download related items are moving out; delayed due to overhaul of 
downloading and many regressions in this area.

Comment 43

[Bill Law]

Attachment: Update to proposal; see change history for details.

Comment 44

[Bill Law]

Attachment: Patch to nsWebBrowserPersist to work with new nsIProgressDialog

Comment 45

[Bill Law]

Attachment: Patch to contentAreaUtils.js to make it work with new nsIProgressDialog

Comment 46

[Bill Law]

Attachment: Patch to uriloader to make it work with the new nsIProgressDialog

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.

Comment 47

[Bill Law]

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

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 
mscott?).

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.

Comment 48

[Bill Law]

Attachment: Additional nsWebBrowserPersist file

I forgot this file in the nsWebBrowserPersist patch.  This needs to be added in
mozilla/embedding/components/webbrowserpersist/locale/en-US
as nsWebBrowserPersist.properties.

Comment 49

[Bill Law]

Attachment: Final proposal/documentation.

Comment 50

[Brian Ryner (not reading)]

This patch doesn't build as-is on Linux because
embedding/components/ui/progressDlg isn't hooked up to the build.

Comment 51

[Brian Ryner (not reading)]

Attachment: unix build changes

Comment 52

[sairuh (rarely reading bugmail)]

brian or bill, have you had a chance to test these patches on Mac [either 9.x or
10.1.x]? just curious. :)

Comment 53

[Bill Law]

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 54

[Adam Lock]

Comment on attachment 68248 [details] [diff] [review]
Patch to nsWebBrowserPersist to work with new nsIProgressDialog

r=adamlock

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

And can you unindent the switch statements so the case line up underneath the
switch?

Comment 55

[Scott MacGregor]

Comment on attachment 68268 [details] [diff] [review]
Patch to uriloader to make it work with the new nsIProgressDialog

sr=mscott

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?

Comment 56

[Scott MacGregor]

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
int"

Comment 57

[Bill Law]

Attachment: nsWebBrowserPersist changes incorporating comments from Adam

re: comment 54

I've made the changes suggested; thanks.

Comment 58

[Matthew T (active 1999-2002)]

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 59

[Boris Zbarsky [:bzbarsky]]

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 60

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 68268 [details] [diff] [review]
Patch to uriloader to make it work with the new nsIProgressDialog

> 		  helperAppDlg \
>+          progressDlg \
> 		  plugin \

Fix the indentation (makefile.win).

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

[Boris Zbarsky [:bzbarsky]]

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
should
    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
    message).
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
actual
    path.

Comment 62

[Matthew T (active 1999-2002)]

Attachment: Ideal replacement for additional nsWebBrowserPersist file

Comment 63

[Matthew T (active 1999-2002)]

Apparently the %1, %2, etc in that attachment of mine should be something else, 
but no-one in #mozilla seems to know precisely what. :-)

Comment 64

[timeless]

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.

Comment 65

[Bill Law]

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.

Comment 66

[Bill Law]

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

Comment 67

[Bill Law]

re: comment 61...comment 64

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

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

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?

Comment 68

[Boris Zbarsky [:bzbarsky]]

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

Comment 69

[Bill Law]

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

   37% - Saving foobar.zip

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.

Comments?

Comment 70

[Blake Ross]

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.

Comment 71

[Scott MacGregor]

I'd like to ditto Blake's comments.

Comment 72

[Kathleen :Brade]

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?

Comment 73

[Bill Law]

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
Components.results).

I wasn't thrilled with this scheme, either, but it's what we decided to do when
the design was being debated.

Comment 74

[Boris Zbarsky [:bzbarsky]]

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

Comment 75

[Bill Law]

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.

Comment 76

[Bill Law]

Attachment: Updated patch to nsWebBrowserPersist

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.

Comment 77

[Bill Law]

Attachment: Revised patch for uriloader/exthandler code

Updated patch to uriloader/exthandler stuff.

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

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.

Comment 78

[Bill Law]

Attachment: Updated patch to contentAreaUtils

Updated patch; no material changes.

Comment 79

[Bill Law]

re: progress window title

OK, I'm trying to wrap this part up and I see where IE (v5.5) uses
  "37% of big.zip 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.

Thoughts?

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

Comment 80

[Boris Zbarsky [:bzbarsky]]

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.

Comment 81

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 69776 [details] [diff] [review]
Updated patch to contentAreaUtils

>+  const flags = nsIWBP.PERSIST_FLAGS_NO_CONVERSION | nsIWBP.PERSIST_FLAGS_REPLACE_EXISTING_FILES;
...
>+    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
"PERSIST_FLAGS_REPLACE_EXISTING_FILES"
in the encoding flags?	That seems odd....  For one thing as encoding flags
that would
be ENCODE_FLAGS_PREFORMATTED and ENCODE_FLAGS_WRAP....	Is that purposeful?  If
so,
just make it explicit.

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

The rest looks good.

Comment 82

[Bill Law]

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.

Comment 83

[Bill Law]

Attachment: Final(?) nsWebBrowserPersist changes

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.

Comment 84

[Bill Law]

Attachment: Final(?) uriloader/exthandler patch

I've enhanced the error handling to insert the file name (temporary vs. final)
into the messages.

Comment 85

[Bill Law]

Attachment: Final(?) contentAReaUtils.js patch

Fixes the encodingFlags snafu and changes the title text to put the percentage
first.

Comment 86

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 70175 [details] [diff] [review]
Final(?) contentAReaUtils.js patch

r=bzbarsky

Comment 87

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 70173 [details] [diff] [review]
Final(?) uriloader/exthandler patch

r=bzbarsky

Comment 88

[Peter Trudelle]

nsbeta1+ per nav triage team

Comment 89

[Bill Law]

fix checked in

Comment 90

[Aleksey Nogin]

The nsWebBrowserPersist changes created a new compiler warning:

embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp:674
 `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...

Comment 91

[Aleksey Nogin]

Another warning caused by this check-in:

uriloader/exthandler/nsExternalHelperAppService.cpp:1159
 `PRBool readError' might be used uninitialized in this function

Comment 92

[Boris Zbarsky [:bzbarsky]]

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

Comment 93

[Scott MacGregor]

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.

Comment 94

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to fix warnings

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 95

[Håkan Waara]

Comment on attachment 70686 [details] [diff] [review]
Patch to fix warnings

r=hwaara

Comment 96

[Bill Law]

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. 

Comment 97

[Aleksey Nogin]

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?

Comment 98

[Boris Zbarsky [:bzbarsky]]

I'd open a separate bug.  Just assign it to me.

Comment 99

[sairuh (rarely reading bugmail)]

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.

RESULTS:

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

RESULTS:

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

RESULTS:

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

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

RESULTS:

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

Comment 100

[Bill Law]

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.

Comment 101

[Kathleen :Brade]

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.

Comment 102

[André Dahlqvist]

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.

Comment 103

[Kathleen :Brade]

that bug is a duplicate of a separate bug which was recently fixed

Comment 104

[sairuh (rarely reading bugmail)]

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 http://hopey.mcom.com/tests/bug27609-save-link.txt --will post as
an attachment soon, since it's on an internal server.

Comment 105

[sairuh (rarely reading bugmail)]

Attachment: test results using "save link target", 200205020x-1.0.0 bits