928250 - Display an informative error when the selected download directory is write-protected, instead of failing silently

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[ISHIKAWA, Chiaki]

+++ This bug was initially created as a clone of Bug #567585 +++

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 ( .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100520 Thunderbird/3.2a1pre

Originally, I was discussing this problem in Bug 429827
Download manager does not warn when its download location does not exist or is write protected

But, per a latest suggestion,
I am branching out with a limited bug case to this new bugzilla
entry so that a sharper focus on a limited scope of a very specific bug 
is discussed here.

I want to limit the discussion here only to TB3's behavior regarding
the saving of an attachment of an e-mail article, when the LEFT mouse
button is clicked twice on the attachment.

I am limiting the bug reporting of my experience under linux, but
I believe this is observed under windows, too. (But correct me if I am wrong.).


Reproducible: Always

Steps to Reproduce:

Steps re-produce. 
I write a summary of what goes wrong and how when we try to
save an attachment to an e-mail article to a write-protected directory.

Create directories for testing:

       mkdir /tmp/a-dir
       mkdir /tmp/b-dir
       mkdir /tmp/c-dir

       create an empty file "existing" in /tmp/a-dir, and make it unwritable.
       touch /tmp/a-dir/existing
       chmod a-w /tmp/a-dir/existing
       
       touch /tmp/b-dir/existing
       chmod a-w /tmp/b-dir/existing
       
       Make directory /tmp/b-dir and /tmp/c-dir unwritable.
       chmod a-w /tmp/b-dir
       chmod a-w /tmp/c-dir


Step 1: Choose an e-mail article with an attachment.

     In my case, I was testing "libc-2.7.so" as attachment. 

     Double LEFT-click on the attachment.

     "Would you like to save this file?" dialog appears. (the attachment
     is recognized as BIN file.)
     There are buttons.
     [Cancel] [Save]

Step 2: Press Save and choose the save directory.

     Depending on the target direcory, there are a few scenarios.

     case (a)  /tmp/a-dir
     	  EXPECTED: TB3 should save the attachment.

	  WHAT HAPPENS.: OK, we can save the attachment.

     case (b)  /tmp/a-dir  but use "existing" as the name of the saved
          attachement. Then press "Save" button.

	  EXPECTED: Since there is a file "existing", TB3 should offer
	  the warning about existing file being overwritten.  Then
	  again, TB3 should save the attachement.
	  (Even if the file is unwritable, the directory is writable.)
	  
	  WHAT HAPPENS: OK. Behaves as expected.

	  (Funny, under linux, "existing" doesn't appear in the file
	  selection box during saving. I can change the filename for
	  the saved attachment to "existing", and get the warning.  I
	  am not sure if this is related to the file being
	  write-proteced or not(?). As it turns out, under my
	  environment, the file chooser only lists the existing files
	  with the same suffix, i.e., ".so" as the attachment. If I
	  create /tmp/a-dir/existing.so, it shows up in the file
	  chooser initially.)

     case (c)  /tmp/b-dir

          EXPECTED: Since the directory is write protected, 
	  TB3 should fail and warns the user that the file could not
	  be saved. Ideally, it should then offer to let user select
	  another directory.

	  WHAT HAPPENS.: NG. TB3 fails to save the attachment, but
	  there is no visual feedback about the error with GUI, and
	  TB3 proceeds as if it succeeded.

     case (d) /tmp/b-dir, but use "existing" as the name of the saved
          attachement. Then press "Save" button.

	  EXPECTED: Since there is a file "existing", TB3 should offer
	  the warning about existing file being overwritten.  But then
	  again, TB3 should fail saving since the directory is
	  unwritable. Ideally, it should offer to let user select
	  another directory (and a name).

	  WHAT HAPPENS.: NG. TB3 warns about the overwriting of
	  the existing file (so far, so good). , but then
	  it fails to save the attachment, and 
	  there is no visual feedback with GUI, and TB3 proceeds
	  as if it succeeded.

    case (e) /tmp/c-dir
    	 Use /tmp/c-dir, but before press the "SAVE" button,
	 remove the directory from another terminal.
	 (  chmod a+w /tmp/c-dir
	    rm -fr /tmp/c-dir  
         )
	 Then Press "SAVE".
    	 
         EXPECTED: TB3 should fail the saving and warns the user.
	 	   Ideally, it sould then offer to let the user to
	 	   choose another directory (and name).  

	WHAT HAPPENS:	   OK

Scoreshhet
Summaries of  TB3 behavior in the above scenarios.

     case (a)  OK
     case (b)  NG
     case (c)  NG
     case (d)  NG
     case (e)  OK


We need to fix the failure to raise an error when TB3 tries to
save an attachment to a write-protected directory with LEFT-clicking.




---

additional information.

The original  Bug 429827 discussion
is now a  mixture of 
 - mozilla's download, and 
 - and thunderbird saving an attachment,
and the error cases where
 - the drive (I think it is very windows-specific) suddenly disappers, or
 - the target directory is write-protected.

So the focus of bug behavior became blurred after about 100 posts unfortunately.
Per a latest suggestion, I decided to create this new bug entry so that
this particular bug is understood better by many and the fix is approved sooner.


Observation: 
I doubt the The wisdom of Mozilla browser and Thunderbird sharing a code for one
for "downloading from web" (presumably), and the other for "saving an
attachment of an e-mail article ". They are very different operation.

Thus, I wanted to limit the discussion only to TB3's behavior
regarding the saving of attachment of an e-mail article,
when the left mouse button is clicked on the attachment.

I say, "LEFT" button here because
even when we limit the discussion to TB only, i.e., saving attachment from
an e-mail article, the clicking of left mouse button and the right
mouse button invokes totally different code paths for saving the
attachment.  This is in the following bugzilla entry.
 
Bug 549719  - Should UNIFY/MERGE the two different code paths for saving an attachment in a message 

The error handling in the two paths are not quite the
same.  Also, the default path used when a previous saving attemp fails
obviously are different in the two code paths.

Here, I would like to limit the discussion only to TB3's saving an attachment
by LEFT-clicking.

TIA

Comment 1

[ISHIKAWA, Chiaki]

This is a slighted edited copy of
https://bugzilla.mozilla.org/show_bug.cgi?id=567585#c93
a comment posted to the original bugzilla.

I am cloning this bug for the following reasons.

The reason is as follows:
 - the relevant file, nsHelperAppDlg.js has undergone a major transition to
   ASYNC Javascript API, and the source looks a different now: (It uses async TASK
   processing.)
 - also, there was a discovery that there TWO different code paths( one that is invoked
   when user clicks on the attachment and is handled by JS code, and the other
   that uses [SAVE] button at the lower-right corner of the screen and is handled basically
   by .cpp code alone.) and I need to focus one at a time, and

 - as Gavin noted the patch accumulated many small patches over time, with 
   comments that address discoveries of new bugs and features all along,
   it needs cleaning up. A major surgery may be in order.

So I have decided to clone this bug and start afresh so that the
patch can be implemented in a following manner (more or less).

Since the
code in question has been modified to use the new Java ASYNC API, and thus
it is a good opportunity to start afresh.

I am attaching a patch to the current nsHelperAppDlg.js and then explains my discoveries
testing the operation under 32-bit linux and 64-bit linux separately.

TIA

Comment 2

[ISHIKAWA, Chiaki]

Attachment: Patch to fix the failure to warn the user that the saving of an attachment to write-protected directory failed.

This is a fix to make sure that an error dialog is shown when
the saving to the destination directory fails.

(More pieces to fill in the cases of errors other than access-permission error
will come and be filled in where (TODO/FIXME) strings are placed.

But for now, let us put this patch in.
 
TIA

Comment 3

[ISHIKAWA, Chiaki]

Here is the plan for developing the patch.

I am cloning the original bug since the
code has been modified to use the new Java ASYNC API, and thus
it is a good opportunity to start afresh.

---------
////////////////////////////////////////
// Strategy for fix
//
// (A) bare minimum.: Important, Dataloss, etc.
//     Show error when the saving fails (due to write-permission problems, etc.)
//  
    This bugzilla entry.

The following (B), (C), (D), and (E) can be taken care of later.

// (B) Use of of myownIOErrorString() for better I/O error reporting
// other than access permission issues.
//
// (C) Caution about the racey-check done by IsUsableDirectory(), and
//     that is why the code below it needs try{} catch() {} protection.
//
// (D) Issue of the usage of this file by TB and FF.
//     The default save location differs (and see (5) below).
//
// (E)	Addressing philosophical issue of using two different code paths for
//	clicking on the attachment, and [save] button on the
//	lower-right corner. The default save position is
//	different for both modes of operaton, and may be confusing.
---------

This is a data-loss issue.

Saving an attachment (by pressing on the attachment icon/name
using left-button) to a write-protected directory does not warn the
user that the operation has failed.

If the user is misled to believe that the saving had succeeded and
removed the attachment from the e-mail message, data loss can occur.

[CAUTION: note this bug entry is about saving the attachment by
clicking on it by left-button, and this operation has nothing to do
with the use of [save] button that is near the lower-right corner of
the window.  These use totally different independent codes, and both
code paths have shown different bugs and behavior over the last
several years while I checked.]

How to reproduce and how the patch (attachment) helps.

I will report two test real-world testing reports under 32-bit and
64-bit environment separately.
Both are essentially the same.
64-bit test case describes the anomaly in test step (6) in detail.
(Memo of 32-bit test case was written down after the current patch was created fully.)

I am using local DEBUG BUILD of comm-central thunderbird.
(The source was refreshed/updated this week.)

I tested both under 32-bit Debian GNU/Linux, and
64-bit Debian GNU/Linux (32-bit build under 32-bit linux, and
64-bit build under 64-bit linux).

[The problem was noticed originaly under Fedora 32-bit about, eh, 5 years ago.]

Comment 4

[ISHIKAWA, Chiaki]

Under 32-bit Debian GNU/Linux.

This is how I tested and observed the behavior of TB. The patch works as expected.

(1) Even before the main operation of saving the attachment to a
write-protected library, I see some warning lines
when I click on the attachment (with left button).

What happens is this:

- the dialog that asks me to open or save (what should Daily do with this
file?) is shown properly,
[Strange, under 64-bit, it asks "What should Thunderbird do with this
file?" Does not mention "Daily". Maybe a slight difference in mozconfig???.)

but at the same time

(i)- the console from which TB was started from the command shell
shows the following "<error>" message line.  (This also appears in
TB's built-in error console, too.)

This error is thrown all the way to the top without getting
caught. Bad, isn't it?

--- quote from console terminal log

sDocShell.cpp, line 8512
++DOMWINDOW == 30 (0x983ab30) [serial = 41] [outer = 0xac5b9c0]
--DOMWINDOW == 29 (0xb1043a8) [serial = 33] [outer = (nil)] [url = about:blank]
--DOMWINDOW == 28 (0x8db25c8) [serial = 37] [outer = (nil)] [url = about:blank]
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'<error>' when calling method: [nsIContentHandler::handleContent]"  nsresult: "0x805d0001 (<unknown>)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"	data: no]
************************************************************
++DOCSHELL 0x9aaffb0 == 15 [id = 18]
++DOMWINDOW == 29 (0x8d21888) [serial = 42] [outer = (nil)]
++DOMWINDOW == 30 (0xb1ebb58) [serial = 43] [outer = 0x8d21888]

---- end quote from console terminal log


Note: This has been filed as a separate bug:
Bug 928240 - Call to xpconnect wrapped JSObject produced this error: * [Exception... "'<error>' when calling method: [nsIContentHandler::handleContent]" nsresult: "0x805d0001 (<unknown>)"

This issue is noticed while testing 64-bit TB under 64-bit linux, too.

(2) preparation of directory/file for testing.

CAUTION: Under windows 7 and XP (with the previous versions of TB) using
local storage only, I could not produce an easy-to-reproduce test
scenario. Maybe it is because I use an account with administrative
privilege. I used a combination of |attrib| command from |cmd.exe|, and
|chmod| from cygwin shell console, to try to create a write-protected
directory, but no matter what, TB wrote to this directory
successfully. With a privileged account under Windows, I think only a
write-protected directory served by a remote file server may show the
symptom, or maybe a write-protected USB disk, etc. under Windows OS.


The following is how you can test under linux easily.

To check that the failure to try to save an attachment to a
write-protected directory, the following directories were prepared.

/tmp/t-dir:
Nobody (except for superuser) can write into it:

ls -laR /tmp/t-dir
/tmp/t-dir:
total 34
dr-xr-xr-x  2 root root	 1024 Oct 17 20:49 .
drwxrwxrwt 30 root root 32768 Oct 17 21:07 ..
-rw-r--r--  1 root root	    0 Oct 17 20:49 abc

/tmp/all-dir: anyone can write to it, but can't see the
existing files there, and  remove other users'
files there (note the 't' permission bit) .  (For example, there is a
file owned by superuser there.)

 ls -Ral /tmp/all-dir
/tmp/all-dir:
total 34
drwxr-xrwt  2 root root	 1024 Oct 17 21:02 .
drwxrwxrwt 30 root root 32768 Oct 17 21:27 ..
-rw-r--r--  1 root root	    0 Oct 17 21:02 owned-by-root-file


(3) Trying to save to the /tmp with my patch, which should succeed.

Result: It works (with/without the patch).

However, I notice the following lines in the console where thunderbird
was started from shell.

INFO: (debug) promptForSaveToFileAsync() is called
--DOCSHELL 0x9aaffb0 == 14 [id = 18]
WARNING: The dialog should nullify the dialog progress listener: file /new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp, line 1895
++DOCSHELL 0x9a04c00 == 15 [id = 19]
++DOMWINDOW == 31 (0xabca068) [serial = 44] [outer = (nil)]
++DOMWINDOW == 32 (0xa9a0180) [serial = 45] [outer = 0xabca068]
WARNING: Set file metadata failed: : 'metadata::download-uri という属性値はセットできません', file /new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/toolkit/components/downloads/nsDownloadManager.cpp, line 2597
WARNING: 1 sort operation has occurred for the SQL statement '0xb016fd0'.  See https://developer.mozilla.org/En/Storage/Warnings details.: file /new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/storage/src/mozStoragePrivateHelpers.cpp, line 110
--DOCSHELL 0x9a04c00 == 14 [id = 19]

3-(i) The line "The dialog should nullify the dialog progress
listener..." probably has something to do with the async downloader.
I have no clue about the seriousness of the warning. Anyone?

3-(ii) Note that "Set file metadata failed:". This line is output from the
following function which seems trying to set some additional data for the
data, but on linux, there is no metadata if I am not mistaken?
This seems highly related to Gnome desktop thingy.
(I have not set up the user account 'mtest2' fully to use the desktop. I am logging into it remotely using ssh on the local machine and share the X server with my developer login account.) 
I wonder if this should be suppressed or not by disabling MOZ_ENABLE_GIO?

mozilla/toolkit/components/downloads/nsDownloadManager.cpp:
(I wonder what MOZ_ENABLE_GIO stands for: Gnome I/O?.)

#ifdef MOZ_ENABLE_GIO
static void gio_set_metadata_done(GObject *source_obj, GAsyncResult *res, gpointer user_data)
{
  GError *err = nullptr;
  g_file_set_attributes_finish(G_FILE(source_obj), res, nullptr, &err);
  if (err) {
#ifdef DEBUG
    NS_DebugBreak(NS_DEBUG_WARNING, "Set file metadata failed: ", err->message, __FILE__, __LINE__);
#endif
    g_error_free(err);
  }
}
#endif

Issue 3-(i) and 3-(ii) are noticed while testing 64-bit TB under
64-bit linux, too.

(4) Saving to /tmp/t-dir:

OK, the real heart of my test.  Trying to save to /tmp/t-dir, which
SHOULD FAIL and the user ought to be notified.

Result: After clicking the attachment with left-button, and
then choose "Save File" and hit [OK] in the shown dialog, and choose
the directory /tmp/t-dir as destination and hitting [OK],
WITH MY PATCH, I get the following ERROR DIALOG when I tried
to save the attachment, good!:
(WITHOUT THE PATCH, no warning and nothing happens. TB pretends as if
saving of the attachment succeeded without an issue. BAD.)

The dialog's message:
The file could not be saved because you do not have the proper
permissions.  Choose another save directory.

Good :-)

4-(i) A fly in the ointment is that I see the following lines in the console
where thunderbird was started from the command shell.:
Note the WARNING: message.

--- quote ---
    ...
INFO: (debug) promptForSaveToFileAsync() is called
--DOCSHELL 0x9ac5d20 == 14 [id = 20]
Error: download failed due to access permission error = Error: Destnation directory non-existing or permission error
WARNING: dependent window created without a parent: file /new-hd1/extra/ishikawa/TB-3HG/NEW-COMMSRC/mozilla/toolkit/components/startup/nsAppStartup.cpp, line 621
++DOCSHELL 0x9ac5d20 == 15 [id = 21]
++DOMWINDOW == 31 (0xa0a5670) [serial = 48] [outer = (nil)]
++DOMWINDOW == 32 (0x906d268) [serial = 49] [outer = 0xa0a5670]
--DOCSHELL 0x9ac5d20 == 14 [id = 21]
--DOMWINDOW == 31 (0xa0a5670) [serial = 48] [outer = (nil)] [url = chrome://global/content/commonDialog.xul]
--DOMWINDOW == 30 (0x8d21888) [serial = 46] [outer = (nil)] [url = chrome://mozapps/content/downloads/unknownContentType.xul]
--DOMWINDOW == 29 (0x906d268) [serial = 49] [outer = (nil)] [url = about:blank]
--DOMWINDOW == 28 (0xabca068) [serial = 47] [outer = (nil)] [url = about:blank]

--- end quote ---

4-(i) I am afraid that "WARNING: dependent window created without a
parent" may have to be looked into.

This issue, 4-(i), is noticed while testing 64-bit version of locally
built TB under 64-bit linux, too.

[CAUTION: Again note this is about saving the attachment by clicking
on it by left-button, and this has nothing to do with the use of
[save] button that is near the lower-right corner of the window.]

(5)  Saving attachment to /tmp/all-dir under a new name.

Now let us try to write to /tmp/all-dir under a new name, which SHOULD SUCCEED.

Result: saving succeeded.
Issues 3-(i), and 3-(ii) apply here, too, though.

(6) Saving attachment to /tmp/all-dir/ under the existing name
(owned-by-root-file)

Trying to write over /tmp/all-dir/owned-by-root-file, which
SHOULD FAIL.

Result: I typed the file name as owned-by-root-file manually (because
I know the name in advance. file picker doesn't show the existing file
to the user, though, due to 't' permission but no 'x' permission.)
Then when I hit OK (to save), I got a dialog to ask me if it is OK to
remove the file.  I said OK to replace and proceed,
WITH MY PATCH, I get the proper error dialog in this case:

The file could not be saved because you do not have the proper permissions.  Choose another save directory.

Good!
(well, the message may be slightly misleading in this particular case
since the directory as a whole is usable and it is just that we tried
to overwrite a particular file, which was not allowed. But the error
dialog to notify the user of the failure is much better than the
current SILENT failure.)

What happens WITHOUT MY PATCH is as follows:
- without my patch, the file was NOT removed, but a new filename
  owned-by-root-file(1) is automatically created and
  the saving is done to this file.
  I don't think this is the intended behavior because I hit [replace]
  explicitly. So I take it to be a bug.

TIA, the 64-bit version observation follows.

Comment 5

[ISHIKAWA, Chiaki]

Under this 64-bit Debian GNU/Linux.

Basically the re-hash of 32-bit testing, but a few twists and
things I forgot to tell in 32-bit testing.

(1) 

The issue 1-(i) as in 32-bit version exists.

>(i)- the console from which TB was started from the command shell shows
>the following "<error>" message.  (This also appears in TB's built-in...

>(1) Even before the main operating of saving the attachment to a
>write-protected library, I see some warning lines
>when I click on the attachment (with left button).

Note: This has been filed as a separate bug:
Bug 928240 - Call to xpconnect wrapped JSObject produced this error: * [Exception... "'<error>' when calling method: [nsIContentHandler::handleContent]" nsresult: "0x805d0001 (<unknown>)"


(2) test directories.

I used the same directories and files for testing.
/tmp/t-dir,
/tmp/all-dir,
/tmp/all-dir/owned-by-root-file

(3) Trying to save to the /tmp with/without my patch, which should succeed.

Yes, it succeeded, but it printed something about missing directory
under the account's home directory. (It happened under 32-bit testing,
but I failed to report it since this has more to do with GTK
integration, and I ignored it back then. But come to think of it, it is worth reporting.)

I think it has something to do with the GTK (Gnome environment).

--- quote log
WARNING: Failed to open external DTD: publicId "-//W3C//DTD SVG 1.1//EN" systemId "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" base "file:///REF-COMM-CENTRAL/comm-central/mail/themes/linux/mail/icons/closeTab.svg" URL "resource://gre/res/dtd/svg11.dtd": file /REF-COMM-CENTRAL/comm-central/mozilla/parser/htmlparser/src/nsExpatDriver.cpp, line 696
WARNING: Failed to open external DTD: publicId "-//W3C//DTD SVG 1.1//EN" systemId "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" base "file:///REF-COMM-CENTRAL/comm-central/mail/themes/linux/mail/icons/closeTab-active.svg" URL "resource://gre/res/dtd/svg11.dtd": file /REF-COMM-CENTRAL/comm-central/mozilla/parser/htmlparser/src/nsExpatDriver.cpp, line 696
++DOMWINDOW == 32 (0x44e7c18) [serial = 52] [outer = 0x1d702a8]
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'<error>' when calling method: [nsIContentHandler::handleContent]"  nsresult: "0x805d0001 (<unknown>)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"	data: no]
************************************************************
++DOCSHELL 0x2b1f960 == 16 [id = 22]
++DOMWINDOW == 33 (0x1ba5858) [serial = 53] [outer = (nil)]
++DOMWINDOW == 34 (0x33f83c8) [serial = 54] [outer = 0x1ba5858]
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
--DOMWINDOW == 33 (0x23287b8) [serial = 50] [outer = (nil)] [url = about:blank]
--DOMWINDOW == 32 (0x1ef21a8) [serial = 51] [outer = (nil)] [url = mailbox:///home/mtest2/.thunderbird/jrpya19e.default/Mail/localhost/Inbox?number=0]
LoadPlugin() /usr/lib/mozilla/plugins/librhythmbox-itms-detection-plugin.so returned 18c6110
LoadPlugin() /usr/lib/gnash/libgnashplugin.so returned 13f4480
LoadPlugin() /usr/lib/mozilla/plugins/libgnome-shell-browser-plugin.so returned 2566ee0
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050
--DOCSHELL 0x2b1f960 == 15 [id = 22]

(thunderbird:16078): Gtk-WARNING **: Attempting to store changes into `/home/mtest2/.local/share/recently-used.xbel', but failed: Failed to create file '/home/mtest2/.local/share/recently-used.xbel.XHXA5W': No such file or directory

(thunderbird:16078): Gtk-WARNING **: Attempting to set the permissions of `/home/mtest2/.local/share/recently-used.xbel', but failed: No such file or directory
WARNING: The dialog should nullify the dialog progress listener: file /REF-COMM-CENTRAL/comm-central/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp, line 1895
++DOCSHELL 0x2b1f960 == 16 [id = 23]
++DOMWINDOW == 33 (0x1918678) [serial = 55] [outer = (nil)]
++DOMWINDOW == 34 (0x27ae538) [serial = 56] [outer = 0x1918678]
WARNING: Set file metadata failed: : 'Setting attribute metadata::download-uri not supported', file /REF-COMM-CENTRAL/comm-central/mozilla/toolkit/components/downloads/nsDownloadManager.cpp, line 2597
WARNING: 1 sort operation has occurred for the SQL statement '0x4505130'.  See https://developer.mozilla.org/En/Storage/Warnings details.: file /REF-COMM-CENTRAL/comm-central/mozilla/storage/src/mozStoragePrivateHelpers.cpp, line 110
--DOCSHELL 0x2b1f960 == 15 [id = 23]

(thunderbird:16078): Gtk-WARNING **: Attempting to store changes into `/home/mtest2/.local/share/recently-used.xbel', but failed: Failed to create file '/home/mtest2/.local/share/recently-used.xbel.9DJK5W': No such file or directory

(thunderbird:16078): Gtk-WARNING **: Attempting to set the permissions of `/home/mtest2/.local/share/recently-used.xbel', but failed: No such file or directory
--DOMWINDOW == 33 (0x1ba5858) [serial = 53] [outer = (nil)] [url = chrome://mozapps/content/downloads/unknownContentType.xul]
--DOMWINDOW == 32 (0x1918678) [serial = 55] [outer = (nil)] [url = chrome://mozapps/content/downloads/downloads.xul]
--DOMWINDOW == 31 (0x27ae538) [serial = 56] [outer = (nil)] [url = about:blank]
--DOMWINDOW == 30 (0x33f83c8) [serial = 54] [outer = (nil)] [url = about:blank]
++DOCSHELL 0x37623f0 == 16 [id = 24]
++DOMWINDOW == 31 (0x1deaed8) [serial = 57] [outer = (nil)]
++DOMWINDOW == 32 (0x1ba5858) [serial = 58] [outer = 0x1deaed8]
++DOCSHELL 0x1266330 == 17 [id = 25]
++DOMWINDOW == 33 (0x23e52d8) [serial = 59] [outer = (nil)]
WARNING: NS_ENSURE_TRUE(NS_SUCCEEDED(rv) && subjPrincipal) failed: file /REF-COMM-CENTRAL/comm-central/mozilla/docshell/base/nsDocShell.cpp, line 8472
++DOMWINDOW == 34 (0x1852188) [serial = 60] [outer = 0x23e52d8]
++DOCSHELL 0x15d4880 == 18 [id = 26]

--- end quote log


The same issue from 32-bit versing exists.

3-(i) The line "The dialog should nullify the dialog progress
listener..." ...

3-(ii) Note that "Set file metadata failed:". ...

NEW: Under this newly created account for which proper setup of GNOME
has not been done yet,
I get the following lines in the log. It seems that
thunderbird tries to use the recent file usage information into
GTK-managed ~/.local directory.

>(thunderbird:16078): Gtk-WARNING **: Attempting to store changes into `/home/mtest2/.local/share/recently-used.xbel', but failed: Failed to create file '/home/mtest2/.local/share/recently-used.xbel.XHXA5W': No such file or directory
>
>(thunderbird:16078): Gtk-WARNING **: Attempting to set the permissions of `/home/mtest2/.local/share/recently-used.xbel', but failed: No such file or directory

I forgot to mention that under 32-bit version, I created
~/.local/share manually to shut up these Gtk-related warnings.

Maybe I should raise this topic: 3-(iii) Too much integration with GNOME?

3-(iv): Under 64-bit version, somehow I got this assertions.
>###!!! ASSERTION: GetMinimumWidgetSize was ignored: 'rect->width == indicator_size', file /REF-COMM-CENTRAL/comm-central/mozilla/widget/gtk/gtk2drawing.c, line 1050

This assertion is triggered when I toggle between the "save as file"
or "open (with an application)" and it seems the assertion message is
printed whenever, the wide horizontal pull-down style bar to select [application] was
toggled.

Maybe I should file a bug about this.

(4) Saving to /tmp/t-dir:

SHOULD FAIL:

Result:
WITHOUT MY PATCH, nothing happens. Not even an error dialog, or a dump
in built-in Error Console. BAD!
This will lead the user to believe the saving succeeded!
We know that the code for saving was invoked (but did not catch the
error to write to write-protected directory) by the following lines
on the console terminal from which thunderbird was invoked from the
command shell.

>(thunderbird:16078): Gtk-WARNING **: Attempting to store changes into `/home/mtest2/.local/share/recently-used.xbel', but failed: Failed to create file '/home/mtest2/.local/share/recently-used.xbel.39194W': No such file or directory

>(thunderbird:16078): Gtk-WARNING **: Attempting to set the permissions of `/home/mtest2/.local/share/recently-used.xbel', but failed: No such file or directory

[These would disappear once we create  $HOME/.local/share )

WITH MY PATCH, we get the proper error dialog as expected, GOOD.: "The
file could not be saved because you do not have the proper
permissions.  Choose another save directory."

[CAUTION: Again note this is about saving the attachment by clicking
on it by left-button, and this has nothing to do with the use of
[save] button that is near the lower-right corner of the window.
Hitting [save] button near the lower-right corner and choose
/tmp/t-dir as destination will show the following error dialog: "Unable
to save the attachment. Please check your file name and try again
later."	 If it mentions "permission", that would have been perfect,
but I digress.]

(5)  Saving attachment to /tmp/all-dir under a new name.

Now let us try to write to /tmp/all-dir under a new name, which should succeed.

Result: saving succeeded.
Issues 3-(i), and 3-(ii) apply here, too, though.

(6) Saving attachment to /tmp/all-dir under the existing name
(owned-by-root-file)

SHOULD FAIL.

Result:
When I type in the name "owned-by-root-file" as the destination
filename under /tmp/all-dir, I got the following dialog:

A file named "owned-by-root-file" already exists.  Do you want to replace it?
When I hit, [replace],

- WITHOUT MY PATCH, the file was NOT removed, but a new filename
  owned-by-root-file(1) is automatically created and
  the saving is done to this file.
  I don't think this is the intended behavior because  I hit [replace]
  explicitly. So I take it to be a bug.

    ls -lRa /tmp/all-dir/
    /tmp/all-dir/:
    total 56
    drwxr-srwt	2 root	 root  4096 Oct 18 07:47 ./
    drwsrwsrwt 24 root	 root 32768 Oct 18 07:47 ../
    -rw-------	1 mtest2 root  4565 Oct 18 07:43 Muttrc
    -rw-r--r--	1 root	 root	  0 Oct 18 07:33 owned-by-root-file
    -rw-------	1 mtest2 root  4565 Oct 18 07:46 owned-by-root-file(1)

    (Muttrc is the attachment name. I copied it from /etc directory.
     This plain text file is the first text file under /etc of Debian
     GNU/Linux when I sort the files in alphabetical order, and so I
     picked it up as attachment.)

WITH MY PATCH, the error dialog appears properly. Good: The file could
not be saved because you do not have the proper permissions.  Choose
another save directory.

I hope this post and the previous post show that the uploaded patch works as expected and
at least show that the current code has the serious issue of failing to
report to user in a clear-cut manner that the saving of an attachment failed!

TIA

Comment 6

[ISHIKAWA, Chiaki]

Other I/O errors can be handled in a similar manner as in the lines of patch(es) posted to the original bug, but they need to incorporate a function for I/O error code to string mapping, etc., and
so it will take time for inclusion. Let us include this major patch as posted first and
tackle the issues later.

TIA

Comment 7

[ISHIKAWA, Chiaki]

Oh yes, linux users, please try the error scenario of
creating a write protected directory and try saving an attachment to it 
and see what happens with your version of TB.

Say, to create /tmp/t-dir,
mkdir /tmp/t-dir
chmod a-w /tmp/t-dir

The above commands should create a write-protected directory on your linux PC for testing purposes.
If there is already /tmp/t-dir, substitute it with /tmp/t0-dir, etc.


For windows users, as I noted in Comment 4,
--- quote
CAUTION: Under windows 7 and XP (with the previous versions of TB) using
local storage only, I could not produce an easy-to-reproduce test
scenario. Maybe it is because I use an account with administrative
privilege. I used a combination of |attrib| command from |cmd.exe|, and
|chmod| from cygwin shell console, to try to create a write-protected
directory, but no matter what, TB wrote to this directory
successfully. With a privileged account under Windows, I think only a
write-protected directory served by a remote file server may show the
symptom, or maybe a write-protected USB disk, etc. under Windows OS.
--- end quote

I could not produce a working test environment to test this failure in the first place.
Maybe I need an user account without administrative privilege or something.
I would appreciate to hear any tips about this under Windows.
(But I don't have the resources to re-create LAN/WAN filesystem just for this local testing.)
I have not bothered to test write-protected USB-drive, or
removing a drive in the middle of testing (after I chose the destination directory, for example)
to simulate the failure cases reported over the years.
Once I get the code for showing the error code (and string) eventually, I will test these.
Right now, with the current patch, it would throw an error all the way to the top-level.


TIA

Comment 8

[:Paolo Amadini]

Comment on attachment 818854 [details] [diff] [review]
Patch to fix the failure to warn the user that the saving of an attachment to write-protected directory failed.

Limiting the bug to one specific case is a good idea.

You should provide a shorter patch (no unneeded "dump" calls, for example).

You should also provide, along with the patch, some manual steps to reproduce
for that case only. The explanation of other cases is unneeded here, it only
makes it more difficult to understand the important one. I think you may have
already provided those, but I can't find them among the longer explanations,
so you should provide the step-by-step instructions for that case only.

After writing the steps, you should apply this exercise: if there is code
that makes no difference on whether the test passes or not, it should be
removed from the patch.

Comment 9

[ISHIKAWA, Chiaki]

(In reply to :Paolo Amadini from comment #8)
> Comment on attachment 818854 [details] [diff] [review]
> Patch to fix the failure to warn the user that the saving of an attachment
> to write-protected directory failed.
> 
> Limiting the bug to one specific case is a good idea.
> 
> You should provide a shorter patch (no unneeded "dump" calls, for example).
> 
> You should also provide, along with the patch, some manual steps to reproduce
> for that case only. The explanation of other cases is unneeded here, it only
> makes it more difficult to understand the important one. I think you may have
> already provided those, but I can't find them among the longer explanations,
> so you should provide the step-by-step instructions for that case only.
> 
> After writing the steps, you should apply this exercise: if there is code
> that makes no difference on whether the test passes or not, it should be
> removed from the patch.

Thank you for your quick feedback/comment. In the previous postings, I tried to
summarize all the details that accumulated over many months, but it
was verbose admittedly. Yes, the cloning copied all the details of the
original post in the first article alone.
I should have simplified it. (But to start another
clone just for the purpose of cleaning it up sounds too ominous)

So I am going to post a somewhat changed patch (not much shorter in any
measure) and a set of steps to show that the codes in there are all
necessary and how to trigger the errors and patched code with clear
steps.  One of the steps is a rather complicated, but I can not help
it. It deals with such error case.

*** NOTE: This discussion is valid for FIREFOX as well as Thunderbird
since the code nsHelperAppDlg.js is shared between Firefox and
Thunderbird.

I am afraid that there are much fewer TB users than FF users.  So to
hit home that this bug is for FF as well and to wake up FF users under
linux, here is what a FF user under linux can do to see the error in
action.

[I tested this using firefox-27.0a1.en-US.linux-x86_64.tar.bz2 (I am
testing this on 64-bit Debian GNU/Linux.)]

Try saving something, say, ftp://ftp.mozilla.org//README to a
write-protected directory using context-menu (right click, save as) in
FF. This is the simple case (1) of TB's failure discussed in the next
posting.

Firefox shows a nice moving arrow, but actually README is not saved
there. No error dialog is shown without the patch discussed here.

(You don't even need to create a write-protected |/tmp/x-dir| for this
simple error case alone. Just try saving README to |/dev| and see that
Firefox pretends that it has saved the file. Nice, huh?)

TB is suffering from similar problems when it comes to saving
mail attachments to files.

Imagine the plight of TB corporate users who thought they had saved
important mail attachments to shared directories on corporate file
system (unfortunately write-protected at the moment) and did not
notice immediately that the saving didn't actually happen because
there was no visual feedback of the error shown by TB, then delete the
attachments from messages in the mail folder to reduce local file
system usage, and maybe later notice that the attachment is nowhere to
be found at all (after compaction of the folders). Agah... :-(

********

Comment 10

[ISHIKAWA, Chiaki]

Attachment: Patch take 2: actually a little more verbose than the first one above due to reasons mentioned in the accompanied post.

I re-created the patch with marker that says "part-1", "part-2", ...,
"part-4", and enabled dump statements which were made into comments in
the previous patch to show that the code in these marked parts are
executed for real during testing. (Makes the code a little verbose, but it is important to show that the code is used for real.)

- From the description of test steps below, the readers will see that
  three parts (part-1, part-2, part-3) for display error dialog are
  necessary to handle all the three cases mentioned below.

- since the display of error dialog of these cases is a common action,
  the extraction of the action and turning it into a function
  displayBadPermissionAlert() is worthwhile to save code count.

- |else| parts of |if (...)| that checks specifically for access permission
  error are necessary place holders: They are the places (marked with 
  ("// TODO/FIXME: other error.")
  where the code will be placed to properly catch and display
  different type of I/O errors such as non-existent directory,
  etc. caused by thread-races (and the interference from OTHER
  programs that run on the same PC). These errors
  are to be handled by future patch, patch to be submitted immediately
  after the basic patch here is accepted.

  I leave them in to clearly mark where such I/O errors can be
  similarly handled to display proper error dialog. (This is the
  future plan as I outlined in Comment 3 above.) Also by marking them like 
  this, someone other than me can tinker with the error reporting of other 
  I/O errors while we discuss the basic patch issues. 

- The part marked with part-4 is left in because, in the distant past,
  there were exceptions thrown through here ( See bug 429827, 567585)
  and I want to be kept informed of such errant behavior just in case.
  This file has a history of broken behavior for more than five years
  and so one is tempted to keep sanity by placing a safety net and 
  since the error discussed here is DATALOSS class, I think the
  added safety is worth it.

People may have a different view, but the reason I culled the longish patch in
the previous bugzilla to the current status is stated above. There is no extra fat to cut down IMHO.

Now below is a series of the steps
to reproduce the simple error cases under linux and show that all the
part-1, part-2, and part-3 and related changes are necessary.

Preparation.

Under a user account, which is different from the test user account
(in my case |mtest2|) create a couple of test directories.

I. Initial setup of test directories.

For step (1) below only, you can skip this and just try to save an
attachment by clicking on the e-mail attachment and try saving it to
write-protected directory such as |/dev|, and see there is no error
reported (buggy behavior).

The created directories.

/tmp/x-dir : not writable at all.
/tmp/y-dir : others can place a file but not read (tricky setup, but
             for making a directory writable, but not readable unless
             you know the exact filename, this is often done even
	     today, like for submission of answers to questionnaire,
	     etc. Or public ftp site's incoming directory often have
	     similar setup.)

Shell commands to create them 
      mkdir /tmp/x-dir
      chmod a-w /tmp/x-dir
      mkdir /tmp/y-dir
      chmod o+rwt /tmp/y-dir
      touch /tmp/y-dir/owned-by-other-file

Here is what the permission would look like:
    ls -laR /tmp/x-dir /tmp/y-dir 
    /tmp/x-dir:
    total 40
    dr-xr-xr-x  2 ishikawa root  4096 Oct 18 23:25 ./
    drwsrwsrwt 26 root     root 32768 Oct 18 23:25 ../

    /tmp/y-dir:
    total 40
    drwxr-xrwt  2 ishikawa root      4096 Oct 18 23:26 ./
    drwsrwsrwt 26 root     root     32768 Oct 18 23:25 ../
    -rw-r--r--  1 ishikawa ishikawa     0 Oct 18 23:26 owned-by-other-file

II. Test Steps.

The following should be done using a dedicated test account, in my
case |mtest2|, which ought to be different from the developer account
that was used to create test directory and such.

Run thunderbird under this test user account.

Step (1) Try saving an attachment to a write-protected file: /tmp/x-dir

(Or you can use |/dev| as the write-protected directory as the target
of save directory.) 

WHAT SHOULD HAPPEN.: TB ought to tell the user, that the saving failed.

WITHOUT THE PATCH, WHAT HAPPENS INSTEAD: 
This fails silently without any error shown to the user. BAD.

WITH THE PATCH, a proper error dialog is shown.
This error handling is done by code in "part-3" of the patch.
The following line is printed on the terminal console window from
which TB was invoked. 

ERROR: part-3. download failed due to access permission error = Error: Destination directory non-existing or permission error

Step (2) Try saving to /tmp/y-dir above and choose |owned-by-other-file| as
the destination file name.

(Be warned that under linux |owned-by-other-file| is not shown by file
picker at least under gnome due to 't' permission. This is the clever setup so
that people can write into it, but unless know the filename in advance, can't even know the presence of the file in the directory. )

During saving operation, the user gets the dialog: 
A file named "owned-by-other-file" already exists.  Do you want to replace it?

When the user sees this, answer [replace] by pushing [replace] button.

WHAT SHOULD HAPPEN: TB ought to show that the saving failed since the
test user account cannot remove the existing file under a directory
with sticky bit set ('t'). Thus the whole saving operation fails, and TB ought
to tell the user about it.

WITHOUT THE PATCH, WHAT HAPPENS INSTEAD:
Without the patch, a file named owned-by-other-file(1) is created
silently as the saved attachment.
I think this is patently wrong since the user EXPLICITLY REQUESTED THE
REPLACEMENT.

WITH THE PATCH, the error dialog is shown that the saving can not
proceed.  This error is handled by the code marked as "part-2".
The following line appears in the terminal console window where TB was
invoked.

ERROR: part-2: download failed due to access permission error = [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: file:///REF-OBJ-DIR/objdir-tb3/mozilla/dist/bin/components/nsHelperAppDlg.js :: LastDirCallback :: line 328"  data: no]

Step (3) Use of a user default download directory.

The following slightly complex case again produces silent failure, and
the code in the position marked as "part-1" ought to protect us from
the silent failure by properly showing error dialog.

This is the case of the user choosing the automatic downloading to a
user's default download directory, and the saving failure occurs when
the default download directory somehow becomes write-protected.

For this scenario to happen, we need a configuration change of
thunderbird using config editor of the advanced configuration in advance.

(3)-i. INITIAL CONFIGURATION SETUP.

In the case of thunderbird, this is how the automatic download is
enabled.

From the Menu box (upper right corner)
->  preference 
  -> advanced 
    -> general 
      -> advanced configuration
        -> config editor

The following option needs to be set to true: this entry may not exist
according to some documents. In that case you need to create one.
If it already exists, click the entry to toggle the value, true <-> false.
You need to set this value to true.

  browser.download.useDownloadDir; true

What is your DEFAULT DOWNLOAD DIRECTORY?

Now where does the automatic download go?
[This may be system-dependent. You may need to tinker with Firefox to
find out where it will go. I think the choice is the same for TB under
the same OS by default.
In firefox, you can find out the default directory,
Using Edit 
      -> Preferences 
        -> General 
           -> Always Download to [which is set to |Download| by default]
 ]
In my case, i.e., Debian GNU/Linux, the default download directory is
|~/Desktop|, or |$HOME/Desktop|: that is 'Desktop' directory under one's own home
directory. I suspect this is used commonly by all the linux
distributions, but YMMV.

3-(ii): Now try automatic saving to make sure it works.

Try saving a known attachment using the automatic download behavior a couple
of times.  And look for the saved attachments under the default
download directory.

Example.
I found the following files after a couple of automatic
saving of an attachments named 'Muttrc' under the default directory.

Example: automatic saving result under Debian GNU/Linux

/home/mtest2 is the home directory of the test user 'mtest2'.

Output of ls -alR /home/mtest2/Desktop:
/home/mtest2/Desktop:
total 24
drwxr-xr-x  2 mtest2 mtest2 4096 Oct 19 00:03 .
drwxr-xr-x 11 mtest2 root   4096 Oct 18 01:15 ..
-rw-------  1 mtest2 mtest2 4565 Oct 18 23:59 Muttrc
-rw-------  1 mtest2 mtest2 4565 Oct 19 00:03 Muttrc(1)

Muttrc is the name of the attachment which I tried to save.

In this automatic download case, TB does not ask for the
deletion/replacement of the existing file (as was done in Step-(2)).

Instead, an automatic name |Muttrc(1)| is created and used for the second
automatic download.  Now I see the filename automatic generation is
handy in such a case.

3-iii: What happens when /home/mtest2/Desktop becomes write-protected?

But what happens if /home/mtest2/Desktop gets suddenly write-protected?

So to test this case, I changed user mtest2's /home/mtest2/Desktop to be
write-protected from a different terminal console window.

Command to do this is:
chmod a-w /home/mtest2/Desktop 
(this ought to be done by a super user or by mtest2 account.)

Now the automatic download (i.e, the saving of attachment in TB's
case) to the user default directory |/home/mtest2/Download| should now fail.

WHAT SHOULD HAPPEN: since the saving of the attachment to the default
directory now fails, the user should be given error dialog to learn
the error happened.

WITHOUT THE PATCH, WHAT HAPPENS INSTEAD: TB fails to save the
attachment SILENTLY. User does not receive any error feedback at
all. BAD(!)

WITH MY PATCH, yes TB fails to save the attachment, but the error
dialog is properly shown. Good.  (albeit the same text as in other
cases. It would be super if we get to see the directory name
there since the user has not selected directory immediately preceding this step. But showing the directory is not that simple due 
to character encoding issues of the directory name, etc. So I leave this as it is. Installing the basic patch to cure this serious problem is the first priority.)

The dump output inside the terminal console window where TB was invoked from the command line shows that this error is handled by the code marked as
"part-1"  in the patch.

INFO: (debug) user preferred downloads directory = [xpconnect wrapped nsIFile @ 0x44ee7a0 (native @ 0x4480650)]
ERROR: part-1 download failed due to access permission error = Error: Destination directory non-existing or permission error

cf.  To test the step 3 for FIREFOX, I need to tweak the setting of FF.

(1) Default download directory name.

For FF, we can change the default download directory using 
Edit 
  -> Preferences 
    -> General 
       -> (select) Always Download to [which is set to |Download| by default]

I changed this directory to |/tmp/z-dir| for easy testing of FF's
failure case. I do not want to change the permission setting of
~/Download too often.

(This configuration change seems to set the following preference to
  true automatically.  
  browser.download.useDownloadDir; true
But you may need to check the value just to be sure by means of
  |about:config| URL.

I hope the above clarifies the errors that the patch tries to address
and how to reproduce them with TB under linux (and if someone is interested in FF in a similar manner.)  

TIA

Comment 11

[ISHIKAWA, Chiaki]

Geez. Somewhere I began mistyping Downloads in place of Desktop interchangeably. I hope the reader is not confused too much.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 819095 [details] [diff] [review]
Patch take 2: actually a little more verbose than the first one above due to reasons mentioned in the accompanied post.

Review of attachment 819095 [details] [diff] [review]:
-----------------------------------------------------------------

just some quick driveby comments. 
probably most dumps should go...

::: toolkit/mozapps/downloads/nsHelperAppDlg.js
@@ +182,5 @@
> +  //
> +  // displayBadPermissionAlert()
> +  // show the bad permission of folder/directory panel.
> +  //
> +  // Made into an independent function to avoid dupicate codes.

that why functions are normally used ;)

Documentation usually tells WHAT the function does, not why. So in this case like

/**
 * Display an alert about bad file access permissions (??)
 */

@@ +187,5 @@
> +  //
> +  displayBadPermissionAlert: function () {
> +    let prompter =
> +      Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
> +                .getService(Components.interfaces.nsIPromptService);

Serivces.prompt

@@ +192,5 @@
> +
> +    let bundle =
> +      Components.classes["@mozilla.org/intl/stringbundle;1"]
> +                .getService(Components.interfaces.nsIStringBundleService)
> +                .createBundle("chrome://mozapps/locale/downloads/unknownContentType.properties");

Services.strings

@@ +218,5 @@
>  
>    promptForSaveToFileAsync: function(aLauncher, aContext, aDefaultFile, aSuggestedFileExtension, aForcePrompt) {
>      var result = null;
>  
> +    // dump("INFO: (debug) promptForSaveToFileAsync() is called\n");

remove uncommented code

@@ +228,5 @@
>      let bundle = Components.classes["@mozilla.org/intl/stringbundle;1"].
>                              getService(Components.interfaces.nsIStringBundleService).
>                              createBundle("chrome://mozapps/locale/downloads/unknownContentType.properties");
>  
> +    let saved_this = this; // to refer to display

usually it's called "self"
let self = this;

@@ +245,5 @@
>            let defaultFolder = yield Downloads.getPreferredDownloadsDirectory();
>  
>            try {
> +            dump("INFO: (debug) user preferred downloads directory = "
> +                 + defaultFolder + "\n");

remove the dump, this is just debug info

@@ +256,3 @@
>                // Display error alert (using text supplied by back-end)
> +              // The following function can not be prefixed by |this| any more since
> +              // |this|, within this function body, refers to the task in this ASYNC version

just use self, it should be clear to people what it's about

@@ +387,5 @@
>     * Ensures that a local folder/file combination does not already exist in
>     * the file system (or finds such a combination with a reasonably similar
>     * leaf name), creates the corresponding file, and returns it.
>     *
> +   * Error ought to be thrown!

once that's the case, add "@throws an error when permissions don't allow access"

Comment 14

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin from comment #13)
> Comment on attachment 819095 [details] [diff] [review]
> Patch take 2: actually a little more verbose than the first one above due to
> reasons mentioned in the accompanied post.
> 
> Review of attachment 819095 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> just some quick driveby comments. 

Thank you for the comments.

> probably most dumps should go...

Yes, I should have mentioned that the second upload is a work-in-progress, and some dumps are added to show which part is executed for each error case so that developers can tinker on their local PC.

I say local PC because, after five years, I am now fully aware that there does not seem to be a |make mozmill| test that handles the issue of saving attachments to write-protected directory.
Once the patch is in, I will try to create the test case(s) to make sure the bug won't creep back.
 
> ::: toolkit/mozapps/downloads/nsHelperAppDlg.js
> @@ +182,5 @@
> > +  //
> > +  // displayBadPermissionAlert()
> > +  // show the bad permission of folder/directory panel.
> > +  //
> > +  // Made into an independent function to avoid dupicate codes.
> 
> that why functions are normally used ;)
> 
> Documentation usually tells WHAT the function does, not why. So in this case
> like
> 
> /**
>  * Display an alert about bad file access permissions (??)
>  */
> 

Right. I thought the lengthy function name was good enough, but add a comment.

> @@ +187,5 @@
> > +  //
> > +  displayBadPermissionAlert: function () {
> > +    let prompter =
> > +      Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
> > +                .getService(Components.interfaces.nsIPromptService);
> 
> Serivces.prompt
> 
> @@ +192,5 @@
> > +
> > +    let bundle =
> > +      Components.classes["@mozilla.org/intl/stringbundle;1"]
> > +                .getService(Components.interfaces.nsIStringBundleService)
> > +                .createBundle("chrome://mozapps/locale/downloads/unknownContentType.properties");
> 
> Services.strings

The above two sentences are from the original file, but now I understand that there is a shorter
idiom.

> @@ +218,5 @@
> >  
> >    promptForSaveToFileAsync: function(aLauncher, aContext, aDefaultFile, aSuggestedFileExtension, aForcePrompt) {
> >      var result = null;
> >  
> > +    // dump("INFO: (debug) promptForSaveToFileAsync() is called\n");
> 
> remove uncommented code
>

I will remove this dump after an initial feedback.

> @@ +228,5 @@
> >      let bundle = Components.classes["@mozilla.org/intl/stringbundle;1"].
> >                              getService(Components.interfaces.nsIStringBundleService).
> >                              createBundle("chrome://mozapps/locale/downloads/unknownContentType.properties");
> >  
> > +    let saved_this = this; // to refer to display
> 
> usually it's called "self"
> let self = this;

I see. I will use |self|. (I am not a regular javascript programmer and it shows.)
 
> @@ +245,5 @@
> >            let defaultFolder = yield Downloads.getPreferredDownloadsDirectory();
> >  
> >            try {
> > +            dump("INFO: (debug) user preferred downloads directory = "
> > +                 + defaultFolder + "\n");
> 
> remove the dump, this is just debug info

will do after the initial review : again, this was an attempt to show which directory was used in this case. It turns out that it is easier to see this by looking inside preference setting.

> @@ +256,3 @@
> >                // Display error alert (using text supplied by back-end)
> > +              // The following function can not be prefixed by |this| any more since
> > +              // |this|, within this function body, refers to the task in this ASYNC version
> 
> just use self, it should be clear to people what it's about

Will do.

> 
> @@ +387,5 @@
> >     * Ensures that a local folder/file combination does not already exist in
> >     * the file system (or finds such a combination with a reasonably similar
> >     * leaf name), creates the corresponding file, and returns it.
> >     *
> > +   * Error ought to be thrown!
> 
> once that's the case, add "@throws an error when permissions don't allow
> access"

Wil do. "@throws an error such as permission doesn't allow creation of file, etc." is more like it since stranger things do happen due to races caused by external programs (like the security issues caused by races in mktemp or even a deletion of the directory AFTER the destination 
directory is chosen by the user. You may never know when the admin of the remote file system changes directory hierarchy or the network is down.)

I will incorporate your suggestion and upload a new patch over the weekend.

TIA

Comment 15

[ISHIKAWA, Chiaki]

Attachment: Patch take 3 (work-in-progress) incorporated suggestions from comment 13

Incorporated comments (comment 13). Locally checked. (I don't think |make mozmill| has test cases for this.)
The patch (WIP) still dump() for people to see for themselves which error scenario triggers which part of the code. (dump ("part-NNN...") will be removed in the final form.)

TIA

Comment 16

[:Paolo Amadini]

Comment on attachment 819543 [details] [diff] [review]
Patch take 3  (work-in-progress) incorporated suggestions from comment 13

(In reply to ISHIKAWA, Chiaki from comment #15)
> Incorporated comments (comment 13).

Thank you, and thanks to Magnus Melin for the first pass review!

> (dump ("part-NNN...") will be removed in the final form.)

I'd like to see something as close to the final form as possible now.

Comment 17

[ISHIKAWA, Chiaki]

Attachment: Patch take 4 : incorporated suggestions from comment 13 and a few more.

Thank you for your comment.

Here is the "final" form for stage-1.

I further modified a few verbose lines to the form suggested by Magnus Melin to use Services.prompt.
The fewer the lines, the better, I think.

In my guesstimate, stage-1 should handle 75%-85% of the cases of failure
to save attachment to a write-protected directory under linux
and not warned as such.

(In my future plan, stage-2 will follow.
For stage-2 to address other types of I/O error, I need to incorporate the mapping function of an I/O error code to a short English message. 
Such function does not exist today.
So I wrote such a function.
Although I could pollute and enlarge nsHelperAppDlg.js like I did in 
https://bug567585.bugzilla.mozilla.org/attachment.cgi?id=705863
(See the helper function near the beginning),
I think this is unnecessary baggage for nsHelperAppDlg.js.
So I would try to have such functionality incorporated into Fileutils.jsm.

Once that is finished (I have no idea when), I will try to add to nsHelperAppDlg.js the necessary code to
handle the other I/O error cases. They will be placed placed in the three locations marked with 
          // TODO/FIXME: other error.: This part will be changed in stage-2.

as a simple function call with the exception/error as an argument.

Stage-2 will add a couple of helper functions to use the I/O error code to English string matching and an error dialog routine to handle the generic case. 
But again, I can't tell when that happens.

In the meantime, I believe many corporate users who got bitten by write-protected directories will now find the simple error cases reported correctly.
(75-85% are simple I/O errors handled by the basic changes in the current patch.)

TIA

Comment 18

[:Paolo Amadini]

Comment on attachment 819797 [details] [diff] [review]
Patch take 4 : incorporated suggestions from comment 13 and a few more.

Review of attachment 819797 [details] [diff] [review]:
-----------------------------------------------------------------

The current patch is much better than the previous iteration, though it still contains too much meta-comments to match our coding style, and there are still some unneeded functional modifications.

I made a detailed list of the changes that should be made to prepare the patch for another review. For the most part, they are just about removing unneeded code and comments, so they should be quite easy to handle.

::: toolkit/mozapps/downloads/nsHelperAppDlg.js
@@ +1,1 @@
> +/* -*- Mode: javascript; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2; js-indent-level: 2; -*- */

The original mode line is correct, don't change it. See "mode line" here:

https://developer.mozilla.org/Developer_Guide/Coding_Style

@@ +186,5 @@
> +  // to be added later in stage-2.
> +  // Handling this particular case was easy since the
> +  // appropriate message string was already in string bundle.
> +  // Stage-2 will add the I/O error code to English string mapping, somewhere,
> +  // maybe in Fileutils.jsm, and is expected to take time.

Remove meta-comment (everything from "cf." to "time.")

@@ +192,5 @@
> +  displayBadPermissionAlert: function () {
> +    let bundle =
> +      Services.strings.createBundle("chrome://mozapps/locale/downloads/unknownContentType.properties");
> +
> +    // Display error alert (using text supplied by back-end)

This comment from the original source is not needed anymore here, remove it.

@@ +215,5 @@
> +  // generally speaking,
> +  // - null was returned if the user cancelled the operation.
> +  // - valid path name was returned if the saving succeeded.
> +  // - So errors such I/O error needed to be THROWN as exception.
> +  // There was no other way out with the current fraemwork.

Remove this comment.

@@ +232,5 @@
> +    let bundle =
> +      Services.strings
> +              .createBundle("chrome://mozapps/locale/downloads/unknownContentType.properties");
> +
> +    let self = this; // to refer to displayBadPermissionAlert() routine.

Since we call "bind(this)" on the function below, you can just use "this" inside the function, the "self" variable is unneeded. Remove.

@@ +257,1 @@
>                // Display error alert (using text supplied by back-end)

Remove meta-comment. Also the original "display error alert" comment can now be removed because we have a clear function name.

@@ +260,5 @@
>                return;
> +            } else {
> +              // TODO/FIXME: other error.: This part will be changed in stage-2.
> +              dump("ERROR: download failed. error = " + ex + "\n");
> +              throw ex;

We should fall through to the file chooser in case of error here, so this "else" block should be removed.

@@ +334,5 @@
> +            // we can encounter an error if the directory finally chosen or
> +            // the file is (write-)protected from the |remove| above.
> +            // Note the example of /tmp/all-dir with 't' permission bit,
> +            // and trying to replace the 'owned-by-root-file' there.
> +            // (in https://bugzilla.mozilla.org/show_bug.cgi?id=928250#c10 )

This is a different case. Currently, if we can't remove the file, we just use a different name. If we want to change this, we can do that in a different bug.

Please remove the change to the "catch" block.

@@ +354,5 @@
>            // Do not store the last save directory as a pref inside the private browsing mode
>            gDownloadLastDir.setFile(aLauncher.source, newDir);
>  
> +          try {
> +              result = this.validateLeafName(newDir, result.leafName, null);

Too much indentation, remove two spaces.

@@ +359,5 @@
> +          }
> +          catch (ex) {
> +            if (ex.result == Components.results.NS_ERROR_FILE_ACCESS_DENIED) {
> +              // part-3.
> +              // Display error alert (using text supplied by back-end)

Remove comments here.

@@ +366,5 @@
> +              return;
> +           } else {
> +             // TODO/FIXME: other error.: This part will be changed in stage-2.
> +             dump("ERROR: download failed. error = " + ex + "\n");
> +             throw ex;

Unfortunately, re-throwing the exception here would cause the download to hang. Remove the entire "else" block, so that we fall through to calling saveDestinationAvailable(result) below, with result still set to null.

@@ +369,5 @@
> +             dump("ERROR: download failed. error = " + ex + "\n");
> +             throw ex;
> +           }
> +          } /* catch */
> +        } /* if (result ) */

Remove comments after closing braces, we don't use them in our coding style.

@@ +401,5 @@
> +      var err = new Error;
> +      err.message = "Destination directory non-existing or permission error";
> +      err.result  = Components.results.NS_ERROR_FILE_ACCESS_DENIED;
> +      throw err;
> +      // return null;

throw new Components.Exception("Destination directory non-existing or permission error", Cr.NS_ERROR_FILE_ACCESS_DENIED);

Remove both the comment and the commented code.

@@ +423,5 @@
> +      // If there is an error thrown here, we will catch it with the dump
> +      // string.
> +      dump("ERROR: part-4: createNiceUniqueFile failed. error = " + ex + "\n");
> +      throw ex;
> +    }

Revert this whole code block, returning to the original createdFile assignment.

Comment 19

[ISHIKAWA, Chiaki]

(In reply to :Paolo Amadini from comment #18)
> Comment on attachment 819797 [details] [diff] [review]
> Patch take 4 : incorporated suggestions from comment 13 and a few more.
> 
> Review of attachment 819797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The current patch is much better than the previous iteration, though it
> still contains too much meta-comments to match our coding style, and there
> are still some unneeded functional modifications.
> 
> I made a detailed list of the changes that should be made to prepare the
> patch for another review. For the most part, they are just about removing
> unneeded code and comments, so they should be quite easy to handle.
> 

Thank you for your time and the review.

I will prepare the new patch incorporating the issues mentioned.
(It may take a while since I am quite tied up with day work this week and the next.)

Thank you again.

Comment 20

[ISHIKAWA, Chiaki]

Attachment: Patch take 5: incorporated suggestions in comment 18.

Hi,

With the exception of the javascript modeline on the 1st line, I 
modified the patch to incorporate all Paolo's suggestions in
comment 18.

BTW, during the testing of the patch, which now works as expected, and
follows Paolo's suggestion, I notice a couple of NEW PROBLEMs.

PROBLEM (1) Strange error message in error console when I left-click
on the attachment.

Every time I left-click on the attachment, error console records the
following error.

Error: [Exception... "'<error>' when calling method: [nsIContentHandler::handleContent]"  nsresult: "0x805d0001 (<unknown>)"  location: "native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0"  data: no]

0x805D0001 is, according to James Ross's error code lookup
service page, http://james-ross.co.uk/mozilla/misc/nserror
has the following meaning.

>	0x805D0001 (2153578497)
>	Name	NS_ERROR_WONT_HANDLE_CONTENT (search MXR, Google, Bing, Yahoo)
>	Module	URILOADER (24)
>	Severity	Failure (1)
>a	Number	1

PROBLEM (2) World-readable Permission of saved attachment

I am not sure if I am doing something wrong, but the attachment seems
to be saved in a world-readable manner now.  Is this intentional?  
It was saved in non-world-readable manner back in October.
In corporate setting, this is *VERY BAD*, and may help only NSA-style
snooping :-) 
I need to check why this has changed.
 
Anyway, here is a recap of the modifications to reflect Paolo's point.

[1] JavaScript mode line issue:

There is a problem here. I beg to differ from your suggestion slightly.

VIM mode line is taken verbatim from the suggested modeline, but
the first line for JavaScript is slightly different.

I put the background why this is so at the end of this post.

[2] Remove meta-comment (everything from "cf." to "time.")

done

[3] This comment from the original source is not needed anymore here, remove it.

done

[4] Remove this comment.

I feel the explanation about the return value ought to be somewhere
for future developers who may want to tinker with the code, but for now I removed
it.

[5] "Since we call "bind(this)" on the function below, you can just
use "this" inside the function, the "self" variable is
unneeded. Remove."

In the earlier reading of the code, I was not so sure about this, but
for now I have removed it and have tested it locally.
The patch now works as suggested. Thank you.

[6] "Remove meta-comment. Also the original "display error alert"
comment can now be removed because we have a clear function name."

Done. (I take that I can leave the comment starting with 
"// we can encounter an error if the directory finally chosen or".)

[7] "We should fall through to the file chooser in case of error here,
so this "else" block should be removed."

OK, I have removed it and am testing, and found the patch working.
Thank you.

[8] "This is a different case. Currently, if we can't remove the file, we just use a different name. If we want to change this, we can do that in a different bug.

Please remove the change to the "catch" block."

Now let me understand this behavior correctly.
So the intention of the download code is this.

- Suppose there is an existing file with the same name as was
  specified by the user as download target.

- TB tries to remove it.

- *IF* the removal fails, the
  code tries to create a file under a different name.

(BTW, Is there a succinct document that describes the behavior of
download manager somewhere? It would help future developers to read
such a document in advance.)

My observation of the current behavior (and the new patch follows this
behavior) is : should we not tell the user that the renaming has
happened?

The current code does not tell the user about this
automagic renaming under the hood, so to speak. I feel it should tell
the user that the renaming has happened. Otherwise the user is
perplexed to find a file aaa.txt aaa(2).txt etc. afterward.
As you suggest, I would file a Bugzilla entry once this new patch is
incorporated into the tree.

[9] "Too much indentation, remove two spaces."

Oops. Fixed. Maybe it was indented using the incorrect setting for
javascript mode (before I inserted "js-indent-level: 2".)

BTW, readers, be warned, even with the slightly modified JavaScript
mode line, the indentation by emacs may not be what we want. So avoid
global indentation using emacs's javascript mode YET before we figure
out the better javascript mode line (including other javascript mode
settings.)

[10] Remove comments here.

done

[11] "Unfortunately, re-throwing the exception here would cause the
download to hang. Remove the entire "else" block, so that we fall
through to calling saveDestinationAvailable(result) below, with result
still set to null."

Sorry, I did not realize this.

So removed the else part.
(But why would download get hang? Just curious.)

As far as TB was concerned, I didn't see a hung.
But obviously, firefox may be different since it can
run multiple simultaneous download. Oh, I have not tested the
download feature of TB's browser-like functionality. That is probably
where this bug/feature manifested with the old patch.

[12] "Remove comments after closing braces, we don't use them in our
coding style."

Done.

But I wish we do for better readability.

I don't know what editors or development environment other people use,
but it is rather awkward to figure out the nesting of series of
if/else's, big while loop's nesting, etc.  

This is personal gripe, but I bet other developers often find the deep
nesting and very large block within "{", "}" very hard to 
deal with :-(

We could improve the style in this regard, but I digress. This may be
a topic of another Bugzilla or devel-platform mailing list discussion.

[13] "throw new Components.Exception("Destination directory
non-existing or permission error", Cr.NS_ERROR_FILE_ACCESS_DENIED);

Remove both the comment and the commented code."

Done.

BTW, I noticed that short hand |Cr.| has to be fully spelled as
|Components.results.| after the testing :-)
Interesting that mozmill test suite uses |Cr| for short hand of the
component, I think.

[14] "Revert this whole code block, returning to the original createdFile assignment."

Done.

But I put a comment line to remind the future developers what might
happen. 

Below is why JavaScript mode is changed and in the form as is.

[1'] JavaScript mode line revisited.

See the discussion on "Javascript style guide. emacs mode line" on
dev-platform mailing list: 
https://groups.google.com/forum/#!topic/mozilla.dev.platform/bx3TInOrtfk
or
http://web.archiveorange.com/archive/v/gbCzhsTfK9S6hChMKz8b

Below I am quoting the relevant section from the following URL:
In https://developer.mozilla.org/en/docs/Developer_Guide/Coding_Style

--- begin quote--
Mode line

Files should have Emacs and vim mode line comments as the first two lines of the file, which should set indent-tabs-mode to nil. For new files, use this, specifying two-space indentation:

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* vim: set ts=8 sts=2 et sw=2 tw=80: */
/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Make sure to use the correct "Mode" in the first line—don't use "C++"
in JavaScript files. The exception is Python code, in which we use
four spaces for indentations.

--- end quote

Please note the first line in the last paragraph: 

       don't use "C++" in JavaScript files.

The patched file *IS* JavaScript, thus I changed the first mode line to
a suitable JavaScript modeline (for emacs, that is).

Also, without the modification of adding js-indent-level: 2", emacs
editor which I use does NOT correctly indent the
code. "js-indent-level: 2" part seems important.
(See the discussion in the mailing list postings mentioned above.)

I am using 
>emacs-version is a variable defined in `C source code'.
>Its value is "24.3.1"

What editor do you use? Emacs version prior to 24.x series may not
honour the javascript modeline very well. The relevant js mode seems
to have gone through much improvement and changes over the last 12-24
months or so.)

If there is a widely agreed upon modeline for *JavaScript* file, we
can simply insert the modeline at the 1st line's position instead of
my hack based on trial and errors.
I think I saw something about this but my memory is very hazy :-(

If anyone can suggest a proper modeline for *JAVASCRIPT* file, that
will be great.

In the new patch, I have not touched VIM mode line from the previously
suggested recommendation, BTW.



==========
TESTING of the new patch:

I tested the modification under linux 32-bit (Debian GNU/Linux).
And tested it again under linux 64-bit (Debian GNU/Linux).

To make a long story short, the patch works as expected :-)

But if the reader has the time, this is what happened with the new patch.

(I will describe the test result in the next post.)

TIA

Comment 21

[ISHIKAWA, Chiaki]

Here is what I did for testing.

First thing, first.

It used to be that double-clicking / left clicking invoked the
function in nsHelperAppDlg.js and right-clicking and choosing "Save
As" from context-menu invoked functions in C++ routine.  

The testing with Right-clicking and choosing "Save As" is not relevant
to this Bugzilla entry if this association of keys is correct and as
intended. (And it seems to be the case in the current comm-central, too.)

Only the testing using doubl-clicking / left clicking on the
attachment is reported here.

========================================
Dec 26

With the modified patch, I tested the operation under 32-bit linux and
64-bit linux.

The testing procedure is similar to the comment 10 above.

I copied comment 10 and changed the relevant parts below as
I followed the testing steps.

Below is a series of the steps to reproduce the simple error cases
under linux and show that all the PREVIOUSLY CALLED part-1, (part-2 is
no longer there) , and PREVIOUSLY CALLED part-3 and related changes
are necessary. (Previously called PART-4 is gone.)  

Preparation.

Under a user account, which is different from the test user account
(in my case |mtest2|) create a couple of test directories.

I did so as a superuser this time around.

I. Initial setup of test directories.

For step (1) below only, you can skip this and just try to save an
attachment by clicking on the e-mail attachment and try saving it to
write-protected directory such as |/dev|, and see there is no error
reported (buggy behavior).

The created directories.

/tmp/x-dir : not writable at all.
/tmp/y-dir : others can place a file but not read (tricky setup, but
             for making a directory writable, but not readable unless
             you know the exact filename, this is often done even
	     today, like for submission of answers to questionnaire,
	     etc. Or public ftp site's incoming directory often have
	     similar setup.)

Shell commands to create them 
      mkdir /tmp/x-dir
      chmod a-w /tmp/x-dir
      mkdir /tmp/y-dir
      chmod o+rwt /tmp/y-dir
      touch /tmp/y-dir/owned-by-other-file

Here is what the permission would look like when the above is done by
    a super user.

    env LANG=C ls -laR /tmp/x-dir/ /tmp/y-dir/
    /tmp/x-dir/:
    total 40
    dr-xr-sr-x  2 root root  4096 Dec 26 02:44 .
    drwsrwsrwt 32 root root 32768 Dec 26 02:58 ..

    /tmp/y-dir/:
    total 48
    drwxr-srwt  2 root   root  4096 Dec 26 02:49 .
    drwsrwsrwt 32 root   root 32768 Dec 26 02:58 ..
    -rw-r--r--  1 root   root     0 Dec 26 02:44 owned-by-other-file

II. TEST STEPS.

The following should be done using a dedicated test account, in my
case |mtest2|, which ought to be different from the developer account
(or superuser account) that was used to create test directory and
such.

Run thunderbird under this test user account.

TEST STEP (1): Try saving an attachment to a write-protected file: /tmp/x-dir

(Or you can use |/dev| as the write-protected directory as the target
of save directory.) 

WHAT SHOULD HAPPEN.: TB ought to tell the user, that the saving failed.

I skipped testing "the without the patch part" this time around.
|(WITHOUT THE PATCH, WHAT HAPPENS INSTEAD: 
| This fails silently without any error shown to the user. BAD.)

WITH THE PATCH, a proper error dialog is shown.
This error handling is done (by code in PREVIOUSLY CALLED "part-3" of) the patch.

TEST STEP (2): Try saving to /tmp/y-dir above and choose |owned-by-other-file| as
the destination file name.

During saving operation, the user gets the dialog: 
A file named "owned-by-other-file" already exists.  Do you want to replace it?

When the user sees this, answer [replace] by pushing [replace] button.

OLD patch behavior.
|WHAT SHOULD HAPPEN: TB ought to show that the saving failed since the
|test user account cannot remove the existing file under a directory
|with sticky bit set ('t'). Thus the whole saving operation fails, and TB ought
|to tell the user about it.

WITHOUT THE PATCH, WHAT HAPPENS INSTEAD: Without the patch, a file
named owned-by-other-file(1) (or with a suitable number in the
parenthesis) is created silently as the saved attachment.  I think
this is patently wrong since the user EXPLICITLY REQUESTED THE
REPLACEMENT.

WITH THE OLD PATCH, the error dialog is shown that the saving can not
proceed.  This error is handled by the code PREVIOUSLY MARKED as
"part-2", and is no longer in the new patch.

With the current patch, a file named owned-by-other-file(1) is created
silently as the saved attachment.  This behavior is what Paolo
explains as the intended behavior.

(TODO/FIXME: I will file a new Bugzilla eventually.)
However, I think this SILENT RENAMING and SAVING by TB is undesired
since the user EXPLICITLY REQUESTED THE REPLACEMENT. But this should
be handled by another Bugzilla as suggested.  Maybe we can use a
user-preference for the behavior, but I digress.

TEST STEP (3): Use of a user default download directory.

The following slightly complex case again used to produce silent
failure, and the code in the position PREVIOUSLY MARKED as "part-1"
ought to protect us from the silent failure by properly showing error
dialog.

This is the case of the user choosing the automatic downloading to a
user's default download directory, and the saving failure occurs when
the default download directory somehow becomes write-protected.

For this scenario to happen, we need a configuration change of
thunderbird using config editor of the advanced configuration in advance.

TEST STEP (3)-i: INITIAL CONFIGURATION SETUP.

In the case of thunderbird, this is how the automatic download is
enabled.

From the Menu box (upper right corner)
->  preference 
  -> advanced 
    -> general 
      -> advanced configuration (agree to the warning)
        -> config editor

The following option needs to be set to true: this entry may not exist
according to some documents. In that case you need to create one.  If
it already exists (this was my case), click the entry to toggle the
value, true <-> false.  You need to set this value to true.

  browser.download.useDownloadDir; true

What is your DEFAULT DOWNLOAD DIRECTORY?

Now where does the automatic download go?
[This may be system-dependent. You may need to tinker with Firefox to
find out where it will go. I think the choice is the same for TB under
the same OS by default.
In firefox, you can find out the default directory,
Using Edit 
      -> Preferences 
        -> General 
           -> Always Download to [which is set to |Download| by default]
 ]
In my case, i.e., Debian GNU/Linux, the default download directory is
|~/Desktop|, or |$HOME/Desktop|: that is 'Desktop' directory under one's own home
directory. I suspect this is used commonly by all the linux
distributions, but YMMV.

TEST STEP 3-(ii): Now try automatic saving to make sure it works.

Try saving a known attachment using the automatic download behavior a
couple of times.  And look for the saved attachments under the default
download directory.

Example.
I found the following files after a couple of automatic
saving of an attachments named 'Muttrc' under the default directory.

Example: automatic saving result under Debian GNU/Linux

/home/mtest2 is the home directory of the test user 'mtest2'.

Output of ls -alR /home/mtest2/Desktop AFTER the test steps:
/home/mtest2/Desktop:
total 40
drwxr-xr-x  2 mtest2 mtest2 4096 Dec 26 02:58 .
drwxr-xr-x 22 mtest2 root   4096 Dec 26 02:40 ..
-rw-------  1 mtest2 mtest2 4565 Oct 18 23:59 Muttrc
-rw-------  1 mtest2 mtest2 4565 Oct 19 00:03 Muttrc(1)
-rw-------  1 mtest2 mtest2 4565 Oct 22 00:42 Muttrc(2)
-rw-r--r--  1 mtest2 mtest2 4565 Dec 26 02:58 Muttrc(3)

Muttrc is the name of the attachment which I tried to save.

In this automatic download case, TB does not ask for the
deletion/replacement of the existing file (as was done in Step-(2)
with the previous patch, but no longer done in the current patch.).

Instead, an automatic name |Muttrc(3)| is created and used for the second
automatic download.  Now I see the filename automatic generation is
handy in such a case.

TODO/FIXME: Hmm. Saved attachment is world-readable, I am not sure if that
was intentional

TEST STEP (3)-iii: What happens when /home/mtest2/Desktop becomes write-protected?

But what happens if /home/mtest2/Desktop gets suddenly write-protected?

So to test this case, I changed user mtest2's /home/mtest2/Desktop to be
write-protected from a different terminal console window.

Command to do this is:
chmod a-w /home/mtest2/Desktop 
(this ought to be done by a super user or by mtest2 account.)

Now the automatic download (i.e, the saving of attachment in TB's
case) to the user default directory |/home/mtest2/Download| should now fail.

WHAT SHOULD HAPPEN: since the saving of the attachment to the default
directory now fails, the user should be given error dialog to learn
the error happened.

WITHOUT THE PATCH, WHAT HAPPENS INSTEAD: TB fails to save the
attachment SILENTLY. User does not receive any error feedback at
all. BAD(!)

WITH MY PATCH, yes TB fails to save the attachment, but the error
dialog is properly shown. Good.  (albeit the same text as in other
cases. It would be super if we get to see the directory name there
since the user has not selected directory immediately preceding this
step. But showing the directory is not that simple due to character
encoding issues of the directory name, etc. So I leave this as it
is. Installing the basic patch to cure this serious problem is the
first priority.)

cf.  To test the step 3 for FIREFOX, I need to tweak the setting of FF.

(1) Default download directory name.

For FF, we can change the default download directory using 
Edit 
  -> Preferences 
    -> General 
       -> (select) Always Download to [which is set to |Download| by default]

I changed this directory to |/tmp/z-dir| for easy testing of FF's
failure case. I do not want to change the permission setting of
~/Download too often.

(This configuration change seems to set the following preference to
  true automatically.  
  browser.download.useDownloadDir; true
But you may need to check the value just to be sure by means of
  |about:config| URL.

I hope the above clarifies the errors that the patch tries to address
and how to reproduce them with TB under linux (and if someone is
interested in FF in a similar manner.)

TIA

Comment 22

[:Paolo Amadini]

Thanks for the update, here are a few comments until have the time for a functional review later.

(In reply to ISHIKAWA, Chiaki from comment #20)
> >	Name	NS_ERROR_WONT_HANDLE_CONTENT
> >	Module	URILOADER (24)
> >	Severity	Failure (1)
> >	Number	1

This is probably an internal error that is now reported due to changes to which errors are reported.

> I am not sure if I am doing something wrong, but the attachment seems
> to be saved in a world-readable manner now.  Is this intentional?

Yes, the process umask can be used to restrict permissions.

> BTW, readers, be warned, even with the slightly modified JavaScript
> mode line, the indentation by emacs may not be what we want. So avoid
> global indentation using emacs's javascript mode YET before we figure
> out the better javascript mode line (including other javascript mode
> settings.)

So, for example "c-basic-offset" may be unneeded now, and new parameters may be needed.

I prefer that the mode line stays untouched for now, rather than proceeding by trial-and-error.

In fact, the recommendation of not using "Mode: C++" in JavaScript files is not widely followed. This is probably because "Mode: C++" gives better results. I suggest you ask in dev-platform, and if other Emacs users agree on a new mode line, then the style guide can be updated, and then you can update the mode line in a different bug.

Comment 23

[ISHIKAWA, Chiaki]

(In reply to :Paolo Amadini from comment #22)
> Thanks for the update, here are a few comments until have the time for a
> functional review later.
> 
Thank you for your quick review and comment.
I will wait for the fuller review later.

> (In reply to ISHIKAWA, Chiaki from comment #20)
> > >	Name	NS_ERROR_WONT_HANDLE_CONTENT
> > >	Module	URILOADER (24)
> > >	Severity	Failure (1)
> > >	Number	1
> 
> This is probably an internal error that is now reported due to changes to
> which errors are reported.

I see. If I can figure out what is causing this, I will report it, but
I doubt if I know the internal of TB and mozilla libraries well enough.
 
> > I am not sure if I am doing something wrong, but the attachment seems
> > to be saved in a world-readable manner now.  Is this intentional?
> 
> Yes, the process umask can be used to restrict permissions.
> 

It seems like so. I would prefer that the release document warns the users of this visible change.
But I am discussing this in a separate entry.

> > BTW, readers, be warned, even with the slightly modified JavaScript
> > mode line, the indentation by emacs may not be what we want. So avoid
> > global indentation using emacs's javascript mode YET before we figure
> > out the better javascript mode line (including other javascript mode
> > settings.)
> 
> So, for example "c-basic-offset" may be unneeded now, and new parameters may
> be needed.
> 
> I prefer that the mode line stays untouched for now, rather than proceeding
> by trial-and-error.
> 
> In fact, the recommendation of not using "Mode: C++" in JavaScript files is
> not widely followed. This is probably because "Mode: C++" gives better
> results. I suggest you ask in dev-platform, and if other Emacs users agree
> on a new mode line, then the style guide can be updated, and then you can
> update the mode line in a different bug.

I asked about the mode-line issue in dev-platform and
it seems there are people who prefer NOT to have mode line at all, but rather having
a separate configuration file (with much richer content if required/possible).
Considering the many options for rich control of editing mode, that is understandable.
We will have to figure out what to do once the rough consensus emerges.

TIA

Comment 24

[:Paolo Amadini]

Comment on attachment 8351457 [details] [diff] [review]
Patch take 5: incorporated suggestions in comment 18.

Review of attachment 8351457 [details] [diff] [review]:
-----------------------------------------------------------------

This patch now looks quite good to me. Three more changes before we're ready to go!

(In reply to ISHIKAWA, Chiaki from comment #23)
> We will have to figure out what to do once the rough consensus emerges.

I agree with this. In the meantime, I see some precedent for the mode line you proposed, we can keep the change in the patch for now.

http://mxr.mozilla.org/mozilla-central/search?string=Mode%3A%2BJavaScript

::: toolkit/mozapps/downloads/nsHelperAppDlg.js
@@ +246,5 @@
>              }
> +            else {
> +              // Other errors are handled generically for now.
> +              dump("ERROR: download failed. part-1-ELSE error = " + ex + "\n");
> +            }

Remove the "else" part.

@@ +251,5 @@
>            }
>  
>            // Check to make sure we have a valid directory, otherwise, prompt
>            if (result) {
> +            // This path is taken where we hae a writable default download directory.

where we hae => when we have

@@ +339,5 @@
> +            }
> +            else {
> +              // Other errors are handled generically for now.
> +              dump("ERROR: download failed. part-X-else error = " + ex + "\n");
> +            }

Remove the "else" part.

Comment 25

[ISHIKAWA, Chiaki]

Attachment: Patch take 6: incorporated suggestions in comment 24.

(In reply to :Paolo Amadini from comment #24)
> Comment on attachment 8351457 [details] [diff] [review]
> Patch take 5: incorporated suggestions in comment 18.
> 
> Review of attachment 8351457 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch now looks quite good to me. Three more changes before we're ready
> to go!
> 

Thank you for your review and comment.

> (In reply to ISHIKAWA, Chiaki from comment #23)
> > We will have to figure out what to do once the rough consensus emerges.
> 
> I agree with this. In the meantime, I see some precedent for the mode line
> you proposed, we can keep the change in the patch for now.
> 
> http://mxr.mozilla.org/mozilla-central/search?string=Mode%3A%2BJavaScript

Oh, there are so many slightly different ones :-)
I suspect many people have struggled to come up with satisfactory line.

I kept the tentative line for now.

> ::: toolkit/mozapps/downloads/nsHelperAppDlg.js
> @@ +246,5 @@
> >              }
> > +            else {
> > +              // Other errors are handled generically for now.
> > +              dump("ERROR: download failed. part-1-ELSE error = " + ex + "\n");
> > +            }
> 
> Remove the "else" part.

I removed the "else" part, but retained a one line comment to indicate that
this is where a future fix can come in (and I have something cooking in my local
queue.)
> 
> @@ +251,5 @@
> >            }
> >  
> >            // Check to make sure we have a valid directory, otherwise, prompt
> >            if (result) {
> > +            // This path is taken where we hae a writable default download directory.
> 
> where we hae => when we have

Fixed.

> @@ +339,5 @@
> > +            }
> > +            else {
> > +              // Other errors are handled generically for now.
> > +              dump("ERROR: download failed. part-X-else error = " + ex + "\n");
> > +            }
> 
> Remove the "else" part.

I removed the "else" part, but retained a one line comment.

TIA.

Chiaki Ishikawa

Comment 26

[:Paolo Amadini]

Comment on attachment 8362187 [details] [diff] [review]
Patch take 6: incorporated suggestions in comment 24.

Review of attachment 8362187 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/downloads/nsHelperAppDlg.js
@@ +244,4 @@
>                aLauncher.saveDestinationAvailable(null);
>                return;
>              }
> +            // Other errors are handled generically for now.

Hm, this does not tell much about how the error is handled.

If you really want to clarify this, don't place a comment here, but update the comment above:

"When the default download directory is write-protected, display an informative error message and stop the download.  In all other cases, prompt the user for a different target file."

@@ +327,5 @@
> +          try {
> +            result = this.validateLeafName(newDir, result.leafName, null);
> +          }
> +          catch (ex) {
> +            // This part is invoked when the target directory is write-protected, for example.

And here:

"When the chosen download directory is write-protected, display an informative error message.  In all cases, the download will be stopped."

Comment 27

[:Paolo Amadini]

Updated the summary to clarify the main behavior change that we introduced here as a side effect of the patch, in addition to displaying the message after file selection.

The new behavior is probably more informative than before.

Comment 28

[ISHIKAWA, Chiaki]

Attachment: Patch take 7.

Thank you for the review and comment.

(In reply to :Paolo Amadini from comment #26)
> Comment on attachment 8362187 [details] [diff] [review]
> Patch take 6: incorporated suggestions in comment 24.
> 
> Review of attachment 8362187 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/mozapps/downloads/nsHelperAppDlg.js
> @@ +244,4 @@
> >                aLauncher.saveDestinationAvailable(null);
> >                return;
> >              }
> > +            // Other errors are handled generically for now.
> 
> Hm, this does not tell much about how the error is handled.
> 
> If you really want to clarify this, don't place a comment here, but update
> the comment above:
> 
> "When the default download directory is write-protected, display an
> informative error message and stop the download.  In all other cases, prompt
> the user for a different target file."

I updated the comment above, but also put in a desire to
display the cause of the error for all cases properly (and stop download there 
and then.)

Actually, I am working on such a patch, but that requires major change and
that will be addressed once the patch here is incorporated in the source tree.
- I need the infrastructure to map low-level error (say, at least, common
POSIX-defined I/O errors) into a short readable ENGLISH text.
- a routine to use the text to report the error.

 
> @@ +327,5 @@
> > +          try {
> > +            result = this.validateLeafName(newDir, result.leafName, null);
> > +          }
> > +          catch (ex) {
> > +            // This part is invoked when the target directory is write-protected, for example.
> 
> And here:
> 
> "When the chosen download directory is write-protected, display an
> informative error message.  In all cases, the download will be stopped."

Here, also, I changed the comment, but added the desire to 
report the cause in all cases (and this too will be handled in another bugzilla.)

TIA

Comment 29

[ISHIKAWA, Chiaki]

(In reply to :Paolo Amadini from comment #27)
> Updated the summary to clarify the main behavior change that we introduced
> here as a side effect of the patch, in addition to displaying the message
> after file selection.
> 
> The new behavior is probably more informative than before.

I am afraid that the new name does not carry the urgency of the original (silent failure and dataloss),
and so changed the last part somewhat.

From: "instead of falling back to file selection"
To:   "instead of failing silently"

Comment 30

[:Paolo Amadini]

Comment on attachment 8363029 [details] [diff] [review]
Patch take 7.

Review of attachment 8363029 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to ISHIKAWA, Chiaki from comment #29)
> I am afraid that the new name does not carry the urgency of the original
> (silent failure and dataloss),and so changed the last part somewhat.

Sounds good. This restricts the scope of this bug, and I think it's a good thing, so let's go for the change that fixes only this issue.

The current patch is actually changing two things:

1. If the preferred (configured) download directory is write-protected, previously we let the user select another destination, now we show an error message and stop the download.
2. If the manually selected download directory is write-protected, previously we stopped the download with no message box, now we show a message box and then stop the download.

::: toolkit/mozapps/downloads/nsHelperAppDlg.js
@@ +240,5 @@
>            catch (ex) {
> +
> +            // When the default download directory is write-protected,
> +            // display an informative error message and stop the
> +            // download.

Let's keep case (1) unchanged and focus on case (2), that is what you're interested in fixing. To do that, the "catch" block here should contain no code. The old code was never actually executed, since validateLeafName used to return null instead of throwing an exception.

Replace the code in the catch block with the comment "When the default download directory is write-protected, prompt the user for a different target file."

Comment 31

[:Paolo Amadini]

(In reply to ISHIKAWA, Chiaki from comment #28)
> I updated the comment above, but also put in a desire to
> display the cause of the error for all cases properly (and stop download
> there and then.)
> 
> Actually, I am working on such a patch, but that requires major change and
> that will be addressed once the patch here is incorporated in the source
> tree.
> - I need the infrastructure to map low-level error (say, at least, common
> POSIX-defined I/O errors) into a short readable ENGLISH text.
> - a routine to use the text to report the error.

I'm pretty sure that, for Firefox, we don't want to map errors to generic strings in the back-end, but focus on informative and actionable UI for common cases.

Since you're proposing this change mainly for the benefit of Thunderbird, that handles downloads differently, you may also want to ask the opinion of someone who works on comm-central code.

Comment 32

[ISHIKAWA, Chiaki]

(In reply to :Paolo Amadini from comment #31)
> (In reply to ISHIKAWA, Chiaki from comment #28)
> > I updated the comment above, but also put in a desire to
> > display the cause of the error for all cases properly (and stop download
> > there and then.)
> > 
> > Actually, I am working on such a patch, but that requires major change and
> > that will be addressed once the patch here is incorporated in the source
> > tree.
> > - I need the infrastructure to map low-level error (say, at least, common
> > POSIX-defined I/O errors) into a short readable ENGLISH text.
> > - a routine to use the text to report the error.
> 
> I'm pretty sure that, for Firefox, we don't want to map errors to generic
> strings in the back-end, but focus on informative and actionable UI for
> common cases.
> 
> Since you're proposing this change mainly for the benefit of Thunderbird,
> that handles downloads differently, you may also want to ask the opinion of
> someone who works on comm-central code.

Thank you for the comment.

For the next patch I am producing, I will certainly ask the opinion of TB users and developers.
Obviously, I would agree with the way the download works for the browser now.
But strangely, somehow I find the behavior a little odd for TB usage. 
Funny, and I thought about it for a while, and now I think it is because mail client users
want to manage attachment filenames under their control (coming
from close friends, business partners, and such) more than browser users in general.
While downloading files from third party web sites, people don't care much about the filename of the download and silent clever renaming when a name clash, etc. occurs is a good thing
as long as people can click on [download] button and goes to the folder where the download took place.(I certainly don't care if symantec anti virus install is called NAV-2013.exe or NAV-2013.msi, etc.)  
Problem is that TB doesn't have such [download] or [save] button that can show you the
history of saving attachments in the past and so we are in trouble if silent renaming takes place(!).

But I digress.

I will re-check the behavior of the proposed patch after the removal of else clause etc. to make sure
that it acts as is explained in the new comments.
Then I will upload the final patch.

Thank you again for your review and patience.

Comment 33

[ISHIKAWA, Chiaki]

> For the next patch I am producing,

Clarification:
I meant this to refer to the new different bugzilla to change the behavior
somewhat, and not the basic change I am proposing here.

I will restrict the patch here for a limited scope only to produce proper error warning 
instead of failing silently.

Comment 34

[ISHIKAWA, Chiaki]

Attachment: Patch Take 8.

Thank you for the review and the suggestion for final comment.

As you can see below, the patch works as your suggested final comment
indicates, and so code-wise it is OK now!

Before the main post, I would like to report that I was pleasantly
surprised that the current version of TB now has [saved file] menu
item that can show the history of saved attachment (used file name,
which may be the automatically renamed file, and folder
info). Good!  (The information for the current session 
seems to be purged once TB is shutdown, though,
and I would rather see [saved *attachments*] menu since it is more
appropriate as TB terminology.)

But I didn't know this menu item existed until today when I noticed it
the comm-central version and ascertained it is also in the current
version 24.2 under Windows.  One major problem with it is that it is
rather well hidden, so to speak :-)

To reach it, and show the saved history (and look at the information
is three mouse clicks (and very long pointer movement, too bad) as
opposed to the case of Firefox where reaching it only a couple of
mouse clicks (and very short pointer movement, easy!).  TB needs the
user to move pointer, "AppMenu -> hunt for Tools in the new menu pane
(well into the lower-right) -> hunt for [saved files] in the pop up menu pane" as opposed to Firefox's
click on the Download button on the visible menu bar ONCE (for the
history of the current session), and another additional click on the
SAME SPOT to see the whole history including preceding sessions.).

Anyway, the added function to look at the save history may alleviate
some attachment usability issues AFTER menu layout is reconsidered.
Some of the changes I thought would be necessary (the topic for a new
bugzilla entry to follow the patch in this bugzilla entry) are
motivated by the lack of the history-of-saved-attachement.

Back to main point in this bug.

Thank you again for the review and careful note
about the behavioral changes.  I did not realize (or have forgotten)
that my patch (produced over many months) changed the download
behavior ever so slightly, which was not explained well in my original
post here. 
Actually when I re-read the posts, both of the the behavioral changes were what I wanted to modify, but I DO agree that one of them is better handled in
another bugzilla entry (the case of write-protected default save directory).
Especially in view of the new saved-attachment-history and the
wider recognition of that Firefox, the browser, and Thunderbird, the mail client
may have slightly different needs for handling "downloading a file" and "saving an attachment".

The main point:
I rechecked the operation of the final changed patch (uploaded) in the
case of using default save directory being write-protected.

I found the operation is OK by the following test steps.
So, the patch is ready for check-in code-wise, I hope.

Test steps.

0. Prerequisites:
    Set "Don't ask the directory when saving." save mode.

    This is how to do it for those who want to repeat and verify my test.
    AppMenu ->  Preference -> Attachment -> Incoming	
    	    Save files to "~/Desktop"

    cf. "~/Desktop" somehow is the default save directory for some reason.
    Maybe a misconfigured setup from my linux installation. But
    any directory would do.

[1-a] Temporarily write-protect ~/Desktop

    E.g.  
    mtest2@vm-debian-amd64:~$ ls -ald ~/Desktop
    drwxr-xr-x 2 mtest2 mtest2 4096 Dec 26 04:52 /home/mtest2/Desktop
    mtest2@vm-debian-amd64:~$ chmod a-w ~/Desktop
    mtest2@vm-debian-amd64:~$ ls -ald ~/Desktop
    dr-xr-xr-x 2 mtest2 mtest2 4096 Dec 26 04:52 /home/mtest2/Desktop

[2] Case of trying to save an attachment to this write-protected default location.

    What to do:
    DOUBLE CLICK on the attachment.

    To the prompt:
    What should daily do with the file?
    Open
    Save ...

     MAKE SURE Save is selected and then HIT [OK]

[2-a] Behavior: when there is no Pre-existing saved file (the same
      name) in the Default save directory.

   EXPECTED: new directory location and file name is asked for.

   OK!

   WHAT HAPPENED.

   The final patched version (patch uploaded) asks for the
   file name (but without mentioning why.) 
   And (when (Save) [OK] is hit,) it starts a file chooser/selector
   with this write-protected "~/Desktop" directory (hmm!).
   
   My comment: OK, but ...

   [To be discussed in a future bugzilla, maybe a different entry. But
   I explain it what lies ahead.]

I think TB ought to display, instead, the save to Default save
location "~/Desktop" would fail and thus TB is asking the user for a
new cation to save. It does not today.  (BTW, the user may be unaware
due to the lack of warning/error, she/he may choose the default
directory that is shown.  Then saving will surely fail with Error
dialog (good!)  because of the write-protection.)

[2-b] The case when there is a file with the same name already
   
   EXPECTED: new directory location and file name is asked.

   OK!

   WHAT HAPPENED: 
   Again, the save could not be done, so a new file name is asked for
   by TB by showing the file chooser/selector with the write-protected
   "~/Desktop" is given.

Extra test:

[3 ] I just confirmed the existing behavior of using Default directory
   is intact.

When default save directory can be written, 
the saving is done automatically once we hit SAVE button
- WITHOUT asking the directory location,  AND 
- WITHOUT asking for the removal of existing file if any 
  (by the renaming of the saved attachment to a new file name.)

My comment: OK, but ..
[To be discussed in a future bugzilla, maybe a different entry. But I
explain it to show what lies ahead.]
This behavior may be inappropriate for TB and will be discussed in
future bugzilla entry, maybe a different one, and maybe also in a
Newsgroup during the discussion.  Why? This behavior may be
controversial for TB, a mail client.  I think the mail client user may
want to have more user control over the names of the attachment.

So, IMHO, TB should explicitly tell that there is a conflicting file
with the same name, and then ask for a directory to save with a file
chooser/selector so that the user can change the name as he/she
wishes.)  But I digress. This would be discussed in a new bugzilla.

Now, as for the comment part, I left the comment from the previous
patch inside #if 0, #endif so that the next bugzilla entry (very likely a
different one), which I will create once this patch goes in, will find
it easy to refer to the place where the modification should be
made. What do you make of it?

In any case, this data-loss issue is serious enough to warrant the
backport of the patch to the current release even.

I wonder if someone can create the patch for the current version and
test it through TryServer. My current local comm-central source has
about three dozen patches and difficult to clean up for try server run, 
and the file space is always tight :-)

TIA

Comment 35

[:Paolo Amadini]

Comment on attachment 8370090 [details] [diff] [review]
Patch Take 8.

Review of attachment 8370090 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/downloads/nsHelperAppDlg.js
@@ +241,5 @@
> +
> +#if 1
> +            // When the default download directory is write-protected,
> +            // prompt the user for a different target file."
> +#else

Just remove the #if and the code in the #else part, thanks!

Comment 36

[ISHIKAWA, Chiaki]

Attachment: Patch (take 9). Tidied up comment.

OK, I removed the comment.

TIA

Comment 37

[:Paolo Amadini]

Comment on attachment 8370530 [details] [diff] [review]
Patch (take 9). Tidied up comment.

Thanks! The testing in comment 34 looks good, will also do some more testing locally before landing this patch. Here is a tryserver build in the meantime:

https://tbpl.mozilla.org/?tree=Try&rev=9361b2c73915

Comment 38

[ISHIKAWA, Chiaki]

(In reply to :Paolo Amadini from comment #37)
> Comment on attachment 8370530 [details] [diff] [review]
> Patch (take 9). Tidied up comment.
> 
> Thanks! The testing in comment 34 looks good, will also do some more testing
> locally before landing this patch. Here is a tryserver build in the meantime:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=9361b2c73915

The tryserver run looks good(!) [Yes, I have been testing the patch locally, but with so many other patches, tryserver test does not hurt.]

TIA

Comment 39

[Magnus Melin [:mkmelin]]

:Paulo, C(In reply to :Paolo Amadini from comment #37)
> Comment on attachment 8370530 [details] [diff] [review]
> Patch (take 9). Tidied up comment.
> 
> Thanks! The testing in comment 34 looks good, will also do some more testing
> locally before landing this patch. Here is a tryserver build in the meantime:

This should land now, right?

Comment 40

[:Paolo Amadini]

(In reply to Magnus Melin from comment #39)
> :Paulo, C(In reply to :Paolo Amadini from comment #37)
> > Comment on attachment 8370530 [details] [diff] [review]
> > Patch (take 9). Tidied up comment.
> > 
> > Thanks! The testing in comment 34 looks good, will also do some more testing
> > locally before landing this patch. Here is a tryserver build in the meantime:
> 
> This should land now, right?

I did some testing in Firefox and determined that downloading to an inaccessible default folder still fails without a prompt (the behavior is unchanged), probably because there is an earlier failure in the Firefox code.

However, given that this fixes the behavior in Thunderbird according to comment 34, I'll land this in mozilla-inbound when the tree is open.

Comment 41

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d3b0ec3c6af

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d3b0ec3c6af

Comment 43

[ISHIKAWA, Chiaki]

(In reply to :Paolo Amadini from comment #40)
> (In reply to Magnus Melin from comment #39)
> > :Paulo, C(In reply to :Paolo Amadini from comment #37)
> > > Comment on attachment 8370530 [details] [diff] [review]
> > > Patch (take 9). Tidied up comment.
> > > 
> > > Thanks! The testing in comment 34 looks good, will also do some more testing
> > > locally before landing this patch. Here is a tryserver build in the meantime:
> > 
> > This should land now, right?
> 
> I did some testing in Firefox and determined that downloading to an
> inaccessible default folder still fails without a prompt (the behavior is
> unchanged), probably because there is an earlier failure in the Firefox code.
> 
> However, given that this fixes the behavior in Thunderbird according to
> comment 34, I'll land this in mozilla-inbound when the tree is open.

Thank you.

I was down with Type B flu and so could not test Firefox very well. (It is hard to believe that
FF doesn't handle this right, but as you say there may be an early issue in the execution path. 
As far as TB goes, I am quite sure this fixes the major issue.

Now that this landed, I will upload a couple of bugzilla entries to discuss the slight change of the behavior, etc. that is taken out from this bugzilla and a fuller error reporting (based on lower error code such as POSIX-defined error code).

Thank you again.

Comment 44

[ISHIKAWA, Chiaki]

What does keyword "verifyme" mean?

TIA

Comment 45

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

"verifyme" puts the fix on QA's radar for verification. It means that someone should verify the fix and confirm it works fine in the latest Firefox 30 build. That means the Reporter, a member of the QA team, or anyone else who has reproduced this problem before.

Comment 46

[Andrei Vaida [:avaida]]

(In reply to :Paolo Amadini from comment #40)
> I did some testing in Firefox and determined that downloading to an
> inaccessible default folder still fails without a prompt (the behavior is
> unchanged), probably because there is an earlier failure in the Firefox code.

I can also confirm that this issue is still reproducible on Ubuntu 13.10 64-bit, using the STR from Comment 9 with:
 * Firefox 30 Beta 8 (Build ID: 20140527133511) [1],
 * Aurora 31.0a2 (2014-05-28) [2],
 * Nightly 32.0a1 (2014-05-28) [3].

[1] Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0
[2] Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0
[3] Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0

Comment 47

[ISHIKAWA, Chiaki]

(In reply to Andrei Vaida, QA [:avaida] from comment #46)
> (In reply to :Paolo Amadini from comment #40)
> > I did some testing in Firefox and determined that downloading to an
> > inaccessible default folder still fails without a prompt (the behavior is
> > unchanged), probably because there is an earlier failure in the Firefox code.
> 
> I can also confirm that this issue is still reproducible on Ubuntu 13.10
> 64-bit, using the STR from Comment 9 with:
>  * Firefox 30 Beta 8 (Build ID: 20140527133511) [1],
>  * Aurora 31.0a2 (2014-05-28) [2],
>  * Nightly 32.0a1 (2014-05-28) [3].

Thank you for the report. It seems then FF uses the download code in somewhat different manner 
from the way TB uses it.
Let me investigate a little further.

Can you tell us a little bit about the destination?
 - Is the destination directory part of a remote-mount file system (such as NFS, CIFS, etc.),
 - Is the destination directory a write-protected directory in an read/write-enabled file system, or
 - is the whole file system where the directory resides mounted read-only?

I have been planning to add handling for OTHER I/O errors 
that may be encountered aside from the permission error 
that is explicitly handled now.

I have noticed that the I/O error generated by remotely-mounted file system may
not be simple local disk I/O errors, but could be EAGAIN, EHOSTUNREACH, etc.
They may not be handled well by the current code.
Thus the questions above.

I am interested in error codes defined by POSIX.
I wonder if during the effort to catch these errors, I may be able to
find the uncovered loophole where the file I/O error is not caught properly and fill it.

Cf. Error codes:

I am talking about error code returned into errno by I/O functions.

http://pubs.opengroup.org/onlinepubs/000095399/basedefs/errno.h.html

Some notable codes relevant to File I/O from the above list: (not exhaustive) 

[EACCES]    Permission denied.
  <--- This one has been handled (although FF may have an issue).

[EROFS] Read-only file system. 
   <--- should have been captured by turning EROFS to EACCESS. But let me investigate this.
   I asked about the read-only file system because of this error code.

[EAGAIN]  Resource unavailable, try again (may be the same value as [EWOULDBLOCK]).
   <--- This error code may be generated when a remote-mounted file system may
   have an issue due to network outaage, or something.

[EBUSY]    Device or resource busy.
  --- could happen on a hung device :-(

[EFBIG] File too large.
  --- may happen. Don&t know exactly how this error is thrown to the download code.

[EHOSTUNREACH] Host is unreachable.
  --- This code is for real. I have verified that CIFS-mount fails with this error code
      temporarily due to network issue between WIN7 server and linux client. I don't know exactly
      how this ends up being thrown to the download code in JavaScript.

[EINTR] Interrupted function.
  --- This could be returned when a remotely mounted file system
      allows interrupt.

[ELOOP] Too many levels of symbolic links.
  --- possible under POSIX-like system where symlinks are misconfigured.

[ENOSPC] No space left on device.
  --- this has bitten many users of TB before. I am not sure how this error
      can be thrown back to the download code.

The codes below may be generated, but I am not sure how they are propagated to
File I/O code of mozilla.

[EMFILE] Too many open files.
[EMLINK] Too many links.
[ENAMETOOLONG] Filename too long.
[ENETDOWN] Network is down.
[ENFILE] Too many files open in system.

At least one error, permission error is explicitly handled in the download code, but
others may have been masked as a generic error in the lower-level I/O code.
So let me investigate this further.

TIA

Comment 48

[:Paolo Amadini]

This platform bug was filed specifically because of the download behavior in Thunderbird, and I expect this to be fixed there, as noted in comment 40.

Moreover, I can't reproduce the issue on a recent Nightly, although maybe I'm not following the steps to reproduce in comment 9 correctly. Andrei, if you have noticed a behavior that you believe is a bug, can you please file a separate bug specifically for Firefox, with detailed steps to reproduce?

Comment 49

[:Paolo Amadini]

Also, please note that the download failing without a blocking prompt is not considered a bug in Firefox, because we're moving towards a model where errors can be viewed asynchronously in the Downloads Panel, which may be different from what Thunderbird expects. If the download is erroneously indicated as succeeded, or does not appear in the panel, that is considered a bug.

Comment 50

[ISHIKAWA, Chiaki]

(In reply to :Paolo Amadini from comment #48)
> This platform bug was filed specifically because of the download behavior in
> Thunderbird, and I expect this to be fixed there, as noted in comment 40.
> 
> Moreover, I can't reproduce the issue on a recent Nightly, although maybe
> I'm not following the steps to reproduce in comment 9 correctly. Andrei, if
> you have noticed a behavior that you believe is a bug, can you please file a
> separate bug specifically for Firefox, with detailed steps to reproduce?

(In reply to:Paolo Amadini from comment #49)
> Also, please note that the download failing without a blocking prompt is not
> considered a bug in Firefox, because we're moving towards a model where
> errors can be viewed asynchronously in the Downloads Panel, which may be
> different from what Thunderbird expects. If the download is erroneously
> indicated as succeeded, or does not appear in the panel, that is considered
> a bug.


OK, thank you for the clarification and also thank you for the latest development info in comment 49.

Anyway, I will work on the work for other error codes in different bugzilla entry.

TIA

Comment 51

[Andrei Vaida [:avaida]]

(In reply to :Paolo Amadini from comment #49)
> Also, please note that the download failing without a blocking prompt is not
> considered a bug in Firefox, because we're moving towards a model where
> errors can be viewed asynchronously in the Downloads Panel, which may be
> different from what Thunderbird expects. If the download is erroneously
> indicated as succeeded, or does not appear in the panel, that is considered
> a bug.
Paolo, the behavior I noticed was identical to what you described in Comment 49, I wasn't sure if it was expected at that point, so I think it's safe to say that this is not an issue.

(In reply to ISHIKAWA, Chiaki from Comment 47)
> Thank you for the report. It seems then FF uses the download code in somewhat different manner 
> from the way TB uses it.
> Let me investigate a little further.
> 
> Can you tell us a little bit about the destination?
>  - Is the destination directory part of a remote-mount file system (such as NFS, CIFS, etc.),
>  - Is the destination directory a write-protected directory in an read/write-enabled file system, or
>  - is the whole file system where the directory resides mounted read-only?
The directory I used to test this was the default Downloads folder (write-protected) on a read&write-enabled file system. For clarity, my only intention here was to confirm Comment 40, but if there's anything else I can help you with in terms of Fx behavior, please let me know.

Comment 52

[Philip Chee]

FYI Firefox is using the new jsdownloads download manager. As far as I know Thunderbird is still using the old C++ based download manager. That might explain the difference.

Comment 53

[ISHIKAWA, Chiaki]

(In reply to Philip Chee from comment #52)
> FYI Firefox is using the new jsdownloads download manager. As far as I know
> Thunderbird is still using the old C++ based download manager. That might
> explain the difference.

Thank you for the tips. (Other engagements have kept me busy and I could not follow this up quickly.) I will keep this in mind when I investigate the issue.

TIA