429827 - Download manager does not warn when its download location does not exist or is write protected

Status: NEW

(Toolkit :: Downloads API, defect, P3)

Description

[Kevin Berkheiser]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

The Saved Files To download preference has a problem.  If, you select a folder on a removable hard disk and for whatever reason some day you don't have that external disk connected, the download manager will give no warnings and will not prompt for a new location when downloading files.  This behavior is frustrating especially if you are downloading a very large file and wait for it, but then when the download manager closes, you realize the drive isn't connected and you have no idea where the file you waited on is on your computer.

The correct behavior would be either for download manager to prompt for a location, or provide an error, or a the very least tell you where the file is.

Reproducible: Always

Steps to Reproduce:
1. Configure download manager to save all files to an external drive.
2. Exit firefox.
3. Disconnect the drive.
4. Download a file
5. Notice it downloads without error.
6. Now try to find the file.
Actual Results:  
File is downloaded to some mysterious location and cannot be found.

Expected Results:  
Download manager prompts you for a valid download location.

Comment 1

[mcdavis941 (sporadically reading bugmail)]

Maybe specific to removable drives.  If (on WinXP) you try saving to a nonexistent directory on a writable drive, the directory is created.  If you try saving to a write-protected drive e.g. a DVD, it tells you it can't write and offers to save to a different location.

Comment 5

[Matthias Versen [:Matti]]

marking new based on the dupes, could not find older open bug

Comment 7

[Matthias Versen [:Matti]]

it seems that bug 270557 regressed

Comment 11

[Gobi]

It is happening in Windows xp also.

Comment 12

[Søren Bramer Schmidt]

When using a tool like http://mozbackup.jasnapaka.com/ to transfer FF profile with changed download location from one machine to another this is experienced too. To the normal user it looks like "firefox doesn't work" as it responds nothing to your download request - it just does nothing.

Comment 14

[Danniel Nascimento]

This bug is also reproducible if you import a profile where the download folder is set to a drive that doesn't exist in the new computer.

Comment 16

[Marlon Nelson]

This fix helps.  I don't know how to turn it into a proper patch, though.

--- tarpit/firefox/components/nsHelperAppDlg.js	2008-08-29 11:02:40.000000000 -0400
+++ Apps/firefox/components/nsHelperAppDlg.js	2008-09-13 15:53:06.000000000 -0400
@@ -168,7 +168,7 @@
       try {
         var lastDir = prefs.getComplexValue("browser.download.lastDir",
                             Components.interfaces.nsILocalFile);
-        if (lastDir.exists())
+        if (lastDir.exists() && lastDir.isDirectory() && lastDir.isWritable())
           picker.displayDirectory = lastDir;
         else
           picker.displayDirectory = dnldMgr.userDownloadsDirectory;
@@ -176,6 +176,9 @@
         picker.displayDirectory = dnldMgr.userDownloadsDirectory;
       }
 
