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.

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.

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

it seems that bug 270557 regressed

It is happening in Windows xp also.

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.

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.

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

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

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

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.

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 ?

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

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.

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

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

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.

Tryserver builds showing up here: https://build.mozilla.org/tryserver-builds/2008-11-21_13:28-sdwilsh@shawnwilsher.com-try-5dbb7e15f5b/

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"

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

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

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

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.

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!

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.

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

Thanks Stephen - looks good.

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]

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

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

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

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

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

(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

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)

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

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.

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

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.

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?

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

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

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

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.

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.

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

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

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

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

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

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

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

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.

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

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.

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;

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

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.

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

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

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

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

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.

(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 on attachment 422754 [details] [diff] [review] Patch to properly warn the user when the download directory is write protected. per comment 94

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

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

(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!

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

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.

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

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

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.

(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

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.

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!

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.

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?

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?

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

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

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

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.

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

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.

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.

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.

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

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

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

(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

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

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$

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

OK, I got confused a little bit. Should I modify mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in in the end?

Comment on attachment 436927 [details] Re-indented version of src/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js Please upload patches, not actual files.

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.

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

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

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.

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

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

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

(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

(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

An early-bird view for evaluation.

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

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

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.

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

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

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

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.

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.

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

(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

>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

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.

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