+      if (!picker.displayDirectory.exists() || !picker.displayDirectory.isDirectory() || !picker.displayDirectory.isWritable())
+        picker.displayDirectory = dnldMgr.defaultDownloadsDirectory;
+
       if (picker.show() == nsIFilePicker.returnCancel) {
         // null result means user cancelled.
         return null;
@@ -220,7 +223,7 @@
      */
     validateLeafName: function (aLocalFile, aLeafName, aFileExt)
     {
-      if (!aLocalFile || !aLocalFile.exists())
+      if (!aLocalFile || !aLocalFile.exists() || !aLocalFile.isDirectory() || !aLocalFile.isWritable())
         return null;
 
       // Remove any leading periods, since we don't want to save hidden files

Comment 18

[Stephen Donner [:stephend] Not actively reading bugmail]

Shawn: do the proposed changes in https://bugzilla.mozilla.org/show_bug.cgi?id=429827#c16 look good to you?

Comment 19

[Shawn Wilsher :sdwilsh]

(In reply to comment #18)
> Shawn: do the proposed changes in
> https://bugzilla.mozilla.org/show_bug.cgi?id=429827#c16 look good to you?
Yes, and we should get an automated test for this too (this one is dirt simple to write luckily).

Comment 20

[Samuel Sidler (old account; do not CC)]

Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x. However, we'll look at a reviewed and baked patch.

This should probably block 1.9.1 though.

Comment 22

[Matthias Versen [:Matti]]

There is a possible patch in bug 462869:
https://bugzilla.mozilla.org/attachment.cgi?id=346056

Olivier Fourdan: Does this patch fixes all cases like non-existing directorys ?

Comment 24

[Olivier Fourdan]

Attachment: Capture of rhe error messages

(In reply to comment #22)
> There is a possible patch in bug 462869:
> https://bugzilla.mozilla.org/attachment.cgi?id=346056
> 
> Olivier Fourdan: Does this patch fixes all cases like non-existing directorys ?

I have only tested on Linux as it's the only platform I have access, but on Linux, entering a path including a non-existent directory displays a dialog and the download fails.

I am attaching a screen capture of the dialogs with the patch applied.

Comment 25

[Matthias Versen [:Matti]]

Oliver: The other case is that you set a specific directory in the Firefox preferences (always download to folder X, default on win32 is desktop) and if that directory doesn't exists anymore.

Comment 26

[Olivier Fourdan]

(In reply to comment #25)
> Oliver: The other case is that you set a specific directory in the Firefox
> preferences (always download to folder X, default on win32 is desktop) and if
> that directory doesn't exists anymore.

No, if you set "Save to Desktop" in the preference and then rename Desktop/ manually, then no dialog is shown.

But I am not sure this is the same issue, though, because this was FF1 and FF2 did not handle that case properly either.

My report and patch were instead to address a regression new in FF3 compared to the previous versions.

Comment 27

[Mike Beltzner [:beltzner, not reading bugmail]]

Doesn't block due to the non-standard configuration use-case, though I'm sure that Shawn would review a patch and a test!

Comment 28

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Make the userDownloadsDirectory function always return something that exists (or throw), and have a bit saner logic in nsHelperApps.js.in

I've kicked this off to the try server to make sure things look sane for folks on other platforms as well.

Comment 29

[Shawn Wilsher :sdwilsh]

Tryserver builds showing up here:
https://build.mozilla.org/tryserver-builds/2008-11-21_13:[e-mail]-try-5dbb7e15f5b/

Comment 30

[Ed Lee :Mardak]

Comment on attachment 349486 [details] [diff] [review]
v1.0

>+++ b/toolkit/components/downloads/src/nsDownloadManager.cpp
>+    // This could be the first time we are creating the downloads folder on the
>+    // desktop, so make sure it exists.
>+    PRBool exists;
>+    rv = downloadDir->Exists(&exists);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    if (!exists) {
>+      rv = downloadDir->Create(nsIFile::DIRECTORY_TYPE, 0755);
>+      NS_ENSURE_SUCCESS(rv, rv);
>+    }
Any particular reason why this is only needed for os x and windows? If it applies to other platforms as well, might as well push this shared code block to after the #ifdef blocks. (Doesn't seem too unreasonable to create the download dir or ~home/Download on linux or just ~home/Download on others.)

While you're sharing code to after the #ifdef blocks, might as well refactor this appending folder name.
PRBool appendFolder = PR_FALSE;

>@@ -1268,16 +1268,26 @@ nsDownloadManager::GetDefaultDownloadsDi
>   // Check to see if we have the desktop or the Safari downloads folder
>   PRBool equals;
>   rv = downloadDir->Equals(desktopDir, &equals);
>   NS_ENSURE_SUCCESS(rv, rv);
>   if (equals) {
>     rv = downloadDir->Append(folderName);
>     NS_ENSURE_SUCCESS(rv, rv);
>   }
downloadDir->Equals(desktopDir, &appendFolder);

>   NS_NAMED_LITERAL_STRING(osVersion, "version");
>   rv = infoService->GetPropertyAsInt32(osVersion, &version);
>   if (version < 6) { // XP/2K
>     rv = downloadDir->Append(folderName);
>     NS_ENSURE_SUCCESS(rv, rv);
appendFolder = version < 6;

>   rv = dirService->Get(NS_OS_HOME_DIR,
>                        NS_GET_IID(nsILocalFile),
>                        getter_AddRefs(downloadDir));
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = downloadDir->Append(folderName);
appendFolder = PR_TRUE;

> #endif
if (appendFolder) ...

>-  NS_ADDREF(*aResult = downloadDir);
>+  downloadDir.swap(*aResult);
Oh? New fancy stuff or nobody used these before?

>+++ b/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in
>@@ -198,29 +198,30 @@ nsUnknownContentTypeDialog.prototype = {
>+      // Default to lastDir if it's valid, then try to use the user's default
>+      // downloads directory.  This should always return a valid directory.
Comment is kinda strange.. *this* _should_ always return? The code looks more like default to user download dir unless there's a usable last directory. (When you mention "this", that's referring to this whole chunk of code or lastDir?)

>       var dnldMgr = Components.classes["@mozilla.org/download-manager;1"]
>                               .getService(Components.interfaces.nsIDownloadManager);
>+      picker.displayDirectory = dnldMgr.userDownloadsDirectory;
>+
>+      // The last directory preference may not exist, which will throw.
>       try {
>         var lastDir = prefs.getComplexValue("browser.download.lastDir",
>                             Components.interfaces.nsILocalFile);
>+        if (lastDir.exists() && lastDir.isDirectory() && lastDir.isWritable())
>           picker.displayDirectory = lastDir;

>@@ -269,17 +270,18 @@ nsUnknownContentTypeDialog.prototype = {
>+      if (!(aLocalFile && aLocalFile.exists() && aLocalFile.isDirectory() &&
>+            aLocalFile.isWritable()))
>         return null;
Would be good to make a shared function like.. "function isUsableDirectory(aFile) aFile && exists && isdir && writable"

Comment 31

[Shawn Wilsher :sdwilsh]

Attachment: v1.1 (checked-in)

(In reply to comment #30)
> Any particular reason why this is only needed for os x and windows? If it
> applies to other platforms as well, might as well push this shared code block
> to after the #ifdef blocks. (Doesn't seem too unreasonable to create the
> download dir or ~home/Download on linux or just ~home/Download on others.)
Yes, actually.  On Vista, OS 10.5, and linux, the downloads folder should exist (it's a system folder), and if it doesn't, we've got serious problems anyway.  However, on XP (we do a version check for it) and 10.4 with an older version of Safari, the folder doesn't exist - hence the checks.  I don't think we want to be creating system folder if they don't exist (they should, so something is wrong).

I'm not inclined to change our behavior on how we handle that this late in 3.1 (but I think this needs to get fixed for 3.1), so I'm going to decline to make that change now.

Also, when we drop 10.4 support, there is even less reason to share code.  This keeps the directory handling for each OS localized instead of jumping around too, which makes code clarity and readability better.

> >-  NS_ADDREF(*aResult = downloadDir);
> >+  downloadDir.swap(*aResult);
> Oh? New fancy stuff or nobody used these before?
It's not widely used, but it saves an AddRef and Release.  Not that this is performance critical code...


> >+++ b/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in
Addressed comments here.  This file is such a mess :(

Comment 32

[Ed Lee :Mardak]

Comment on attachment 350858 [details] [diff] [review]
v1.1 (checked-in)

(In reply to comment #31)
> (In reply to comment #30)
> > download dir or ~home/Download on linux or just ~home/Download on others.)
> I'm not inclined to change our behavior on how we handle that this late in 3.1
A new bug for creating ~home/Download then, though that doesn't sound like it has to be a system directory.

>-      // Default to lastDir if it's valid, use the user's default
>-      // downloads directory otherwise.
>+      // Default to lastDir if it is valid, then try to use the user's default
>+      // downloads directory.  userDownloadsDirectory should always return a
>+      // valid directory, so we can safely default to it.
>       var dnldMgr = Components.classes["@mozilla.org/download-manager;1"]
>                               .getService(Components.interfaces.nsIDownloadManager);
>+      picker.displayDirectory = dnldMgr.userDownloadsDirectory;
>+
>+      // The last directory preference may not exist, which will throw.
>       try {
>         var lastDir = prefs.getComplexValue("browser.download.lastDir",
>                             Components.interfaces.nsILocalFile);
>+        if (isUsableDirectory(lastDir))
>           picker.displayDirectory = lastDir;
The comment is still kinda odd. I think the original one was pretty good in the "if .. otherwise .." Right now the comment sounds more like "do this then do this".

Did you have the unit test somewhere? You said it should be simple -- just make a directory that isn't writable, yeah?

Comment 33

[Shawn Wilsher :sdwilsh]

(In reply to comment #32)
> Did you have the unit test somewhere? You said it should be simple -- just make
> a directory that isn't writable, yeah?
When I started to actually write one, it turned out to be really hard :(
I don't recall what it was at the moment, but it was one of those "this is gonna take me a couple days" kind of problems, hence no test.  I'll flag this as in-testsuite? so we can come along in the future and write a test (like we've done with other hard to test bugs).

Comment 35

[(mostly gone) XtC4UaLL [:xtc4uall]]

Shawn, nicely asking if you're going to land this one soon?

Comment 37

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/cab8b946ee86

Comment 38

[Shawn Wilsher :sdwilsh]

Comment on attachment 350858 [details] [diff] [review]
v1.1 (checked-in)

This will be really nice to have on branch and fixes a regression from 2.0 to 3.0.  Automated tests are going to be pretty hard here, but I'm working with QA right now to generate a litmus test.

Comment 40

[Stephen Donner [:stephend] Not actively reading bugmail]

Windows testcase: https://litmus.mozilla.org/show_test.cgi?id=7450
Mac/Linux testcase: https://litmus.mozilla.org/show_test.cgi?id=7446

Need everyone interested in this to look over those testcases, suggesting tweaks (additions, removals, clarifications).

Thanks!

Comment 41

[Marius Hudea]

Windows users will most likely be confused by the error messages shown in the litmus test attachment (they're Linux messages btw, even though I clicked on the Windows testcase - both litmus tests contain linux images).

I think the error message should say something like this:

"The destination folder is no longer available. 
You may have removed the drive or the selected network drive that is not available at this time.
Please choose another destination for your download."

or something like that... and show the save file as/select folder dialog.

Comment 42

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #41)

<snip>

Indeed, thanks for the feedback; I'll change that and upload a new screenshot and reference it when I get one that's representative of the change (so, tomorrow).  I'll also need to double-check Mac (both 10.4/10.5) and ensure that I've got those correct, too.

Comment 43

[Shawn Wilsher :sdwilsh]

Thanks Stephen - looks good.

Comment 44

[Stephen Donner [:stephend] Not actively reading bugmail]

Tried to verify using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090111 Shiretoko/3.1b3pre.

After the first failure, in which I get "Shiretoko cannot save the file; an unknown error occured", I receive the following two exceptions trying to download files by either left or right-clicking:

Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIHelperAppLauncher.saveToDisk]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///C:/Program%20Files/Shiretoko/components/nsHelperAppDlg.js :: anonymous :: line 483"  data: no]
Source File: file:///C:/Program%20Files/Shiretoko/components/nsHelperAppDlg.js
Line: 483

Error: uncaught exception: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIDownloadManager.userDownloadsDirectory]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: getTargetFile :: line 448"  data: no]

Comment 45

[Shawn Wilsher :sdwilsh]

Stephen - what steps did you do so I can reproduce that exactly and try to debug?

Comment 46

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #45)
> Stephen - what steps did you do so I can reproduce that exactly and try to
> debug?

1. In Windows (XP/Vista doesn't appear to matter), insert a removable drive (mine went to E:)
2. Tools | Options, "Save Files to...", set to E: (root directory)
3. Load http://www.ubuntu.com/getubuntu/downloading?release=desktop-newest&mirror=http%3A%2F%2Fmirror.its.uidaho.edu%2Fpub%2Fubuntu-releases%2F&arch=i386
4. Save the file to E:
5. Physically remove E: -- you'll get the error mentioned in comment 44 (although my wording wasn't correct)
6. Now, try to right-click on the "Download URL:" link you see inline on that page, and choose "Save Link As..."; you'll get those exceptions.

Comment 47

[pal-moz]

regression from this bug ?
see http://forums.mozillazine.org/viewtopic.php?p=5471445#p5471445

Comment 48

[Shawn Wilsher :sdwilsh]

(In reply to comment #47)
> regression from this bug ?
> see http://forums.mozillazine.org/viewtopic.php?p=5471445#p5471445
AFAIK, that was supposed to happen in Firefox 3 - the fact that it didn't concerns me...

Comment 49

[pal-moz]

(In reply to comment #48)
> (In reply to comment #47)
> > regression from this bug ?
> > see http://forums.mozillazine.org/viewtopic.php?p=5471445#p5471445
> AFAIK, that was supposed to happen in Firefox 3 - the fact that it didn't
> concerns me...


the folder is created after 0109 Nightly, maybe after this checkin.
it is not necessary. no need. I delete it immediately every time.

so the folder should not be created ?
why the folder is created ? What is it made for?

Comment 50

[Jim Mathies [:jimm]]

(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #47)
> > > regression from this bug ?
> > > see http://forums.mozillazine.org/viewtopic.php?p=5471445#p5471445
> > AFAIK, that was supposed to happen in Firefox 3 - the fact that it didn't
> > concerns me...
> 
> 
> the folder is created after 0109 Nightly, maybe after this checkin.
> it is not necessary. no need. I delete it immediately every time.
> 
> so the folder should not be created ?
> why the folder is created ? What is it made for?

The decision to create this goes back about a year and a half ago. Two camps involved, some didn't like downloads getting dropped on the desktop, some liked it. The Downloads folder camp won out due to security issues revolving around dropping downloads on the desktop.

https://bugzilla.mozilla.org/show_bug.cgi?id=308073#c51

Comment 51

[Mike Beltzner [:beltzner, not reading bugmail]]

Right, but it seems to be being created in the wrong place, is the thing

(btw, we use the "Downloads" folder in Vista and OSX, right? and create one in XP? separate issue, I'll find you on IRC to ask)

Comment 52

[Shawn Wilsher :sdwilsh]

(In reply to comment #51)
> Right, but it seems to be being created in the wrong place, is the thing
No, it looks like it's in the right place - on XP it was decided to place it on the desktop.

Comment 53

[Ria Klaassen (not reading all bugmail)]

If I create a new profile on Windows XP, the default download location (in the Options window) is Desktop, and indeed the download goes to the desktop, but it creates an additional Downloads folder on the desktop which remains empty. So this is clearly a bug.

Comment 54

[Jim Mathies [:jimm]]

(In reply to comment #53)
> If I create a new profile on Windows XP, the default download location (in the
> Options window) is Desktop, and indeed the download goes to the desktop, but it
> creates an additional Downloads folder on the desktop which remains empty. So
> this is clearly a bug.

Which version are you testing with Ria?

Comment 55

[Shawn Wilsher :sdwilsh]

I can't reproduce this issue at least on OS X with the latest from mozilla-central.  Wondering if folks could try the next hourly that comes out since we've had a few fixes land to this code yesterday and this morning.

Comment 56

[Stephen Donner [:stephend] Not actively reading bugmail]

Shawn: I'll take a look when I get time and have my removable drive.  In the meantime, could you quickly take a look at Windows?  Also, where does bug 326228 and its patch intersect with this, given the "or is write protected" portion of this bug's summary?

Comment 57

[Shawn Wilsher :sdwilsh]

I don't think bug 326228 intersects with this issue unless you can't write to your desktop (or now My Documents).

Comment 58

[WADA:World Anti-bad-Duping Agency]

I'm not sure Bug 443006 & Bug 454063 is DUP of this bug. Setting dependency of these bugs.

Comment 59

[WADA:World Anti-bad-Duping Agency]

Tested with scenario of Bug 443006 Comment #13, with Firefox latest trunk. 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090131 Minefield/3.2a1pre

> browser.download.useDownloadDir = false (always ask, lastDir is used)
> browser.download.lastDir = X:\...\NON-EXISTENT-DIR (emulation of deleted/renamed/removed/unmounted directory/drive)
> (case-a) browser.download.dir     = C:\...\READ-ONLY-DIR\NON-EXISTENT-DIR (emulation of internal directory creation falure)
> (case-b) browser.download.dir     = C:\...\READ-ONLY-DIR (file creation error)
> browser.download.folderList = 0/1/2

Original severe problem(no file picker dialog, silent failure) seems to have been resolved.
(case-a, Save Image As)
With any browser.download.folderList, file picker dialog was opened.
Final fall-back directory when browser.download.useDownloadDir=false seems to have been changed from Firefox 2, but silent failure(no file picker dialog issue) was not observed any more. Needless to say, exception of Comment #44 was not observed.
(case-b, Save Image As)
File picker dialog was opened, and when file name was specified, "insufficient permission" was detected, and Fx asked whether save to other location or not.

Note: Following is also observed.
When browser.download.folderList=2, ...\downloads was set in browser.download.dir after file picker dialog display. This automatic change of browser.download.dir was not observed with browser.download.folderList=0/1.
> https://developer.mozilla.org/en/Download_Manager_preferences

Comment 60

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090131 Minefield/3.2a1pre

After deleting the folder that used to be the default download location, Firefox recreates a new folder with the same name on the same spot. Instead it should just show the file picker. Showing the file picker is the clear sign that there is something wrong with the download location; most people will open the Options window and subsequently verify the folder's existence.

Comment 62

[Henrik Skupin [:whimboo][⌚️UTC+1]]

At least on OS X when the download folder isn't available, we have an empty textbox in the main panel of the preferences window. I can remember that Stephen already told about that on another bug.

Comment 64

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #60)
> Showing the file picker is the clear sign that there is something wrong with the download location;

"Save all attachments to this folder" case? (I tested false case only)
> browser.download.useDownloadDir = true (AFAIK, lastDir is never used)
What is your choice of browser.download.folderList?
> browser.download.folderList = 0/1/2
> (AFAIK, default=0. When user intentionally selects location via UI, set to 2)
What is displayed in selected location field of UI?
What is set in browser.download.dir?
> browser.download.dir
If a file path is already set in browser.download.dir, you did delete the directory just before your test? 

(In reply to comment #60)
> I can remember that Stephen already told about that on another bug.

AFAIK by reports to forums, many of Mac user cases have relation to followings.
(a) Download folder is not selected via UI by user.
    i.e. browser.download.folderList=0(or 1), and;
    (a-1) "Desktop"(or "Downloads") is displayed, and it looks as if location
          is already selected via UI. browser.download.dir is not set.
    (a-2) Or blank is displayed as selected location.
    Note: Problem of "unable to select" was reported to a forum in the past.  
(b) Value in browser.download.dir is "persistentDescriptor".
    > https://developer.mozilla.org/en/NsILocalFile/persistentDescriptor
    (b-1) Windows/Linux : Full path is used as persistentDescriptor
    (b-2) Mac OS X      : persistentDescriptor is not full path
                          (internally generated string)
(c) Environment depends on OS version.
    (c-1) Mac OS X Leopard : "downloads" folder exists
    (c-2) Mac OS X Tiger   : "downloads" folder doesn't exist
Combination of aboves sometimes produces funny problem on Mac OS X. It's probably "another bug" you say.

Comment 68

[Wim de Winter]

FYI:

Thought it would also occur with network connections that temporarily disappear. TEsting this failed, though, for it kept proposing to save to the original directory, in spite of restarting. MZ3.06

Comment 69

[Tyler_cmpt212_tsa45]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5)
Gecko/2008032620 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5)
Gecko/2008032620 Firefox/3.0b5

When I try to produce the error, another error seemed to happen:
 I followed the Steps to Reproduce:
1. Configure download manager to save all files to an external drive.
2. Exit firefox.
3. Disconnect the drive.
4. Download a file
5. Notice it downloads without error.
6. Now try to find the file.

But after step 3(disconnect the drive), the download dialog never comes out when I try to download some thing.  After I re set the download option to save all files in an existing directory,  the error disappears. 
Possibly my error has some relationship with this Bug 429827

Comment 70

[Shawn Wilsher :sdwilsh]

(In reply to comment #69)
> User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5)
> Gecko/2008032620 Firefox/3.0b5
> Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5)
> Gecko/2008032620 Firefox/3.0b5
Why oh why are you on 3.0b5?

Comment 71

[Christopher Aillon (sabbatical, not receiving bugmail)]

One of Red Hat's customers would really like to see this fixed in _3.0.x_.  I can see if they can wait until 3.5 however, it doesn't appear to have made it into even 1.9.1 yet.  Would it be possible to get this into 3.5 final or perhaps 3.5.1?

Also, as this is checked in to trunk, shouldn't it be marked fixed? http://hg.mozilla.org/mozilla-central/rev/cab8b946ee86

Comment 72

[Ed Lee :Mardak]

Shawn: Any particular reason why you removed a191? at 2009-01-22 11:12:10 PDT?

Comment 73

[Shawn Wilsher :sdwilsh]

(In reply to comment #71)
> Also, as this is checked in to trunk, shouldn't it be marked fixed?
> http://hg.mozilla.org/mozilla-central/rev/cab8b946ee86
No, because given comment 69, it doesn't appear to be fixed.

(In reply to comment #72)
> Shawn: Any particular reason why you removed a191? at 2009-01-22 11:12:10 PDT?
See comment 69.

If someone else wants to grab this and drive it, please do.

Comment 74

[:Gavin Sharp [email: [e-mail]]]

Did the fix that landed not have an observable positive effect? If it didn't, it probably shouldn't have landed, and we should back it out. If it did, then I think comment 69 should be split off into a newer bug, and it should probably also have tests, assuming its effects were functional and not just code cleanup.

Comment 76

[kevin]

when this bug will be fixed ? in which version ?
because i saw the first report of this bug was the   2008-04-19, one year ago..

Comment 77

[ISHIKAWA, Chiaki]

I am using a beta of TB 3.
Mozilla/5.0 (X11; U; Linux i686; ja; rv:1.9.1b3pre) Gecko/20090223 Thunderbird/3.0b2

I am seeing that the problem, similar to the reported one(s), is
still observed in this build of TB3 beta.

I am using linux box(es).

I create a directory under one user.
(Owned by that user and not group-writable.)

I run thunderbird under a different user in the group
that belongs to the group that owns this directory.
(But recall that the directory is not group-writable.)

In this case, TB fails to save the attachment silently as far as GUI
is concerned. However, later on, there are messages on the
console where TB was originally launched.

Below, I show the output  of strace (where the failure to write
occured) and the console message lines.


------------------------------- from strace outout ----
inotify_rm_watch(228, 38)               = 0
access("/extra/ishikawa/download/XXXYYY-DIR/Qqqqqqqqqqqq_20090428-CI-KT-mods.xls", F_OK) = -1 ENOENT (No such file or directory)
access("/extra/ishikawa/download/XXXYYY-DIR", F_OK) = 0
gettimeofday({1241166208, 896468}, NULL) = 0
access("/extra/ishikawa/download/XXXYYY-DIR/Qqqqqqqqqqqq_20090428-CI-KT-mods.xls", F_OK) = -1 ENOENT (No such file or directory)
open("/extra/ishikawa/download/XXXYYY-DIR/Qqqqqqqqqqqq_20090428-CI-KT-mods.xls", O_WRONLY|O_CREAT|O_EXCL|O_TRUNC|O_LARGEFILE, 0600) = -1 EACCES (Permission denied)

         <=== The above is where the saving failed.

write(2, "*** exception in validateLeafNam"..., 355) = 355
stat64("/extra/ishikawa/download/XXXYYY-DIR/Qqqqqqqqqqqq_20090428-CI-KT-mods.xls", 0xa548ba0c) = -1 ENOENT (No such file or directory)
lstat64("/extra/ishikawa/download/XXXYYY-DIR/Qqqqqqqqqqqq_20090428-CI-KT-mods.xls", 0xa548ba0c) = -1 ENOENT (No such file or directory)
write(36, "", 0)                        = 0
close(36)                               = 0
futex(0xb693e9c8, FUTEX_CMP_REQUEUE, 1, 2147483647, 0xb693ceb4, 0) = 1
futex(0xa5ace820, FUTEX_WAKE, 1)        = 1


--- from the console where thunderbird was originally launched.


*** exception in validateLeafName: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.create]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: file:///u2/ishikawa/THUNDERBIRD-DIR/thunderbird-v3-beta2/thunderbird/components/nsHelperAppDlg.js :: anonymous :: line 283"  data: no]
*** exception in validateLeafName: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.create]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: file:///u2/ishikawa/THUNDERBIRD-DIR/thunderbird-v3-beta2/thunderbird/components/nsHelperAppDlg.js :: anonymous :: line 283"  data: no]
*** exception in validateLeafName: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.create]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: file:///u2/ishikawa/THUNDERBIRD-DIR/thunderbird-v3-beta2/thunderbird/components/nsHelperAppDlg.js :: anonymous :: line 283"  data: no]




As some of the reporter(s) here, I would like this to be fixed.
I had to scratch my head many times because every time I thought I saved the
file, I didn't see it in the directory, and had to repeat this many times,
until I notice the permission problem.

TIA.

Comment 84

[Ginn Chen]

I did a quick look on Solaris with mozilla-central.
save page as, or save image as will popup Downloads dialog, and  "could not be saved, becasue you canno change the contents of that folder" dialog.
save link as will not.

In nsHelperAppDlg.js makeFileUnique(), the exception is caught and throw.
I think the download will not be started.

Also nsHelperAppDlg.js uses aDirectory.isWritable() to check if the directory is writable.
Probably it's not good.
IsWritable is checking S_IWUSR | S_IWGRP | S_IWOTH, it doesn't ensure the current user can write to this directory.
Even if the checking is failed, e.g. /proc directory. validateLeafName() did nothing to alert user.

The only positive effect is, for the next time, "Save to ..." dialog will go back to ~/Downloads if the last saved directory is /proc.

BTW: What's the expected behavior? Go back to "Save to..." dialog or get "could not be saved, becasue you canno change the contents of that folder" dialog?
If it is the latter, the following change might help.

303     validateLeafName: function (aLocalFile, aLeafName, aFileExt)
304     {
305       if (!(aLocalFile && isUsableDirectory(aLocalFile)))
Change to if(!aLocalFile)
306         return null;

373       catch (e) {
374         dump("*** exception in validateLeafName: " + e + "\n");
375 
376         if (e.result == Components.results.NS_ERROR_FILE_ACCESS_DENIED)
377           throw e;
Change to return;

Comment 85

[ISHIKAWA, Chiaki]

TB3b4 still suffers from the similar problem when the directory chosen
is write-protected for the particular user.

TB3b4 accepts the path, which is unwritable for the user, and silently drops the copy. (No saving is actually done.) 

Silently as far as GUI goes, that is.
On the console where the binary was invoked, I see the following:


*** exception in validateLeafName: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.create]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: file:///u2/ishikawa/THUNDERBIRD-DIR/thunderbird-v3-beta4/thunderbird/components/nsHelperAppDlg.js :: anonymous :: line 292"  data: no]
*** exception in validateLeafName: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.create]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: file:///u2/ishikawa/THUNDERBIRD-DIR/thunderbird-v3-beta4/thunderbird/components/nsHelperAppDlg.js :: anonymous :: line 292"  data: no]
ishikawa@dell-w2k-note$ 

I hope the above message helps in pinpointing where the error occurs, and
which steps should be taken.

The end result that TB3 seems to be suceeding the saving the attachment, and
then the user can't find the file anywhere is very confusing.

At least, TB3 should report  the file could not be created (because the
directory was inaccessible or something. "unwritable" in this particular case would be nicer.)

TIA

Comment 86

[ISHIKAWA, Chiaki]

Today, TBv3b4 announced security/stability update and so I upgraded and
voila I have TB3. Great work.

Unfortunately, this bug still persists in TB3 release.
If a directory chosen for saving an attachment is write protected,
then TB3 silently performs the unsuccessful saving (as far as GUI goes),
and then the user has to scratch her/his head until he/she recalls this bug :-(

Just as in the previous TBv3b4, TB3 prints error message on the console
it was run.
(Ignore the -beta4 suffix for the directory of the executable. Obviously
auto-upgrade chose the existing directory for the upgrade.
I have verified that this is indeed TB3 (final user release) by clicking help
about TB.)

*** exception in validateLeafName: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.create]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: file:///u2/ishikawa/THUNDERBIRD-DIR/thunderbird-v3-beta4/thunderbird/components/nsHelperAppDlg.js :: anonymous :: line 292"  data: no]

I am mounting NFS in a somewhat contorted manner to work with
multiple projects and tend to be hit by this bug often.

Obviously, there are others who got bitten by this bug in the last
half a year or so.

If no one is going to take this old bug, I may be tempted to go in and
pest the developers :-)
until this is fixed.  

Comments will be appreciated.

Comment 88

[ISHIKAWA, Chiaki]

(In reply to comment #87)
> *** Bug 532866 has been marked as a duplicate of this bug. ***

After reading through 523866 bug report, I now realize
that  there seem to be a few causes of this
download failure (and not reporting the failure to users) and
this confuses the developers in producing "correct" fix.

The failure cause:

(a) - The download target directory doesn't exist.

(b) - The download target directory exists, but is write protected.

In both cases, I think we, the users, prefer to be told of the fact so that
we can take appropriate action when TB faces these problems when it is
asked to download a file.

In case (a), I don't think many users prefer the mailer to store the intended file under "default" directory WITHOUT TELLING US because 
 - not many users are  aware of what "default" directory is,
 - and such user mistake (of trying to use non-existent directory) should
   be warned LOUD and CLEAR.


In case (b), the current behavior of silently failing download and behave as if
it succeeded is totally unacceptable.

Any thoughts on this from other users/observers?

Comment 89

[ISHIKAWA, Chiaki]

(In reply to comment #88)
> (In reply to comment #87)
> > *** Bug 532866 has been marked as a duplicate of this bug. ***
> 
> After reading through 523866 bug report, I now realize
> that  there seem to be a few causes of this
> download failure (and not reporting the failure to users) and
> this confuses the developers in producing "correct" fix.
> 
> The failure cause:
> 
> (a) - The download target directory doesn't exist.
> 
> (b) - The download target directory exists, but is write protected.
> 
> In both cases, I think we, the users, prefer to be told of the fact so that
> we can take appropriate action when TB faces these problems when it is
> asked to download a file.
> 
> In case (a), I don't think many users prefer the mailer to store the intended
> file under "default" directory WITHOUT TELLING US because 
>  - not many users are  aware of what "default" directory is,
>  - and such user mistake (of trying to use non-existent directory) should
>    be warned LOUD and CLEAR.
> 
> 
> In case (b), the current behavior of silently failing download and behave as if
> it succeeded is totally unacceptable.
> 
> Any thoughts on this from other users/observers?

At least under linux, with the TB 3.0.1, case (a) seems to be handled.
I was warned that the directory doens't exist.

(This situation is hard to create since under Linux, in my case using 
Gnome, file chooser seems to pick up the existing directory before
proposing to save the attachment into a directory.
Before hitting "OK", I renamed the target directory from a different console after logging into the computer, thus the target directory no longer exists when I hit [OK] (to save) button.)

Case (b) still seems to be not handled in TB 3.0.1.

Again, I manually removed the write permission of the targetr directory before
trying to save an attachment to the suggested directory.
Again 3.0.1 behaved as if it succeeded, but the attachment was not
copied there. 
The console where I invoked TB 3.0.1 showed the similar error message, and so
this is a GUI issue where the error is not properly passed back to the user.

*** exception in validateLeafName: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.create]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: file:///u2/ishikawa/THUNDERBIRD-DIR/thunderbird-v3-xxxx/thunderbird/components/nsHelperAppDlg.js :: anonymous :: line 292"  data: no]

There was a suggested fix in comment #84.

Can any developer comment on the validity of that suggested fix?

Comment 90

[ISHIKAWA, Chiaki]

I am uploading a patch based on comment #84
to modify 
mozxilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in

I hope the developers of TB3 can evaluate it.

I checked that both the missing directory and
the unwritable directory are handled properly, i.e., the proper
error dialog is displayed to explain the error to the user.

TIA

Comment 91

[ISHIKAWA, Chiaki]

Attachment: Patch to properly warn the user when the download directory is write protected.

The patch to properly warn the user when the download target directory is write
protected and download can't succeed.

Based on comment #84.

Please advise who is the module owner so that I can fill in the requestee field.

Comment 92

[ISHIKAWA, Chiaki]

Comment on attachment 422754 [details] [diff] [review]
Patch to properly warn the user when the download directory is write protected.

(not certain) but put sdwilsh address in review requestee field. If someone likes to do so, he/she can assign this bug to me.

Comment 93

[Ginn Chen]

I think for long term solution, 
we need to fix a) in comment #88, we should popup a dialog to let user select a directory.
For b) in comment #88, we should also give a file-chooser dialog, rather than a warning dialog.

And we should fix IsWritable(), S_IWUSR | S_IWGRP | S_IWOTH doesn't make sense.

Comment 94

[Shawn Wilsher :sdwilsh]

(In reply to comment #84)
> Also nsHelperAppDlg.js uses aDirectory.isWritable() to check if the directory
> is writable.
> Probably it's not good.
> IsWritable is checking S_IWUSR | S_IWGRP | S_IWOTH, it doesn't ensure the
> current user can write to this directory.
We should fix this.  Is a bug filed?

> BTW: What's the expected behavior? Go back to "Save to..." dialog or get "could
> not be saved, becasue you canno change the contents of that folder" dialog?
> If it is the latter, the following change might help.
The latter, so we should do your proposed change.

We also should make a test for this since this code path is clearly complicated.

Comment 95

[Shawn Wilsher :sdwilsh]

Comment on attachment 422754 [details] [diff] [review]
Patch to properly warn the user when the download directory is write protected.

per comment 94

Comment 96

[ISHIKAWA, Chiaki]

(In reply to comment #95)
> (From update of attachment 422754 [details] [diff] [review])
> per comment 94

Thank you, Shawn, for looking at this problem.

I have an observation and a few reasons for NOT fixing isWritable() here.

I felt fixing isWritable() was not worth people's man-power.

Let me explain.

The patch in attachment 4227534 (comment 91) based on the idea in
comment 84 fixes the proper dialog problem. That is, the patch makes
TB produce an error dialog that essentially says "could not save the
file because it could not change the contents of the directory".

So far, so good.

But my patch didn't address the deficiency of isWritable() mentioned
in comment 84 and simply did away with such checking before the real
writing takes place.  

Why? There are a few reasons.

When I looked the comment in comment 84 and began wondering what to
do, I gave up after a while. I decided that simply ignoring the
a priori test before writing is better.

The list of reasons:

(1) Such a check in advance of real opening and writing to a file
    has a race condition and not bullet-proof.

    Essentially, between the time the check of isWritable() is done and
    the real writing is done, someone can change the permission or
    even DELETE the directory(!). [In a sense, my testing by producing 
    non-existing target directory and non-writable target-directory is a 
    twist on this theme. In my case, TB stopped in the middle of file chooser
    dialog and so the timing window I took advantage is not related to
    the race window I mention here, exactly speaking, but you get the idea).

    For this particular downloading problem, I felt fixing
    isWritable() was not worth the effort because of this ESSENTIAL
    deficiency to and the difficulties mentioned below (2), and (3) in
    particular.


(2) On UNIXen, the use of effective and real {user,group} ID compounds
    the checking of the read/write permissions.

    Some readers may say, why not access()? Why not, indeed.
    access() only checks the access of "REAL" {user,group} ID and not
    the effective ID used for permission checking.

    So access() is not useful when effective ID is in use. (*IF* in use may be 
    more appropriate). 

    Even if we come up with a better check, the race-condition mentioned in (1)
    still renders the result of such prior check useless.
   ( glibc access(2) man page under linux specifically mentions this
    race problem! )

   For that matter, access() may not behave as specified by POSIX for
   superuser under linux according to libc manual under my linux
   PC. This is bad.
   (There was a mention of access() behaving differently under sunos and linux
   in TB code also. C++ definition is #ifdef'ed due to this reason, I suspect.)

   Now some readers might say, why don't we open the file in the first
   place and obtain an file descriptor to avoid such race condition,
   but then you already have either opened it successfully
   or failed to open it by then for writing  purposes and 
   so the additional a priori isWritable() check is frivolous/redundant. You   
   already know the permission by then(!). [My patch essentially avoids such   
   pre-checking and notifies the failure of write operation 
   at that time using GUI.] 

   (Re effective ID issue) euidaccess() seems to exist in modern
   glibc, but I am not sure if these are available on other UNIXen. It
   seems to be called as eaccess() in other systems. euidaccess() is
   not a posix API, sadly.  Also, access(), and euidaccess() probably
   don't work over network file server reliably (see (3) below).

    And of course, euidaccess() like access() has race-conditions.

(3) Use of remote file servers essentially makes the local efforts
    such as access() and euidaccess() to figure out the access based
    on IDs and such useless.

     Some readers may want to go further by checking the 
     effective {user, group} ID and do some detailed checking. 
     But this is useless because of the
     race-condition for our purposes and the reason below.

     If all file access is local, then maybe a detailed check can be
     possible.  But when a file is stored on a remote file server and
     its access is managed on that server and NOT on a computer where
     TB runs. Anything can happen.

     This problem is also mentioned in libc access(2) man page under linux.

 
My Conclusion: After trying to come up with a reasonable fix to
isWritable for the particular purpose of checking if the saving to a
directory has failed, I gave up.
isWritable() is full of pitfalls and can not be fixed in a bullet-proof manner. 

So because of the race condition, an essential deficiency, and the
unreliability of isWritable() with remote file servers, 
to fix the problem of non-reporting of non-existing and non-writable directory during saving an attachment, I simply decided to this a priori check
 (commenting out isUsable check) as was suggested in comment 84.

The bottom line here is we can only tell reliably whether a file is
writable under a certain directory when we open it for write and write
the data.

This is why I decided to remove isUsable() check.


My take is that All TB can depend on the success or failure of writing
in the end.  *THAT* is what we should pick up and tell the user with
appropriate GUI, which is now done by the proposed patch.

This fix is good enough for its purpose IMHO.


Now, *IF* some other paths in TB depends on this isWritable()
function [I don't know. I don't see similar problems so far in
bugzilla when I began writing the comment to this particular bug], my
suggestion is to highlighting isWritable() issue by putting an
appropriate comment of the above deficiencies to isWritable() function
definition and that the programmer ought to be prepared for the
eventual failure of supposedly accessible file operation due to the
problems mentioned above.

PS: Additionally, I am not sure if the current TB code handles the
case of failure during writing in mid-way: for example, suppose that
the partition becomes full after TB writes half the attachment. But
this is another topic.  This failure scenario can't be caught even if
we use the file descriptor for initial writing access check, etc. I
realized this corner-case when I thought of using file descriptor to
check for write permission.  We have to perform error code checking
for *EVERY* I/O operation anyway. But someone ought to test this and
if TB fails, file a bug here. One bug fix at a time.

PPS: Yes, anyone is welcome to overhaul the code to use file
descriptor for checking the file write permission for downloading sanity checking, but IMHO, since we are going to open it for write anyway, this is redundant and not worth developer's time.

Comment 97

[Shawn Wilsher :sdwilsh]

(In reply to comment #96)
> The patch in attachment 4227534 (comment 91) based on the idea in
> comment 84 fixes the proper dialog problem. That is, the patch makes
> TB produce an error dialog that essentially says "could not save the
> file because it could not change the contents of the directory".
Sure, there are several ways we could accomplish this.  However, as submodule owner I'm saying that the current proposed patch is not a suitable solution.

> (1) Such a check in advance of real opening and writing to a file
>     has a race condition and not bullet-proof.
...but is unlikely to happen in 99.9% of the cases.  And even if it does happen, we can deal with it (by a failed download, throw up a dialog, or something else).

> (2) On UNIXen, the use of effective and real {user,group} ID compounds
>     the checking of the read/write permissions.
[snip]
>    For that matter, access() may not behave as specified by POSIX for
>    superuser under linux according to libc manual under my linux
>    PC. This is bad.
I can't see people using Firefox/Thunderbird as a superuser frequently.

> (3) Use of remote file servers essentially makes the local efforts
>     such as access() and euidaccess() to figure out the access based
>     on IDs and such useless.
This doesn't mean we shouldn't try to offer a better experience to users who aren't using remote file servers.


> isWritable() is full of pitfalls and can not be fixed in a bullet-proof manner. 
But we don't need bullet-proof, we need "better than now".

> The bottom line here is we can only tell reliably whether a file is
> writable under a certain directory when we open it for write and write
> the data.
But that is done once the download is completed.  We want to be able to alert the user, when possible, before then.

> My take is that All TB can depend on the success or failure of writing
> in the end.  *THAT* is what we should pick up and tell the user with
> appropriate GUI, which is now done by the proposed patch.
Yes, but we should save the user some bandwidth by also checking before the end.

> Now, *IF* some other paths in TB depends on this isWritable()
> function.
Lots of code uses isWritable:
http://mxr.mozilla.org/mozilla-central/ident?i=isWritable

> PS: Additionally, I am not sure if the current TB code handles the
> case of failure during writing in mid-way
It does.

Comment 98

[ISHIKAWA, Chiaki]

(In reply to comment #97)
  ...
> > isWritable() is full of pitfalls and can not be fixed in a bullet-proof manner. 
> But we don't need bullet-proof, we need "better than now".
> 

Agreed. Working code even if the internal is messy is preferable than
non working code.

So what should we do?

For a SHORT term solution:

Should we leave the advance check back in, and then
file a SEPARATE bug for isWritable() for possible later fixing?

To me, this approach is most convenient (assuming that this
"leaving the advance check back in" produces the desired behavior, as in comment 90,  for now.). 
I will investigate if the fix this manner satisfies the
test I performed in comment  90  (under linux).
This behavior, once verified, should satisfy  the need mentioned
in the latter half of comment 94.

What do you think?

For a LONG term solution, I agree with comment 94, especially about
showing chooser dialog again if the download fails due to non-existent
or write-protected directory.  But I need a SHORT TERM fix in the
official release now. And I am not familiar with the internal of TB
to produce such re-popping of chooser again with the suitable
string to explain that "we need to choose another file/directory because
the download failed due to {missing, write-protected} directory or file, etc.

The following can wait for a while: basically it is a fix to isWritable().

> > Now, *IF* some other paths in TB depends on this isWritable()
> > function.
> Lots of code uses isWritable:
> http://mxr.mozilla.org/mozilla-central/ident?i=isWritable

Wow. I am surprised. 

I notice one asymmetry. I find the definitions in three places
(counting the ifdef'ed dupe for unix as one),
in 
 xpcom/io/nsLocalFileWin.cpp 
 xpcom/io/nsLocalFileUnix.cpp 
 xpcom/io/nsLocalFileOS2.cpp 

But the function is used only in nsLocalFileWin.cpp and
nsLocalFileWin.cpp and not in nsLocalFileUnix.cpp?

 xpcom/io/nsLocalFileWin.cpp (View Hg log or Hg annotations)
          o line 1647 -- target->IsWritable(&isWritable); 
  xpcom/io/nsLocalFileOS2.cpp (View Hg log or Hg annotations)
          o line 1554 -- target->IsWritable(&isWritable); 

Strange, isn't it? 
I will investigate if no one else is going to 
do something about isWritable()

For this, will someone file a bug for isWritable() under {linux, solaris}
if it is not there?
(I found a mention of isWritable in
 Bug 437216 -  Add support of NTFS permissions in nsLocalFile::IsWritable  
and the comment there in April of 2009 is not encouraging.)

My take on this is that except for BeOS, we should merge the SOLARIS code
with the rest of OSes to use either stat, access [or eaccess or whatever], etc.  I re-checked the bugid mentioned in the afore-mentioned comment related to Solaris-specific code,  but the bug is not quite
related to the use of access() and friends. (bug 351595 only discusses
creating a file in Desktop.)  I think the comment is no
longer useful today or the number mentioned is incorrect.

cf.
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileUnix.cpp#1313
   // access() problem also exists in Solaris POSIX implementation
   // see bug 351595, https://bugzilla.mozilla.org/show_bug.cgi?id=351595


I initially thought of suggesting BeOS support be dropped, but to my
surprise I find out there is an active community who resurrects BeOS and its API.
So I decided not to.
cf. http://www.haiku-os.org/news/2009-09-13_haiku_project_announces_availability_haiku_r1alpha_1


For UNIXens, we should probably simply use euidaccess() and then if it
is not available access() and done with it.  (UNIXens: linux, solaris,
and who else is using TB under their version of POSIX/UNIX by compiling the
code?)

Remember "better than now" approach? I think we should stick to this.

However, weshould not hold our breath. I am afraid other users of
isWritable() may insist on a somewhat more rigorous implementation when the bug is announced :-(

But again, this can wait until the bug for isWritable is filed and
someone takes up the cleanup efforts. It is orthogonal to the bug in this
bug entry.

As far as the SHORT term handling of download problem mentioned
in this thread is concerned, the short term fix should be easy to
produce IMHO.

> 
> > PS: Additionally, I am not sure if the current TB code handles the
> > case of failure during writing in mid-way
> It does.

Good!

Comment 99

[Ginn Chen]

(In reply to comment #94)
> (In reply to comment #84)
> > Also nsHelperAppDlg.js uses aDirectory.isWritable() to check if the directory
> > is writable.
> > Probably it's not good.
> > IsWritable is checking S_IWUSR | S_IWGRP | S_IWOTH, it doesn't ensure the
> > current user can write to this directory.
> We should fix this.  Is a bug filed?

I checked again and it is only doing it for Solaris and BeOS.
I filed Bug 542738 for Solaris case.

I think for this bug, we can assume IsWritable() work as expected, also we should not assume the directory is always writable if IsWritable() is true, permission can be changed at any time.

I don't think the IsWritable() issue would block this bug any way.

Comment 100

[ISHIKAWA, Chiaki]

In comment 98, I said:
>I will investigate if the fix this manner 
 (with the pre-test back in)
>satisfies the
>test I performed in comment  90  (under linux).
>This behavior, once verified, should satisfy  the need mentioned
>in the latter half of comment 94.

I tested with/without the pre-check of isUsableDirectory.
Unfortunately, pre-check produces the bad result: failure to notify the
error when the target directory is write-protected.

My conclusion. pre-check is bad in the current form.
My proposed patch is "better than now".

Read on.

case I: with the pre-check in:
    if (!(aLocalFile && isUsableDirectory(aLocalFile) ))

(II-1) the case of non-existing directory. : handled ok

    Synopsis:
    under linux, after the file chooser dialog is shown and
    set for a directory (/tmp/dir-a), I manually
    remove /tmp/dir-a, and then hit [save] button.
    TB said basically, the directory content could not be read, and
    returned to file chooser with the "save-directory" chosen
    at the parent directory. So I can continue to save the file
    in "/tmp". Good.

cf. After cancelling to save further, the console showed this line.
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIHelperAppLauncher.saveToDisk]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///home/ishikawa/TB-3HG/objdir-tb3/mozilla/dist/bin/components/nsHelperAppDlg.js :: anonymous :: line 499"  data: no]
************************************************************

line 499 of mozilla/dist/bin/components/nsHelperAppDlg.js :
  This is within this block:

  479	
  480	    notify: function (aTimer) {
  481	      if (aTimer == this._showTimer) {
  482	        if (!this.mDialog) {
  483	          this.reallyShow();
  484	        } else {
  485	          // The user may have already canceled the dialog.
  486	          try {
  487	            if (!this._blurred) {
  488	              this.mDialog.document.documentElement.getButton("accept").disabled = false;
  489	            }
  490	          } catch (ex) {}
  491	          this._delayExpired = true;
  492	        }
  493	        // The timer won't release us, so we have to release it.
  494	        this._showTimer = null;
  495	      }
  496	      else if (aTimer == this._saveToDiskTimer) {
  497	        // Since saveToDisk may open a file picker and therefore block this routine,
  498	        // we should only call it once the dialog is closed.
* 499	        this.mLauncher.saveToDisk(null, false);
  500	        this._saveToDiskTimer = null;
  501	      }
  502	    },
  503	
  504	    postShowCallback: function () {
  505	      this.mDialog.sizeToContent();
  506	
  507	      // Set initial focus
  508	      this.dialogElement("mode").focus();
  509	    },


  I think the message for initial error of trying to save to
  non-existing error was held until the timing of "cancel".


(II-2) the case of non-writable directory. : NO, VERY BAD!

    Very bad. The original bad behavior of pretending that
    saving was successful when it failed actually...

    under linux, after the file chooser dialog is shown and
    set for a directory (/tmp/dir-b), I manually
    remove write permission for /tmp/dir-b,
    chmod a-w /tmp/dir-b, and then hit [save] button.
    TB acts as if it saved the file, but actually it failed.

    The console where tb was invoked  show the following.

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIHelperAppLauncher.saveToDisk]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///home/ishikawa/TB-3HG/objdir-tb3/mozilla/dist/bin/components/nsHelperAppDlg.js :: anonymous :: line 499"  data: no]
************************************************************

	Obviously, this error is not properly picked up and shown to
	user with proper GUI dialog!
	(People don't look at the original console usually. I don't.)

----------------------------------------

case II: with the pre-check removed:

       // see bug 429827, comment # 84
       // and additional.
       if (!(aLocalFile /* && isUsableDirectory(aLocalFile)*/ ))

(II-1) the case of non-existing directory. : ok

    The behavior is the same as in II-1.

   Again, the console where TB was invoked showed the following lines.

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIHelperAppLauncher.saveToDisk]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///home/ishikawa/TB-3HG/objdir-tb3/mozilla/dist/bin/components/nsHelperAppDlg.js :: anonymous :: line 499"  data: no]
************************************************************


(II-2) the case of non-writable directory. :  OK, Good!

    under linux, after the file chooser dialog is shown and
    set for a directory (/tmp/dir-d), I manually
    remove write permission for /tmp/dir-d,
    chmod a-w /tmp/dir-d, and then hit [save] button.
    TB shows the following dialog to clearly tell the user that
    it failed to save the file.

    /tmp/dir-d/libc.so could not be saved, because you cannot change
    the contents of that folder.

    Change the folder properties and try again, or try saving in a
    different location.		 [OK]

    If I hit [OK], then the dialog disappears. (chooser is not
    shown any more.)


    The console where TB was invoked showed this.

*** exception in validateLeafName: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.create]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: file:///home/ishikawa/TB-3HG/objdir-tb3/mozilla/dist/bin/components/nsHelperAppDlg.js :: anonymous :: line 313"  data: no]

---

Short summary

========================================
with Pre-check...            Warned by GUI    Further saving offered
----------------------------------------
non-exsisting directory      Yes.             Yes
non-writable  directory      NO, VERY BAD!    n/a
========================================
Without Pre-check            Warned by GUI    Further saving offered.
----------------------------------------
non-exsisting directory      Yes.             Yes
non-writable  directory      YES, GOOD!       no. (long-term goal).
========================================

Although that no chooser is shown for download failure case in II-2 (a
long-term goal), the behavior in case II is much more favorable since
a user is NOT misled to believe that saving has succeeded when, in
fact, it didn't.

So my tentative conclusion here is that the patch proposed
is "better than now".

Any thought?

TIA

PS: I am afraid that the observation in comment 88 was correct:

The attempted fixes were too much focused on the case of
"Non-existing directory", and not on the "write-protected".

>that  there seem to be a few causes of this
>download failure (and not reporting the failure to users) and
>this confuses the developers in producing "correct" fix.
>
>The failure cause:
>
>(a) - The download target directory doesn't exist.
>
>(b) - The download target directory exists, but is write protected.

Comment 101

[ISHIKAWA, Chiaki]

>(II-1) the case of non-existing directory. : ok
>
>    The behavior is the same as in II-1.

     I meant to say I-1 here.

Comment 102

[ISHIKAWA, Chiaki]

In comment 100:

I forgot to mention clearly that  the test/experiment was done by 
either enabling and disabling
the isUsableDirectory(), but using
"return e"instead "throw e" as noted
in comment 84:
>
>376         if (e.result == Components.results.NS_ERROR_FILE_ACCESS_DENIED)
>377           throw e;
>Change to return;

that is why, in comment 100, we didn't see the 
as in comment 86 and comment 89.

Maybe someone familiar with the code can figure out
why the chooser is not shown in the case of
non-writable directory case, considering the
function call stack, and the difference of returning and throwing an error.
I noticed a couple of calls to makeFileUnique(...) don't check the
return value. Maybe this is the culprit?
With the "return e" above, makeFileUnique() returns the error instead of
throwing it.

TIA

Comment 103

[ISHIKAWA, Chiaki]

Dear Shawn and others,

I have been looking into this problem.

I figured out why there is no extra file chooser dialog when the 
permission problem prohibits saving the attachment from TB3, and
there IS when the previously selected directory disappeared (and causes the failure of saving attachment).

The flow of control is like this:
When a saving of attachment is tried:

  If a save directory is previously set.
    then 
       try saving to it.
       if OK then return
    endif

  if not, i.e.   (save directly is not set OR 
     the saving to a previously set directory fails.)  
  then
      FALLTHROUGH here.
  endif
 
  choose a directory by file picker.
     try saving to it.
     if OK, fine, 
     but if failed, show a dialog and 
     NO ATTEMPT attempt to choose another directory is ATTEMPTED AT ALL
     in Javascript.
  
So naturally, we want to add a code to try picking another directory when the
saving attachment failed due to permission problem.

But I have been unable to fix the code in a 
satisfactory manner yet because

Q1:  - the existing code has problems of failing to return meaningful value
       in failure cases (which came to light during my rewrite.),
Q2:  - I am not sure what values to be returned (is there a design doc somewhere?
   Or should I just go with the flow, so to speak, by improvisation? )

Q3: To top it off, I have no idea how to debug this javascript interaction with
TB3 GUI internally.

I noticed there is an error console in the "Tools" pull down menu.
But that seems to show only the errors noticed by Javascript interpreter and there doesn't seem to be a way to print some variable values and stuff that aids
debugging.

How do people handle such javascript debugging?

Q4: OK, if the re-attempt to show file picker dialog is not made inside
    javascript, there may be a chance that whatever invokes this JS code
    may try to do it, but I doubt if it does.
    Is there a document that describes the interaction between
    javascript codelet and main c++ binary? I am not entirely sure how
    the control is passed between them.


I wonder if people who have done programming for TB2 and TB3 can shed light on my
question/comment above. Especially the debugging of Javascript.
Once it is handled more easily, I believe I can fix the problem pretty quickly now that I know the basic program flow. Right now, I am not sure if the control
is properly caught by try{} catch{} construct even.

TIA

PS: I tried mozilla newsgroup(s), but unfortunately, there is no
mozilla.dev.tech.javascript newsgroup any more (?).
If suitable newsgroups exist, I would like to know about them.

Comment 104

[Shawn Wilsher :sdwilsh]

(In reply to comment #103)
> Q1:  - the existing code has problems of failing to return meaningful value
>        in failure cases (which came to light during my rewrite.),
This doesn't surprise me.  This code has been hacked up and around for years.

> Q2:  - I am not sure what values to be returned (is there a design doc
> somewhere?
>    Or should I just go with the flow, so to speak, by improvisation? )
Go with the flow - the only "design docs" are the comments in the code.  We've been adding more comments as we [re]write bits, so that adds more documentation.

> Q3: To top it off, I have no idea how to debug this javascript interaction with
> TB3 GUI internally.
You can use the dump function, which will dump out to the console.  You can also use Components.utils.reportError to dump something to the Error Console.  You'll also want to set these preferences (automatically set in debug builds AFAIK):
https://developer.mozilla.org/en/Setting_up_extension_development_environment#Development_preferences

> Q4: OK, if the re-attempt to show file picker dialog is not made inside
>     javascript, there may be a chance that whatever invokes this JS code
>     may try to do it, but I doubt if it does.
>     Is there a document that describes the interaction between
>     javascript codelet and main c++ binary? I am not entirely sure how
>     the control is passed between them.
The download manager code in question that would pick up on the error is here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManager.cpp#2426

It's not clear to me how we could throw up a file picker at that point, however (at least with re-using code).

> PS: I tried mozilla newsgroup(s), but unfortunately, there is no
> mozilla.dev.tech.javascript newsgroup any more (?).
> If suitable newsgroups exist, I would like to know about them.
You want mozilla.dev.platform likely

Comment 105

[ISHIKAWA, Chiaki]

Dear Shawn,

Thank you for the helpful tips. They are very valuable. 

Debugging javascript in a mail client is something I didn't expect doing when I 
started investigating. 

I will look into the documents and try debugging.

Re: Q4.
>     the control is passed between them.
>The download manager code in question that would pick up on the error is here:
>http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads>/src/nsDownloadManager.cpp#2426
>
>It's not clear to me how we could throw up a file picker at that point, however
>(at least with re-using code).

The C++ code is very spartan. We need to take care of the details in javascript code, I think.

Thank you again.

Comment 106

[ISHIKAWA, Chiaki]

dump() doesn't seem to work always in xpcom components.
See my posting to mozilla.dev.platform newsgroup and
responses. But I finally got crude debug output using
the replacement function mentioned in the follow-up posts there.

So I should be able to come up with a fix hopefully soon!

Comment 107

[ISHIKAWA, Chiaki]

Hmm, a big problem.

It seems the program path taken in the two cases below are completely 
different and I have been only checking the first path so far.

In TB3's saving the attachment case.
(Under Linux)

(1) Double-Clicking on the attachment and cause a pop up menu with
"Opening xxyyz" header, and "what thunderbird should do with this file? Open with Application,  Save file" panel. I choose the save button to run the
save path.

(2) Right-click on the attachment and get the popup "Open/Save As/Detach/Delete" and then choose "Save As" and see the Save Attachment file chooser.

Although I was more interested in the behavior of case (2) above,
actually the file I was looking at handles the path for case (1) above!

I am not sure now where case (2) is handled (in which JS file, etc.).

Hmm... At least I get the error message regularly in (1) now. 
Maybe dump() also works, but I was trying to trigger the run through the path of case (1) above by trying the scenario for case (2) [right clicking], thus didn't see the dump() output.

I think the unfortunate fact is:

TB (a mailer)'s save attachment and browser's save feature is handled
by the same routine, it seems. Thus the terminology is somewhat confusing.

The above case of having a predefined directory (case (1) above) [the last path component presumably chosen automatically], and
the user choosing the filename under which the file is saved (case (2))
is very different but is discussed in this single bug entry.

I will see what I can do to get out of this mess, but can someone enlighten me where the code that handles the case (2) above then?

I am totally confused now.

Comment 108

[ISHIKAWA, Chiaki]

OK, I *THINK* the execution path is in the following file when the saving of attachment of a mail message is attempted from the menu item "Save As" from the popup menu
shown by RIGHT clicking on the attachment.  

src/mailnews/base/src/nsMessenger.cpp

Correct me if I am wrong. 

It seems a little odd that this is a C++ source file while the other execution
path has the JavaScript code. Am I missing something?

Comment 109

[ISHIKAWA, Chiaki]

OK, I *THINK* the execution path is in the following file when the saving of attachment of a mail message is attempted from the menu item "Save As" from the popup menu
shown by RIGHT clicking on the attachment.  

src/mailnews/base/src/nsMessenger.cpp

Correct me if I am wrong. 

It seems a little odd that this is a C++ source file while the other execution
path has the JavaScript code. Am I missing something?

Comment 110

[ISHIKAWA, Chiaki]

Question: Does anyone know for sure that nsMessanger.cpp is the other file to fix?

Progress so far:
Thanks to the use of error console, I am moving forward for nsHelperAppDlg.js
modification. My heavily modified nsHelperAppDlg.js seems to work just fine.
(Except for a case where I don't know yet how to handle dialog to show
[OK][Cancel] to allow the user to select the future operation path, and
in one case, the dialog says the user shall choose another filename to retry, but the chooser was not shown after hitting [OK] button, and saving attempt ends there. I am still reusing the
old dialog panels with my code shuffle and so this should be expected. I will fix these soon. I need to introduce a user action choice result as the return value aggregate or something to notify the upper functions of the user's selected action.)

But, the second execution path for saving an attachment from a message
in mail client, TB3, by right-clicking on the attachment and then chose "Save As" menu entry is not handled yet because it seems to be
handled by another file, nsMessanger.cpp.
But I am not confident if nsMessanger.cpp *IS THE* file to fix.

Can it be there is a wrapper JavaScript that goes with it and that javascript is the one which I should be fixing for the GUI defficiency?

Does anyone know for sure?

cf. 
JavaScript debugging is very difficult. I mistyped a function name which
was throwing "Reference Error" or whatever inside the Javascript interpreter.
To my surprise, this interpreter error is handled as any other runtime error, and this error is thrown for the user javascript program to catch.

But unfortunately, some functions in nsHelperAppDlg.js were trapping and hiding ALL such errors by try { ... } catch (e) {} construct. (NOTE that there is no action in catch clause!) VERY, VERY BAD.
So until I inserted dumping caught errors from left and right like someone did in

https://bug462869.bugzilla.mozilla.org/attachment.cgi?id=346056
(note the dumping of error value in dump at the end)

I didn't realize my typo. After fixing this typo, things have gotten to the point that all I need to fix is the dialog panels and intended actions to match each other now.
I really wonder how others handle javascript debugging. 
I should install one of the javascript debugger inside thunderbird, I think.

PS:
I really feel that the downloading a file done by browser, and 
saving attachment from a message (already in local folder!) to
a local file by a mail client should be handled as a different operation.
The surrounding and expected errors are so different IMHO.

But obviously, TB3 re-uses the code originally written for
browser at least for the case of saving the attachment when
the user (left-) clicks on the attachment.

I now know why I see strange small blank window for an instant (and it disappears quickly)  when I save
the attachment of a message in TB.
See Bug 460930
	
It has no particular use for MAIL Client context. But it is shown there because
TB re-uses the code for the browser downloading (progressive bar) operation, I think.

It may be that someone might want to file a separate bug/improvement request on this matter. The design decision must have been made years ago, and so I don't know if the change at this stage of development will be accepted or not. But at least, that no one before me seems to have realized that 
the problems mentioned in this bug entry are actually related to TWO different action paths handled by two different sets of files suggest that there is a serious confusion. (On re-reading, I now see comment 44 - comment 46 talks of
URL, http:// and so I assume it is FF browser downloading.)

Maybe I should freshly file a bug for "TB Saving Attachment fails without warning  when the download directory does not exist or write-protected".

I have been fixing a wrong problem almost :-)
Now I need to fix the real problem I face most often (silent saving failure after right-clicking and choosing "Save As" when the target directory is write protected in THUNDERBIRD mail client.)

Comment 111

[ISHIKAWA, Chiaki]

(In reply to comment #103)
> Dear Shawn and others,
> 
> I have been looking into this problem.
> 
> I figured out why there is no extra file chooser dialog when the 
> permission problem prohibits saving the attachment from TB3, and
> there IS when the previously selected directory disappeared (and causes the
> failure of saving attachment).
> 
> The flow of control is like this:
> When a saving of attachment is tried:
> 

I modified the flow like the following so that attempts to
offer new saving locations are made.

>   If a save directory is previously set.
>     then 
>        try saving to it.
>        if OK then return
>     endif
> 
>   if not, i.e.   (save directly is not set OR 
>      the saving to a previously set directory fails.)  
>   then
>       FALLTHROUGH here.
>   endif
> 

The above remains the same.

>   choose a directory by file picker.
>      try saving to it.
>      if OK, fine, 
>      but if failed, show a dialog and 
>      NO ATTEMPT attempt to choose another directory is ATTEMPTED AT ALL
>      in Javascript.
> 

The above is changed.

     do
        choose a directory by file picker and
        try saving to it.
        Handle error such as write-protected directory, etc..
     until 
        the user cancels,
        or
        the saving succeeds. 
        
The do part is actually turned into a file-scope function (since
I don't think this auxiliary function needs to be exported via XPCOM,
I simply moved the processing into an independent function. Also,
initially, I had thought this function may be reusable for the case
right-clicking-then-save-as execution path [which doesn't seem to
be the case.]


> Q1:  - the existing code has problems of failing to return meaningful value
>        in failure cases (which came to light during my rewrite.),


> Q2:  - I am not sure what values to be returned (is there a design doc
> somewhere?
>    Or should I just go with the flow, so to speak, by improvisation? )
> 


As is done, returning null to mean that the user cancels the operation is very
tricky and not clear.
Also there are codes in the file that does try {...} catch (e) {}
to suppress ALL thrown errors. Please note there is no action in
catch clause. VERY BAD CODING PRACTICE.
All my typos during debugging threw errors such as undefined function or symbol
which were caught by such empty catch clause and were suppressed until I began
dumping error values right and left.
 
In a short while, I am uploading the current status for nsHelperAppDbg.js with
debug output in the form of dump to ask others to evaluate the
fix approach.

Comment 112

[ISHIKAWA, Chiaki]

Attachment: Requesting for comment for the fix approach in nsHelperAppDlg.js

This is a still work-in-progress: with mydump() sprinkled here and there.
This is to solicit comment to the approach taken now to fix the problem.
So please view and post/send comments.

There is one change I made to validateLeafName. The first argument
seems to be directory/folder and so I changed the name for easier
understanding.


Also, note that this was created to the somewhat older mozilla-central 
which was switched a couple of weeks ago.
(If I say, under mozilla directory, hg diff -U 8 -bw, the diff shows more
XPCOM related changes. So I did a local "hg cat" under mozapps/download directory, 
to obtain the pristine original and created a diff against my copy"

The file discussed here is
src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js

TIA

Comment 113

[ISHIKAWA, Chiaki]

I have filed another bugzilla entry

Bug 549719 : Should UNIFY/MERGE the two different code paths for saving an attachment in a message

The fix for nsHelperAppDlg.js is almost ready, but
the different path for saving the attachment is not fixed at all yet.

Comment 114

[ISHIKAWA, Chiaki]

Now I think I know where to change the code to offer re-selection of files
in the second path:

http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMessenger.cpp#901

899   SetLastSaveDirectory(localFile);
900 
901   rv = SaveAttachment(localFile, aURL, aMessageUri, aContentType, nsnull, nsnull);
902   return rv;
903 }
904 

We simply fail and return on 901 and 902. 

If we make this into a loop until
success or it would be OK (we can repeat on failure( in an ideal world. 

Unfortunately, the current
code doesn't seem to distinguish between the genuine failure after which we may
want to retry, and the explicit user cancellation after which we don't want to retry and return immediately. (Both case seem to return nsnull.)

In the case of nsHelperAppDlg.js,
 - user cancellation is thrown as JS error,
 - failure is returned as a function value
and so we can distinguish the two different cases and
decide either to repeat or return.

What a mess, indeed...

TIA

Comment 115

[ISHIKAWA, Chiaki]

Now that it has been more than a couple of weeks since I posted the
basic idea to fix nsHelperAppDlg.js, I will clean it up
and post a patch for review hopefully before next week.

As for fixing, nsMessenger.cpp, it is a tough nut to crack.
I tried to propagate the user-cancellation information from the low-level
function to the upper-level function so that this information can be used
for intelligent GUI handling. Then I found that I needed to
change some publicly visible class function prototypes as well, 
I tread carefully, but then compile errors forced me to realize at least one such publicly visible function prototype comes from idl files, that is, it is 
an XPCOM interface function. So changing this (possibly plural) may not be 
easy policy-wise although, as far as I checked using http://mxr.mozilla.org/, this function may not be referenced outside the file(!) at all.

Considering the overhead of chaning IDL defintion,
I am afraid I will leave nsMessenger.cpp as is for now, but
concentrate in submitting a cleaned patch for nsHelperAppDlg.js.

Comment 116

[ISHIKAWA, Chiaki]

Attachment: cleaned up patch for nsHelperAppDlg.js

This is an RFC (request for comment) patch before the final review request.
Basically, the code has all the functionality for the final patch.

The issues are as follows:

I left a check in isUsableDirector() intentionally using #if DEBUG, #endif.
I wonder if such debug statements within #if DEBUG, #endif can't be left 
in the final code. 

Also, by mistake, my emacs editor buffer modifies the white space indentation
and so these have to be fixed before the final patch.

I also mark some empty catch () clauses which may interfere with debugging
by catching legitimate errors which should be notified the programmer.

TIA

But I would like the people concerned to take a look at this.

Comment 117

[ISHIKAWA, Chiaki]

BTW, I tried to fix nsMessenger.cpp, but eventually gave up for now since the
low-level routine(s) didn't seem to pass the error status (such as
failure to write to a file due to the directory being write-protect, etc.) 
back to the caller in a clearly defined manner at all.

Probably the original authors didn't bother to incorporate the user-friendly 
warning or retry based on such low-level error since the error information is
missing :-(

That file is hard to fix easily.

Comment 118

[ISHIKAWA, Chiaki]

(In reply to comment #116)
> Created an attachment (id=435887) [details]
> cleaned up patch for nsHelperAppDlg.js 
> 
   [...]
> Also, by mistake, my emacs editor buffer modifies the white space indentation
> and so these have to be fixed before the final patch.

>  [...]

Folks, I took a hard look at the indentation issues in this file (nsHelperAppDlg.js).
 - The mode line near the beginning is incorrect and doesn't agree with
   mozilla's javascript coding style.

 - The file uses four-spaces indentation in some places, and
   two-spaces indentation (which is Mozilla's suggested JavaScript 
   coding style) in other places.

   It is in bad shape in terms of coding style.

Would the owner(?) or whoever looks at this file for review/superreview mind
if I submit a series of patches, i.e.,
 - firstly, purely re-indentation patch to make nsHelperAppDlg.js
   conform to javascript coding style as much as possible, and THEN
 - secondly, submit the final patch which is a tidied up version of the
   patch already submitted?

If there is no strong objection from the wider community, I will try this sequence and request review/superreview to get the indentation fix, and the final fix  done in a week or so.

To be frank, emacs plus a clever javascript mode is not useful at all
when the source file is a hodgepodge of different coding styles. (especially
indentation space differences.)

Without the suggested cleanup of indentation issues first, I can't fix the file
without creating suprious diff lines in the proposd patch, which would make
the evaluation very difficult.


TIA.

Comment 119

[Shawn Wilsher :sdwilsh]

(In reply to comment #118)
>  - firstly, purely re-indentation patch to make nsHelperAppDlg.js
>    conform to javascript coding style as much as possible, and THEN
>  - secondly, submit the final patch which is a tidied up version of the
>    patch already submitted?
This is fine, but I'd wait ~24 hours.  We just noticed that the Style Guide was inconsistent with itself, and that there was a separate document for JS only that wasn't fully accurate.  I'm in the process of fixing both of these now.

Comment 120

[ISHIKAWA, Chiaki]

(In reply to comment #119)

> This is fine, but I'd wait ~24 hours.  We just noticed that the Style Guide was
> inconsistent with itself, and that there was a separate document for JS only
> that wasn't fully accurate.  I'm in the process of fixing both of these now.

I will wait for a few days. (I am not in a rush unlike some others who got bitten by this bug). 

Do drop me a line when the fixing of the style guide document
is done.

(This is a round-about way of fixing the bug, but will be effective for maintenance in the long run.)

Comment 121

[Shawn Wilsher :sdwilsh]

(In reply to comment #120)
> Do drop me a line when the fixing of the style guide document
> is done.
Everything should be merged and updated here:
https://developer.mozilla.org/En/Developer_Guide/Coding_Style

Comment 122

[ISHIKAWA, Chiaki]

I am going to upload the following TWO files for review.

(1) Re-indented nsHelperAppDlg.js-re-indented

src/mozilla/toolkit/mozapps/download/nsHelperAppDlg.js

(2) The patch against this re-indented version of nsHelperAppDlg.js

The file (1) differs from the original only in the number of whitespaces, except as follows:

I removed the offending mode-line which is in clear violation of
javascript coding style: the style calls for two-space indentation while the mode line sets it at four spaces.

In one place, a block of old code is ifdef'ed out by #if 0, #endif, but
there is a lone opening brace which interferes proper indentation efforts afterward (javascript mode is not aware of #if* construct). So I commented the
offending commented code with "//".

To wit, the only two differences are here:
diff -U 8 -b src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js /tmp/nsHelperAppDlg.js-re-indented 
--- src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js	2010-04-05 00:23:58.000000000 +0900
+++ /tmp/nsHelperAppDlg.js-re-indented	2010-04-05 00:50:23.000000000 +0900
@@ -1,10 +1,9 @@
 /*
-# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
 # ***** BEGIN LICENSE BLOCK *****
 # Version: MPL 1.1/GPL 2.0/LGPL 2.1
 #
 # The contents of this file are subject to the Mozilla Public License Version
 # 1.1 (the "License"); you may not use this file except in compliance with
 # the License. You may obtain a copy of the License at
 # http://www.mozilla.org/MPL/
 #
@@ -518,21 +517,21 @@
         // if a file sent as text/plain contains binary characters, and if so (*)
         // it morphs the content-type into application/octet-stream so that
         // the file can be properly handled. Since this is not generic binary
         // data, rather, a data format that the system probably knows about, 
         // we don't want to use the content-type provided by this dialog's 
         // opener, as that's the generic application/octet-stream that the 
         // uriloader has passed, rather we want to ask the MIME Service.
         // This is so we don't needlessly disable the "autohandle" checkbox.
-        var mimeService = Components.classes["@mozilla.org/mime;1"].getService(Components.interfaces.nsIMIMEService);
-        var type = mimeService.getTypeFromURI(this.mLauncher.source);
-        this.realMIMEInfo = mimeService.getFromTypeAndExtension(type, "");
+      // var mimeService = Components.classes["@mozilla.org/mime;1"].getService(Components.interfaces.nsIMIMEService);
+      // var type = mimeService.getTypeFromURI(this.mLauncher.source);
+      // this.realMIMEInfo = mimeService.getFromTypeAndExtension(type, "");
 
-        if (type == "application/octet-stream") {
+      // if (type == "application/octet-stream") {
 #endif
         if (shouldntRememberChoice) {
           rememberChoice.checked = false;
           rememberChoice.disabled = true;
         }
         else {
           rememberChoice.checked = !this.mLauncher.MIMEInfo.alwaysAskBeforeHandling;
         }
ishikawa@debian-vm:~/TB-3HG$ 

The proposed final patch is against this re-indented version.

TIA

Comment 123

[ISHIKAWA, Chiaki]

Attachment: Re-indented version of src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js

Re-indented version of nsHelperAppDlg.js so that it is more in line with
the javascript coding style.

My final patch touches two sections of code: it seemed that one section used
two-space indentation (which was OK), but the other used four-space indentation
which was not quite OK.

Also, the source code had this emacs-mode line which was not quite correct: it specified four-space indentation which was not in line with javascript coding style.

I tried to preserve the original as much as possible.

!diff
diff -U 8 -b src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js /tmp/nsHelperAppDlg.js-re-indented 
--- src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js	2010-04-05 00:23:58.000000000 +0900
+++ /tmp/nsHelperAppDlg.js-re-indented	2010-04-05 00:50:23.000000000 +0900
@@ -1,10 +1,9 @@
 /*
-# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
 # ***** BEGIN LICENSE BLOCK *****
 # Version: MPL 1.1/GPL 2.0/LGPL 2.1
 #
 # The contents of this file are subject to the Mozilla Public License Version
 # 1.1 (the "License"); you may not use this file except in compliance with
 # the License. You may obtain a copy of the License at
 # http://www.mozilla.org/MPL/
 #
@@ -518,21 +517,21 @@
         // if a file sent as text/plain contains binary characters, and if so (*)
         // it morphs the content-type into application/octet-stream so that
         // the file can be properly handled. Since this is not generic binary
         // data, rather, a data format that the system probably knows about, 
         // we don't want to use the content-type provided by this dialog's 
         // opener, as that's the generic application/octet-stream that the 
         // uriloader has passed, rather we want to ask the MIME Service.
         // This is so we don't needlessly disable the "autohandle" checkbox.
-        var mimeService = Components.classes["@mozilla.org/mime;1"].getService(Components.interfaces.nsIMIMEService);
-        var type = mimeService.getTypeFromURI(this.mLauncher.source);
-        this.realMIMEInfo = mimeService.getFromTypeAndExtension(type, "");
+      // var mimeService = Components.classes["@mozilla.org/mime;1"].getService(Components.interfaces.nsIMIMEService);
+      // var type = mimeService.getTypeFromURI(this.mLauncher.source);
+      // this.realMIMEInfo = mimeService.getFromTypeAndExtension(type, "");
 
-        if (type == "application/octet-stream") {
+      // if (type == "application/octet-stream") {
 #endif
         if (shouldntRememberChoice) {
           rememberChoice.checked = false;
           rememberChoice.disabled = true;
         }
         else {
           rememberChoice.checked = !this.mLauncher.MIMEInfo.alwaysAskBeforeHandling;
         }
ishikawa@debian-vm:~/TB-3HG$

Comment 124

[ISHIKAWA, Chiaki]

Attachment: Fixing the problem in nsHelperAppDlg.js (patch against re-indented version.)

Fixed the mentioned problem(s) in nsHelperAppDlg.js code.

The patch has been posted in the previous indentation for a while.
Basically, it does
- use my own version of dump as suggested in a mozilla newsgroup for easier
  debugging.
- move the repeated action (when the user retries specifying the filename) after
  a failure to save to a separate function,
- Change the name of one parameter to validateLeafName from aLocalFile to 
  aLocalFolder since it seems to fit the role of the argument better.
- Fixed validateLeafName to throw an error explicitly at early stage.
- Fixed makeFileUnique to throw an explicit error even for generic error.
- Marked a few empty catch(expr) {} clauses as dangerous since they
  interfere javascript debugging very badly.

With this patch, on double-clicking using left-button, TB3 correctly
handles the case of write-protected error, etc.
Furthermore, it now repeats asking for another filename if the saving fails
until the user cancels the save operation(!).

(The case of double-clicking using right mouse and having nsMessenger.cpp
involved is still left as is due to the problems mentioned earlier.)

TIA

Comment 125

[ISHIKAWA, Chiaki]

OK, I got confused a little bit.
Should I modify

mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in

in the end?

Comment 126

[Shawn Wilsher :sdwilsh]

Comment on attachment 436927 [details]
Re-indented version of src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js

Please upload patches, not actual files.

Comment 127

[Shawn Wilsher :sdwilsh]

Comment on attachment 436928 [details]
Fixing the problem in nsHelperAppDlg.js (patch against re-indented version.)

Also, per https://developer.mozilla.org/en/Creating_a_patch, please upload an hg generated patch here.

Comment 128

[Shawn Wilsher :sdwilsh]

(In reply to comment #125)
> Should I modify
> mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in
> in the end?
Yes

Comment 129

[ISHIKAWA, Chiaki]

Attachment: (patch) Re-indented version of src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js

Re-indented version of nsHelperAppDlg.js.
Re-submitted in patch form.
This may look a substantial change, but as I noted earlier, this
only removes the bogus mode line and
hides a long opening brace within #if 0, #endif
and tries to conform to the JavaScript coding style especially
for two-space indentation issues.

I will upload the patch for the real fix AFTER this patch is in the
source repository so that I can generate a patch using "hg diff -U 8".

Comment 130

[ISHIKAWA, Chiaki]

Hmm.
Between the last time I tried to create the patch and today,
obviously the nsHelperAppDlg.js disappeared in the source repository and
instead, probably now it is generated at built time from nsHelperAppDlg.js.in
template. Back to square one. I will investigate a little further to see
WHICH nsHelperAppDlg.js.in file should be modified.

Comment 131

[Shawn Wilsher :sdwilsh]

(In reply to comment #129)
> I will upload the patch for the real fix AFTER this patch is in the
> source repository so that I can generate a patch using "hg diff -U 8".
You can use mq (mercurial extension) to make patches on top of other patches.  Or, you could just commit it to your local repository.

Comment 132

[:Paolo Amadini]

Comment on attachment 437712 [details]
(patch) Re-indented version of src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js

Thanks for the code formatting clean-up! This is useful, but while doing it
we must also try to reduce to the minimum the chance of disrupting other
people's work on the same file.

We're about to check-in a patch on the same file in a week or two, so we'd
better wait for it before changing the indentation. If meanwhile you'd like
to go on working on this bug, I suggest you generate a patch against the
old indentation (remember to tick the "patch" checkbox on the attachment).

Also, please file a separate bug for the indentation change; this way we
can properly track the dependencies with this and other bugs.

Shawn, if you are aware of other people currently working on this code,
please tell us so that we can notify them that we're going to change
the indentation, and set proper dependencies on the new bug.

Comment 133

[ISHIKAWA, Chiaki]

Hi, thank you for the update.

I will then, hmm..., try to re-create the patch again against the old indentation as cleanly as possible. We will figure out where an improper indentation in the original cause a few indentation changes in my patch (using proper mozilla indentation style.).

Then after the dust settles, we will clean up the whole file's indentation in one full sweep hopefully.
(In any case, it seems to me that we need to go back to the original(?) .js.in
in the long term if I am not mistaken. )

TIA

Comment 134

[:Paolo Amadini]

(In reply to comment #133)
> Then after the dust settles, we will clean up the whole file's indentation in
> one full sweep hopefully.

Thanks, that's even better. If you can file a separate bug now, we can set the
correct dependencies and you can start working on it when it is the right time.

Meanwhile I looked at your previous work in progress / request for comments,
and I noticed that it works on parts of the code I'm also working on as part
of another cleanup: take a look at attachment 439487 [details] [diff] [review] on bug 506987.
The idea there is to have less code, simpler, and shared.

I don't have time to look into the details of this bug report now, but if
you want, I'm available to discuss the issues related to this bug in the
next weeks. You could start by applying my patch on a clean trunk tree,
see how it performs in relation to the original problem, and then see
how it interacts with your changes.

Comment 135

[ISHIKAWA, Chiaki]

(In reply to comment #134)

OK, I filed a separate bug for the indentation problem:

Bug 559833  - Should fix incorrect indentation of src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js

Comment 136

[ISHIKAWA, Chiaki]

(In reply to comment #131)
> (In reply to comment #129)
> > I will upload the patch for the real fix AFTER this patch is in the
> > source repository so that I can generate a patch using "hg diff -U 8".
> You can use mq (mercurial extension) to make patches on top of other patches. 
> Or, you could just commit it to your local repository.

I understand that Paolo Amadini is producing a patch that changes function
makeuniq and friends(?) that are shared by nsHelperAppDlg.js and another file which he is working on.

Upon cursory reading of my patch (on top of the 
re-indented source file) which I am uploading in a few minutes,
the overlap is not so much that our patches seem to co-exist 
peacefully without stepping on each other's toes. 

I am uploading the patch created after I commit the re-indented version so that
people who need to look at the proposed change can understand the direction
my patch is following.

TIA

Comment 137

[ISHIKAWA, Chiaki]

Attachment: pending patch created against re-indented version.

An early-bird view for evaluation.

Comment 138

[ISHIKAWA, Chiaki]

Attachment: pending patch created against re-indented version.

(Oops) Uploaded an incorrect file. Here is the (correct) patch created against re-indented nsHelperAppDlg.js
An early-bird view of the proposed changes so that people can evaluate
the change I am proposing before some coordination with another bug fix can
take place.

Comment 139

[ISHIKAWA, Chiaki]

Hi,

I have posted the clean-up patch for indentation problem for nsHelperAppDlg.js
in bug 506987 against the source code that includes Paolo's patch.

I will post the patch to fix the problem discussed here, once the code
for 506987 is, or in a few days against the proposed re-indented file, whichever comes first.

TIA

Comment 140

[:Paolo Amadini]

Chiaki, I tried out your code (latest patch, where I had to apply some
changes manually), but it's not clear to me what is the problem that it
is designed to solve. It doesn't seem to be related to either comment 0
or comment 69, or your observations in comment 88, case (a).

I joined the discussion on this bug only recently, and so it's difficult
to find the relevant information by reading more than 100 comments. If
this patch is not about the case described in "steps to reproduce", then
it's definitely better to open a new bug report and put the detailed
steps to reproduce in its description.

Note that in the end the steps to reproduce should be automatable in some
way, and you'll have to write tests, so it's better to write the steps in
plain English while keeping this in mind.

Once you make clear which behavior the patch is changing and provide
developers with instructions on how to check out the change themselves,
you will be able to receive more focused feedback.

Comment 141

[ISHIKAWA, Chiaki]

(In reply to comment #140)
> Chiaki, I tried out your code (latest patch, where I had to apply some
> changes manually), but it's not clear to me what is the problem that it
> is designed to solve. It doesn't seem to be related to either comment 0
> or comment 69, or your observations in comment 88, case (a).
> 

Thank you for taking the time to try out the patch.

The correct answer is "comment 88, case (b)".
(b) - The download target directory exists, but is write protected.


In this case, TB silently fails to store the file and
no error is reported via GUI. Which is troublesome.

With my patch, at least, the permission error is reported, and TB
now goes back to offer the selection of new directory.

This is what my patch tries to achieve.

Yes, you are right, maybe I should re-file a bug since the cause and
error behavior seem to be very different for the case of missing drive (under window) and write-protected directories.

(I will take a look at the comment about clean up patch in another bug entry.
Too bad, emacs's idea of indentation doesn't exactly match the coding style, or maybe I can tweak some customization variables.)

Comment 142

[:Paolo Amadini]

(In reply to comment #141)
> Yes, you are right, maybe I should re-file a bug since the cause and
> error behavior seem to be very different for the case of missing drive (under
> window) and write-protected directories.

Definitely, thanks. I'll try to reproduce the issue when the new bug with
the patch and steps to reproduce is ready.

Comment 143

[ISHIKAWA, Chiaki]

(In reply to comment #142)
> (In reply to comment #141)
> > Yes, you are right, maybe I should re-file a bug since the cause and
> > error behavior seem to be very different for the case of missing drive (under
> > window) and write-protected directories.
> 
> Definitely, thanks. I'll try to reproduce the issue when the new bug with
> the patch and steps to reproduce is ready.

Now I have filed
Bug 567585  - TB3 fails to raise an error when it tries to save an attachment to write-protected directory. 

in order to discuss the limited case.
Hopefully, I will post a patch (against the cleanup version of nsHelperAppDlg.js
tomorrow.)

Comment 144

[ISHIKAWA, Chiaki]

The code path invoked by Right-clicking and then "Save as" has
improved in 3.2a1pre. I tested this under linux.

Write-protected directory case:

If I try to save an attachment to a write-protected directory using Right-click and "Save As", TB3 3.2a1pre (the current comm-central code) 
warns the user TWICE in succession that
"Unable to save the attachment. Please check your file name and try again"

The message could be improved to state the permission problem explicitly,
and getting this TWICE for a single save attempt
is a little nuisance, but this is a great improvement over current behavior of
silent failure.

Non-existing directory case:
(I create /tmp/c-dir, and then delete it while it is chosen in file picker.)

If I try to save a directory and while the directory is chosen and before
hitting the save button, I remove the directory from a different console, then TB3 3.2a1pre complains that
Error accessing ///tmp/c-dir/savefilename  non-existing file.

So aside from two remaining problems,  

(1) - the use of different default save path in the two paths (left-clicking case, and right-clicking case and then "save as"), 
   [Left-click case uses mistakingly the BROWSER default in nsHelperAppDlg.js ]   
   (Bug 549719
     https://bugzilla.mozilla.org/show_bug.cgi?id=549719#c17
   )

(2)- and the misuse of umask,
      https://bugzilla.mozilla.org/show_bug.cgi?id=549719#c14
      and bug 533976
      https://bugzilla.mozilla.org/show_bug.cgi?id=533976#c5
      also holds true for me and I am really interested in seeing this umask
      problem nailed down and fixed.

I think we are not far from seeing
the fixing of bugs for real (under POSIX-like systems IMHO) finally under POSIX-like systems.

Comment 145

[ISHIKAWA, Chiaki]

Now a big question:

Something is fishy here. Is the related code very different in
mozilla-central presumably used by TB 3.0.5, and comm-central that I
have been using to build my test builds for debugging?

I have a feeling that maybe the broken ancient code in the original
mozilla-central is carried now by comm-central while the problematic
code has been fixed (?) in mozilla-central and is now reflected back to the
3.0.5.  3.2a1pre problem for left-clicking seems to be real from
what I checked. The bug persisted as it did for a long time.  BUT this
problem somehow DISAPPEARED from 3.0.5 (!?).  I just checked this on a
PC and am quite confused. Isn't the code for 3.2a1pre a linear
descendant of 3.0.x series?

Another question

After so many postings (100-plus), and somewhat divergent 
discussion, should I probably separate the thread of this
slight anomaly and problem caused by  "right-clicking  and then Save
As" in TB3 into a separate bugzilla? 

I think so, since "right-clicking and then Save AS" in TB3 doesn't
involve "Download manager".  TB3 and Mozilla are very different, too.

That said, here is a new discover.

A new discovery. TB3 3.0.5 seems to fare much better(!).

While I was busy trying to fix the problem with the code path of
"Saving an attachment of an e-mail message by double clicking of
left-mouse button" in TB3, not in FF mind you, I found out that saving
attachment by 'right mouse button and then "Save As"' seems to have
been fixed almost in TB 3.0.5 (!).

Version
Mozilla/5.0 (X11; U; Linux i686; ja; rv:1.9.1.10) Gecko/20100512 Thunderbird/3.0.5

(Right-clicking and Save As case [Left clicking

(1) Write-protected case:

    TB 3.0.5 complains that the file could not be saved
    to a write-protected directory by a warning dialog.
    Once.

    So the message could be a more clearer and explicitly state
    the permission problem, but it is a great improvement.

    3.2a1pre shows the dialog TWICE (?) which is strange, OTOH.

(2) Non-existing directory case.
    It is handled as in 3.2a1pre correctly.
    So good.

However, strange umask problem is there.

Right clicking and "Save As" writes the attachment file with very restrictive
permission whereas the left-clicking writes the attachment with a less
restrictive permission.

Below shows the file permission and umask.



ishikawa@dell-w2k-note$ ls -l /tmp/*dir

--- (This one is saved by right clicking and then "Save As")

/tmp/a-dir:
total 72
drwxr-xr-x 2 ishikawa ishikawa  4096 Jun  2 12:00 ./
drwxrwxrwt 9 root     root     45056 Jun  2 12:04 ../
-rw------- 1 ishikawa ishikawa  6937 Jun  2 12:00 Saved-by-right-clicking.eml

--- (This is a write-protected directory for testing purposes.)
/tmp/b-dir:
total 60
dr-xr-xr-x 2 ishikawa ishikawa  4096 Jun  2 11:59 ./
drwxrwxrwt 9 root     root     45056 Jun  2 12:04 ../

--- (This one is saved by double left-clicking")
/tmp/c-dir:
total 96
drwxr-xr-x 2 ishikawa ishikawa  4096 Jun  2 12:06 ./
drwxrwxrwt 9 root     root     45056 Jun  2 12:04 ../
-rw-r--r-- 1 ishikawa ishikawa 30076 Jun  2 12:06 xxyyzz.docx


My umask setting.

ishikawa@dell-w2k-note$ umask
0022


(a) Dialog message can be improved : it should mention permission
problem explicitly when saving an attachment fails due to permission
problem.

(b) umask and/or permission : two different file permissions after
saving an attachment by "Left-clicking" and "Right-clicking" are very
confusing, and this behavior should be fixed.

(a) is cosmetic and minor.
(b) is very problematic and should be fixed.

( 3.2a1pre seems to regress on (a) since it shows the warning dialog
  TWICE at least under my experimental setup.
  I wonder if this is reproduced on other people's machines.)

So in a sense, the problem with umask is the different file permissions
used by the two different code paths invoked left-clicking and
right-clicking, and that the file permission setting has
become very restrictive (?) in one case.

In my personal case, I would prefer the lenient setting of
left-clicking case as above. Maybe it should be a user-settable
preference?

PS: I think I will file a separate bugzilla for this
"Right-clicking and then Save As" for TB3.

Comment 146

[[not reading bugmail]]

(In reply to comment #145)
> Now a big question:
> 
> Something is fishy here. Is the related code very different in
> mozilla-central presumably used by TB 3.0.5, and comm-central that I
> have been using to build my test builds for debugging?
> 
> I have a feeling that maybe the broken ancient code in the original
> mozilla-central is carried now by comm-central while the problematic
> code has been fixed (?) in mozilla-central and is now reflected back to the
> 3.0.5.  3.2a1pre problem for left-clicking seems to be real from
> what I checked. The bug persisted as it did for a long time.  BUT this
> problem somehow DISAPPEARED from 3.0.5 (!?).  I just checked this on a
> PC and am quite confused. Isn't the code for 3.2a1pre a linear
> descendant of 3.0.x series?

If it makes fixing this bug more manageable for each significant issue, filing separate bugs that block this bug makes some sense.

Comment 147

[ISHIKAWA, Chiaki]

(In reply to comment #146)
> ...
> If it makes fixing this bug more manageable for each significant issue, filing
> separate bugs that block this bug makes some sense.

I will do so in the hope that separate bugzilla entries each bug focusing on a 
problem of limited scope helps us fix these bugs entirely.

That said, about the following observation: I was wrong. Mea Culpa.

> BUT this problem somehow DISAPPEARED from 3.0.5 (!?).  I just checked this on a
> PC and am quite confused. Isn't the code for 3.2a1pre a linear
> descendant of 3.0.x series?

On this particular PC which I tested TB3 this lunchtime, I had replaced the
original TB3's nsHelperAppDlg.js with the one that I fixed (i.e., like the one
I posted here and in different bugzilla entry.)
That is why the test cases worked(!).

Sorry about the noise.
Once I replaced nsHelperAppDlg.js with the original, and restarted TB3.0.5 the bug manifested again.
The saving to the write-protected directory silently failed. (Bad.)


Again, I think limiting the bug entry to a  narrow focus is a way to go.
I will post a different entry for TB3 specific entry for the
related bug [left-clicking] later today or tomorrow.

TIA

Comment 148

[ISHIKAWA, Chiaki]

>Again, I think limiting the bug entry to a  narrow focus is a way to go.
>I will post a different entry for TB3 specific entry for the
>related bug [left-clicking] later today or tomorrow.

I meant to say [Right-clicking] case here.

I filed a bugzilla entry:
Bug 569807  - Right-clicking on an attachment and choose "Save AS" in TB3 and try to save to a write-protected directory shows error dialog TWICE 

It turns out that TB 3.0.5 also shows the error dialog twice (I reported 3.0.5 fixed the problem by showing it only once. I was wrong. Sorry for misleading early report) 

I wish someone takes care of this bug and others once for all soon :-(

TIA

Comment 149

[Joe Leikhim]

Please look at my critical data loss Bug 632431 - Save Attachments to full drive results in WP Error and damaged file allocation table. It may be related.

Comment 150

[ISHIKAWA, Chiaki]

(In reply to comment #149)
> Please look at my critical data loss Bug 632431 - Save Attachments to full
> drive results in WP Error and damaged file allocation table. It may be
> related.

I have filed a new bug for linux's case of saving an attachment in an almost full file system.

Bug 663071
Saving an attachment to an almost full filesystem generates an erroneous error message(linux), ...


There the system remains more or less sane, but the shown error message
is very misleading and the partially saved incomplete file is left behind. (and the file descriptor to it seems not even closed immediately. I think it is closed at the exit of TB3 automatically.).

The code that detects the error condition 
is clearly buggy (in the sense that it misdiagnoses the problem),
and is the file descriptor is not closed, it is likely to
make a trouble when your file system overflows under XP.

But these should be discussed in Bug 632431 and Bug 663071.

As far as I can tell, nsHelperAppDlg.js is not likely the culprit.
It seems to be caused by improper error handling in
lower-level routines in other files that handles file I/O, writing in this case.

Comment 153

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Comment 154

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mak, could you have a look please?

For more information, please visit BugBot documentation.