567585 - TB3 fails to raise an error when it tries to save an attachment to write-protected directory.

Status: RESOLVED DUPLICATE of bug 928250

(Toolkit :: Downloads API, defect)

Description

[ISHIKAWA, Chiaki]

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]

Correction:

I meant to say
case (a)  OK
     case (b)  NG
     case (c)  NG
     case (d)  NG
     case (e)  OK

Comment 2

[ISHIKAWA, Chiaki]

Ouch, incorrect posting. I meant to say
case (a)  OK
     case (b)  OK   (<= not NG)
     case (c)  NG
     case (d)  NG
     case (e)  OK

Comment 3

[:Paolo Amadini]

A condition similar to case (c) also happens with Firefox on Windows when
automatically saving an attachment to a folder for which the current user
does not have write permissions. I believe it is the same failure.

Comment 4

[WADA:World Anti-bad-Duping Agency]

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

Save location choice is affected by next settings.
Tools/Options/Attachments
  [ ] Save files to xxx
      browser.download.useDownloadDir=true
  [ ] Always ask me where to save file  
      browser.download.useDownloadDir=false
And, directory choice is affected by next settings.
> browser.download.folderList
> browser.download.dir
> browser.download.downloadDir
> messenger.save.dir <= last used directory by Tb for save mail or attachmet
> mail.compose.attach.dir <= last used directory for attachment choice
Used prefs.js entries by Tb is slightly different from browser. See next documents, for related settings.
> http://mdn.beonex.com/en/Download_Manager_preferences
> http://kb.mozillazine.org/Unable_to_save_or_download_files
Please be careful for these settings in your test.

FYI.
Tb3's behavior on Win with next settings.
> browser.download.useDownloadDir=false
> browser.download.folderList=0 (default)
Folder displayed at folder picker dialog
(1) messenger.save.dir=existent => this folder
(2) messenger.save.dir=non-existent => MRU(Most Recently Used) directory
(3) messenger.save.dir=Read permission only directory
    => The Read permission only directory at folder picker dialog
    Try to save in it.
    => Warning of no write permission, and
       asked to save directory specified by browser.download.folderList or not.
    Document says "Desktop", but offrred Desktop\My documents in my test.

Comment 5

[ISHIKAWA, Chiaki]

(In reply to comment #4)

Thank you, WADA-san, for find details of
default directory choices.

While I am reading the comment, I realize that
the bug I noticed was for real.

Two different paths used by TB3 to save 
attachment (one path invoked by left-button clicking, and the
other by right-button clicking) use DIFFERENT
default: the left-button uses "BROWSER" default even in TB3 (!).

See 
https://bugzilla.mozilla.org/show_bug.cgi?id=549719#c17

TIA

Comment 6

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #5)
> Two different paths used by TB3 to save attachment
> (one path invoked by left-button clicking, and the other by right-button clicking)

I could see it too on Win. My previous result was my usual "context menu/save as" case. If double click/save file, Desktop(browser.download.folderList=0) was proposed with any messenger.save.dir value. It seems the reason why I sometimes saw "Desktop" as preselected directory at folder picker dialog even though I always set browser.download.useDownloadDir=false in my ordinal testing.

Comment 7

[ISHIKAWA, Chiaki]

(In reply to comment #6)

Thank you, Wada-san, for verifying the problem.

The more people know the problem, the higher the chance it gets fixed, I hope.
I will leave that particular problem in Bug 549719 for now.

Anyway, I am posting a preliminary patch to fix the problems initially posted to this bugzilla myself, i.e., a patch nsHelperAppDlg.js to make TB3 friendly
to the case of trying to save attachment to write-protected directory.

But this patch assumes that a previous patch (indentation patch to the file is already applied.) So I am posting it this time just for reference purposes.

Once the indentation patch is accepted, I will re-post appropriate patch (including any change I may have to make to accommodate the latest indentation changes, etc.).

TIA

Comment 8

[ISHIKAWA, Chiaki]

Attachment: RFC (patch in the works) not in the proper patch form yet.

A patch in the works.
Not in the final patch form.

Produced by "diff -U 8" against the newly indented nsHelperAppDlg.js
(See  Bug 559833  ) which is not yet in the official repository.

So this may not be applied to the current nsHelperAppDlg.js without at least
one manual intervention for a hunk.
 
Also, there is a code for debugging near the beginning to print the
directory attribute to javascript console. (inside #if 1, #endif)

Thus not in the final patch form yet.

But this could be helpful for those who got bitten by the bug, or
developers to verify that the patch in the works fixes the said issues.

Comment 9

[ISHIKAWA, Chiaki]

My attempt to create the working patch to the latest cleanly indented version of nsHelperAppDlg.js has failed.

The old patch I posted worked. But lately in the last couple of weeks
I don't see it work any more.

See my posting  to mozilla.dev.tech.xpcom
http://groups.google.co.jp/group/mozilla.dev.tech.xpcom/browse_thread/thread/40fbbee39bd3d8c7

After a search for the clue, I posted a message to Bug 249143 which seems to be related to the bug now I see.

Until I find the way to call alert dialog or find the replacement, showing the
alert that the writing of attachment fails due to the write-protection of the directory may be impossible.

One alternative is to show such a dialog outside nsHelperAppDlg.js, maybe after the call to promptForSaveToFile in nsExternalHelperAppService.cpp

http://mxr.mozilla.org/comm-central/source/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp#2026

But that will require an addition of clearly defined return paramter to show that
the writing failed (and the reason of the failure).

I wonder if someone can suggest a way out.

TIA

Comment 10

[ISHIKAWA, Chiaki]

(In reply to comment #9)
> My attempt to create the working patch to the latest cleanly indented version
> of nsHelperAppDlg.js has failed.
> 
> The old patch I posted worked. But lately in the last couple of weeks
> I don't see it work any more.
> 
  [...]

Voila. A good news.

The patch works again and it can properly
show the dialog when the saving of the attachment fails
due to the missing write permission, or when
the target directory is removed when TB3 is about to save to it.

Details: 
For the last one and half months or so, I could not even compile TB3 due to 
the XPCOM reshuffle and then a strange media `raw' related problem.

But somehow the first one is over, and I put a bandage on the second problem.
Then I could compile TB3 again.
But the problem still persists in the unmodified 3.1a1pre.

I noticed that the official nsHelperAppDlg.js had a few changes and so
I incorporated the change to my patch and tested my patch again in this
new 3.1a1pre from comm-central.

Then to my delight, I found that the patch works again!

I am uploading the patch to nsHelperAppDlg.js so that
someone who wants to test it can do so on his/her computer.
Just store the modified nsHelperAppDlg.js on your mozilla's bin/components
directory if you are using the comm-central version (I think it is called nightly?). Better still, compile your version by replacing the
nsHelperAppDlg.js with the modified version :-)

Anyway a great progress for many affected users including me.

(The right-clicking case still shows the error dialog TWICE, but
at least we get warned. Before, there was total silence even though the
saving failed.)

I found out nsHelperAppDlg.js was slightly changed in the official

Comment 11

[ISHIKAWA, Chiaki]

Attachment: A first cut to the

(The last statement in the main comment 10 was cut short.
"I found out nsHelperAppDlg.js was slightly changed in the official
distribution and the changes were reflected in the patch." was what I meant to write.)

The uploaded patch is for evaluation purposes-only and not meant for
the immediate inclusion due to some additional error messages, which can
be disabled by #if 0, #endif easily, though.

 - I am using my own mydump() function for dumping purposes.
   The message can be seen in Error Console. (Tools -> Error Console)
   This actually is better using dump() as in the original, but
   other developers may disagree.

 - I am printing debugging messages using mydump() inside isUsableDirectory().
   The message is very, very informative, but should be removed eventually.
   (You can see immediately, for example, what default saving directory is used.)

 - function auxpromptForSaveToFile() is a function
   Basically, does the last half of the original promptForSaveToFile function.

   The creation of this function was necessitated since the original code
   never let the user retry saving after the silent failure of saving an attachment.
   (The original code that was moved into this function is
   right now commented out by #if 0, #endif)

 - The above auxpromptForSaveToFile() is called again 
   after saving to a file failed and a dialog is shown.

 - A renaming of a variable in function validateLeafName()
   aLocalFile -> aLocalFolder.
   This is more apt as far as the use in the first part of the function is concerned.
   Later, though, it also holds the leaf function name. Hmm...


 - A few comments to clarify or pose a question explicitly about the
   nature of return value. I didn't try to fix, but exposed the potent
   problem if any.
 

 - Oh, and a sprinkling of 
   "// TODO/FIXME: empty catch-clause. VERY BAD CODING PRACTICE."
   where empty catch-clause is seen.
   This is indeed very bad. A syntax error in the lower level function etc.
   is thrown as java run time error but such empty
   clause hides the existence of such syntax error during debugging. It hurts.


- I reflected the latest changes to nsHelperAppDlg.js
  Web progress listener change.
  XPCOM registration change.

  I am not sure what make my patch work again after early June failure.
  python preprocessor change (and the subsequent affected code change(s)?)
  and other minor changes noticed during XPCOM transition?

  So I have a nagging feeling something may go wrong again.
  (But the patch worked close to 10 months or so before it stopped working.)

  So posting this crude patch early may help some of you, developers,
  to test it out. And if this stops working again, maybe those
  developers can also complain -:)

Comment 12

[ISHIKAWA, Chiaki]

Sorry typos. 

The version numbers ought to be 3.2a1pre

> But the problem still persists in the unmodified 3.1a1pre.
                                               *****************
> 
> I noticed that the official nsHelperAppDlg.js had a few changes and so
> I incorporated the change to my patch and tested my patch again in this
> new 3.1a1pre from comm-central.
**********************

Comment 13

[:Paolo Amadini]

I think I'll be able to try this updated patch to see if it solves the
bug on Firefox trunk too, but not before two or three weeks, I suppose.

Comment 14

[ISHIKAWA, Chiaki]

(In reply to comment #13)
> I think I'll be able to try this updated patch to see if it solves the
> bug on Firefox trunk too, but not before two or three weeks, I suppose.

Thank you, Paulo. 

In the meantime, what is the correct procedure to try this on trunk? (The development version I tinker with, namely comm-central seems to be what is called nightly version.) Maybe I can also try the trunk build with the patch myself. Is simply obtaining the source tar ball and patch it the way to go?

Comment 15

[ISHIKAWA, Chiaki]

(In reply to comment #14)
> (In reply to comment #13)
> > I think I'll be able to try this updated patch to see if it solves the
> > bug on Firefox trunk too, but not before two or three weeks, I suppose.
> 
> Thank you, Paulo. 
> 
  [ ... ]

>  Is simply obtaining the source tar ball and patch it the way to go?

Paulo,

I did just that. And I found the patch to nsHelperAppDlg.js.in seems to
produce a workable nsHelperAppDlg.js 
(The first time I compiled the unmodified trunk tree, nsHelperAppDlg.js
is a real file in MOZ_OBJDIR/mozilla/dist/bin/components/nsHelperAppDlg.js.
After I modified nsHelperAppDlg.js.in and re-compiled, nsHelperAppDlg.js is now
a link to a file that is produced in SRCDIR/mozilla/toolkit/mozapps/downloads/nsHelperAppDlg.js. How interesting.)

I am now uploading the crude diff for evaluation.
No consistent indentation, or the use of the function you produced to
reduce duplication several months ago, etc.
But the modified version certainly produced warning when saving the attachment
failed due to permission error.

Please try.

Comment 16

[ISHIKAWA, Chiaki]

Attachment: (TRUNK version) diff to nsHelperAppDlg.js (evaluation purposes)

To evaluate the effectiveness of the proposed patch (comm-central, nightly)
forthe current TRUNK version 3.1.2, I created a diff with some
debug statement, etc.
It works as far as I can tell. Please try.

Comment 17

[ISHIKAWA, Chiaki]

Attachment: Cleaned up patch candidate for nsHelperAppDlg.js

I am uploading a slightly cleaned up patch candidate for review (comm-central version.)
 - Took out one "#ifdef DEBUG, #endif" section for debugging,
 - took out large a section inside "#if 0, #endif", which is moved
   in an earlier position in the file and made into a separate function.

I hope this can be reviewed soon. I wondered who should be the reviewer.
In a separate bugzilla entry, I put Shawn as the final reviewer (bug https://bugzilla.mozilla.org/show_bug.cgi?id=559833
) so I am doing the same here. Please advise this is not the case.

TIA

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 470103 [details] [diff] [review]
Cleaned up patch candidate for nsHelperAppDlg.js

I'll still need to look at this, but Paolo is likely to find most, if not all of the issues I'll find and I bet he has a lot more review bandwidth.

Comment 19

[:Paolo Amadini]

Comment on attachment 470103 [details] [diff] [review]
Cleaned up patch candidate for nsHelperAppDlg.js

Hello Chiaki, thanks for your contribution! In the following weeks I'll be able to review it and help you to turn this into a final patch that can be checked in. I'll work on the "mozilla-central" tree for the purpose of testing and reviewing.

There is still a lot of work to do, however. Most importantly, nearly every fix needs an automated test case, that helps avoiding regressions caused by future modifications of the code.

In fact, I can't reproduce the bug on Windows since the file picker dialog doesn't allow me to save to a write-protected directory in the first place. An automated test could instead provide a "mock" implementation of the file picker that returns a "bad" path regardless of the operating system, without showing any user interface at all.

Your automated test case should probably be a "chrome" test
(https://developer.mozilla.org/en/Mozilla_automated_testing#Chrome_tests),
and it should do the following operations:

 * Create folders with appropriate permissions in a temporary directory.
 * Set the preferences to always ask where to save the files.
 * Open the dialog that asks what to do with a downloaded file.

    You can see how this test does it:
     /toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_dialog_layout.xul

 * Replace the file picker implementation with a mock one, that returns the
    file path in the temporary folder.

    You can see how this test does it:
     /toolkit/content/tests/browser/browser_save_resend_postdata.js

 * Replace the message box implementation with a mock one, that actually
    checks that the prompt about bad permissions is shown.

    You can see how this test does it:
     /toolkit/mozapps/downloads/tests/chrome/test_bug_412360.xul

 * Simulate clicking the "OK" button.
 * Detect that the test is finished, and clean everything up.

I understand that doing all of this in the right way is quite a challenge!
Especially, most calls are asynchronous and every step must be implemented
inside a callback function.

To see a chrome test in action, you can use the following command line:

TEST_PATH=toolkit/mozapps/downloads/tests/chrome/test_unkownContentType_dialog_layout.xul make mochitest-chrome

You can make a copy of the file to use as a starting point for your test.

On a related subject, I was able to manually reproduce the bug when the _default_ directory for downloads is write-protected, and that case is not actually covered by your patch yet, while I think the code should be structured to handle both cases in the same function. Probably another automated test should reproduce this specific condition, unless it is clear from the code that one test covers both cases.

On the other hand, considering that this patch is going to fix an edge case, if the patch can be reduced to a nearly one-liner, the test might not be necessary, although this is a decision that should be agreed upon by the module owner.

That said, here are my comments on the patch itself. I'll try to answer to your in-line questions, let me know if you need some other clarification. For the next version, however, please make sure that the patch is in its "final" form, so that the result is how you intend it to appear when checked in.

>+ * Helper for debugging in XPCOM component.
>[...]
>+function mydump(msg)
>+{
>+  Components.utils.reportError(msg);
>+}

In fact, debugging functions should not be present in the final form.

>+///////////////////////////////////////////////////////////////////////////////
>+//// auxpromptForSaveToFile()

If I understand correctly, this displays the file picker and deletes an existing
file if the user chose to overwrite. Please name the function accordingly.

>+// @aLauncher               same as in promptForSaveToFile

For new functions, use JSDoc-style comments. Example:

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/downloads/DownloadUtils.jsm#206

Also, provide an explicit explanation of the meaning of the parameters.

>+function  auxpromptForSaveToFile (aLauncher, aContext, aDefaultFile, aSuggestedFileExtension, baseobject) {

Why a global function with a "baseobject" parameter? This should be a member function inside the object prototype, and continue to use "this" instead of "baseobject".

>+  baseobject.debug = true;

Debug code to be removed.

>+  // The last directory preference may not exist, which will throw.
>+  try {
>+    var lastDir = gDownloadLastDir.file;
>+    if (isUsableDirectory(lastDir))
>+      picker.displayDirectory = lastDir;
>+  }
>+  catch (ex) {
>+  } // TODO/FIXME: empty catch-clause.  VERY BAD CODING PRACTICE.

Empty catch clauses are necessary sometimes, especially when they wrap
a single statement that is expected to raise an exception in some usual
cases but we don't know exactly which exception (it's the case of the
MIME method calls, for instance).

In this case, given the comment in the code, the try-catch block may be
limited to getting "gDownloadLastDir.file". Make sure that exceptions
with isUsableDirectory are handled correctly, however.

>+  if (picker.show() == nsIFilePicker.returnCancel) {
>+    // null result means user cancelled.
>+    // TODO/FIXME: this type of overloading of meaning should
>+    // be avoided. We should have an out parameter that tells
>+    // us the user action explicitly. (cancel in this case.)
>+    return null;
>+  }

As long as this is documented, having the function return null for
an expected outcome is acceptable, even recommended to avoid the
overhead of having out parameters in JavaScript. Exceptions can
instead be used for unlikely failure cases.

>+    catch (e) {
>+      // TODO/FIXME: remove above can fail due to NS_ERROR_FILE_ACCESS_DENIED here,
>+      // or even some timing-related race conditions between the exists() check
>+      // (which returns true) and remove() operation if somebody removes
>+      // the file in-between.

Then you should check for the corresponding exception and filter it.

>+      if (e.result == Components.results.NS_ERROR_FILE_ACCESS_DENIED) {
>+        // TODO/FIXME
>+        // Here we want to see the save operation dialog re-displayed.
>+        // How do we achieve this?

Leave the responsibility for handling exceptions to the outer function
(as you are correctly doing already).

>+//// nsUnknownContentTypeDialog  : a typo!

Just fix it! :-)

>       let autodownload = false;
>       try {
>         autodownload = prefs.getBoolPref(PREF_BD_USEDOWNLOADDIR);
>-      } catch (e) { }
>+      } catch (e) { } // TODO/FIXME: empty catch-clause. VERY BAD CODING PRACTICE.

Example of appropriate empty catch clause. We don't know the exact result code,
and we don't want to report the error to the console.

>+            // TODO/FIXME: So what do we want to return here?
>+            // At least under linux, there doesn't seem to be an ill-effect
>+            // with "return" only.
>             return;

"return null" is better, good catch.

>+            // Display error alert (using text supplied by back-end)
>+            prompter.alert(this.dialog,
>+                           bundle.GetStringFromName("badPermissions.title"),
>+                           bundle.GetStringFromName("badPermissions"));

I've seen the same lines in other parts of the code. The code should instead
be structured to have a single instance of this call.

>+        // WE BREAK OUT OF LOOP of here.
>+        // Actually a simple "return result" will do
>+        // this is not very clear enough! So I wrote the code as below.
>+        if(result == null) // user cancelled.
>+        {
>+          return null;
>+        }
>+        else
>+        {
>+          break;
>+        }
>+      } // end while
>+      return result;

Just move "return result" in the infinite loop, and ensure that there are no break statements inside.

Also, the fact that the save dialog is re-displayed in case of early error detection is a separate change. It certainly needs a separate test, and we have to decide if this is actually the behavior we want, as in most other cases we can't detect the error so early and the dialog isn't re-displayed in any case, so the behavior might seem inconsistent from the user's point of view.

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> Your automated test case should probably be a "chrome" test
> (https://developer.mozilla.org/en/Mozilla_automated_testing#Chrome_tests),
> and it should do the following operations:
Could probably do it with xpcshell too.

> I understand that doing all of this in the right way is quite a challenge!
> Especially, most calls are asynchronous and every step must be implemented
> inside a callback function.
Note that generators can help here.  A number of these tests use this technique:
http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/

> On the other hand, considering that this patch is going to fix an edge case, if
> the patch can be reduced to a nearly one-liner, the test might not be
> necessary, although this is a decision that should be agreed upon by the module
> owner.
We should probably write the test so we don't regress this in the future.

> Also, the fact that the save dialog is re-displayed in case of early error
> detection is a separate change. It certainly needs a separate test, and we have
> to decide if this is actually the behavior we want, as in most other cases we
> can't detect the error so early and the dialog isn't re-displayed in any case,
> so the behavior might seem inconsistent from the user's point of view.
Probably should be done in a follow-up bug too.

Comment 21

[ISHIKAWA, Chiaki]

(In reply to comment #19 and comment #20)

I will study the comment to generate the test case.
(I have been manually creating the temporary directory under /tmp
and set the attribute with chmod to create a write-protected directory for this
bug.)

I was surprised to learn that Windows doesn't allow one to pick a write-protected
directory for the attachment saving. (I am not sure if this were the case with
my XP SP3 environment, but a clever GUI library would certainly tries to
enforce some restraint and such behavior is understandable.)

I will also study the comments about the patch style, etc. and
come up with a better patch in the next several days hopefully.

TIA

Comment 22

[ISHIKAWA, Chiaki]

(In reply to comment #21)
> I was surprised to learn that Windows doesn't allow one to pick a
> write-protected
> directory for the attachment saving. (I am not sure if this were the case with
> my XP SP3 environment, ...

I think the problem discussed in this thread may only occur on POSIX-compliant
systems such as linux, Solaris, FreeBSD, etc.
(And later version of windows such as Vista(?), Windows 7(?), maybe?)

XP SP3 shows strange behavior.

Under Windows, I did the following.

create a directory:   c:\download\readonly-dir

make it read-only:     attrib +r c:\download\readonly-dir

checked it using "ls -ld" from cygwin.
 
C:\Documents and Settings\ci>ls -ld /cygdrive/c/download/readonly-dir
dr-x------+ 1 ci blahblah 0 2010-09-02 10:30 /cygdrive/c/download/readonly-dir

blahblah shows something unreadable (must be a group name in Japanese characters
or something.)

Then I used TB 3.1.2 under XP SP3, and tried to save an attachment
to this read-only directory.

To my surprise, the download manager window appears for a flick of a second
indicating the write-operation (?).
To my bewilderment, the attachment was saved to the directory(!)

So either, 
- the "attrib +r" may not be the proper way to make a read-only directory
  under XP SP3,  OR
- TB 3.1.2 doesn't enforce the read-only property of a directory and write into
  it anyway (and XP allows such writing, or maybe the runtime library
  did some magic like make the directory writable and then revert it
  to read-only?).

I am a bit perplexed. But these must be very peculiar to Windows XP, I suppose.

At least, under linux, the write-protection mechanism works and
TB3 fails to write an attachment to a write-protected directory,
but the warning as to this failure is not shown.

Honestly, I won't be able to handle Window's strange behavior without
clear explanation of what is going on.

TIA

Comment 23

[ISHIKAWA, Chiaki]

(In reply to comment #20)
> (In reply to comment #19)
> > Your automated test case should probably be a "chrome" test
> > (https://developer.mozilla.org/en/Mozilla_automated_testing#Chrome_tests),
> > and it should do the following operations:
> Could probably do it with xpcshell too.
> 
> > I understand that doing all of this in the right way is quite a challenge!
> > Especially, most calls are asynchronous and every step must be implemented
> > inside a callback function.
> Note that generators can help here.  A number of these tests use this
> technique:
> http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/test/
> 
 ...
> We should probably write the test so we don't regress this in the future.
> 

I have tried to understand the test framework, but being a JavaScript newbie doesn't help me very much.

Also, I found the explanation in various articles are very heavily tuned to
the browser debugging if I am not mistaken.

Can someone point at a good example of TB3 test case that is generated
by the generator or to be used by xpcshell?

To be frank, at this stage, writing the test in javascript for TB3 seems to be the highest barrier for any submitter of reasonably complex patch :-(

TIA

PS: Re XP's strange behavior, since my XP account has the administrative 
priviledge, maybe the C or C++ runtime didn't bother to check for
the write permission but simply wrote the file to a write-protected directory
(Emacs, a ka meadow port, probably checks the write permission bit and warns the user to show a behavior similar to POSIX environment.)

Comment 24

[:Paolo Amadini]

(In reply to comment #23)
> To be frank, at this stage, writing the test in javascript for TB3 seems to be
> the highest barrier for any submitter of reasonably complex patch :-(

It is often the toughest and most time consuming part indeed, but once you
understand how to leverage it, it makes your development workflow much easier.

Understanding the inner workings of the framework is a huge task, and I never
attempted it. The good news are that you don't need to understand it to
begin writing new tests or running existing tests. The framework is mostly
automated, and should work with only a few manual steps, that are the ones
explained in the build guide (like creating ".mozconfig" and so on).

This is in fact the approach I follow when I have to work with code bases as
complex as the testing framework. I start from scratch, following with a lot
of attention all the instructions in the documentation I can find, and run
the program (tests in this case) in the default configuration. Only at this
point I make changes and see the effect they have - sometimes I start with
a change totally unrelated to the task at hand, just to see if it has the
effect I expect, and gain confidence with the development tools.

In the case of Mozilla applications, the documentation allowed me to set up
a test environment easily, even though it required a lot of time and patience.
I started with building Firefox. The components in the Thunderbird build
system are basically a superset of the ones of Firefox, and thus building
and running tests on it is just a _bit_ more complex. Since your change is
part of Toolkit, which is a portion shared by all the Mozilla applications,
I think you can make your change in a Firefox build, if you think this may
help you in folllowing the build and test instructions.

I don't have a Thunderbird build and I can't figure out how an xpcshell test
might work for this case, maybe Shawn might help with this. In fact, the best
references I can provide at present are the ones in comment 19. In any case,
I think your first goal might be to run the "unknownContentType_dialog_layout"
mochitest, then make a change in the test to see it fail:

TEST_PATH=toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_dialog_layout.xul make mochitest-chrome

This is for a Firefox build, maybe TEST_PATH is different if you have a
Thunderbird build.

Then, in order to make changes towards the final goal of testing the patch
you are writing, you should study some of the advanced language features
introduced in recent JavaScript versions, like generators and iterators:

https://developer.mozilla.org/en/JavaScript/Guide/Iterators_and_Generators

Even if you'll not be using such features, this will make you understand
the code of other tests you can use as reference. Also, you have to be
familiar with the event system of the Document Object Model, since it is
used by tests when they load web pages and drive XUL dialogs.

If you have a specific question you can't find in the documentation, just
ask! I don't have a lot of time available now, so the answer might come
after some days, but I'll do my best, within the limits of what I know.
For questions that you think have a simple answer, you may also try asking
in realtime in the #developers IRC channel.

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to comment #24)
> I don't have a Thunderbird build and I can't figure out how an xpcshell test
> might work for this case, maybe Shawn might help with this. In fact, the best
> references I can provide at present are the ones in comment 19. In any case,
> I think your first goal might be to run the "unknownContentType_dialog_layout"
> mochitest, then make a change in the test to see it fail:
> 
> TEST_PATH=toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_dialog_layout.xul
> make mochitest-chrome
> 
> This is for a Firefox build, maybe TEST_PATH is different if you have a
> Thunderbird build.
Given that this is a patch to toolkit functionality, the test should be developed to be ran in the browser (which is where we run tests in mozilla-central).  Thunderbird will still pick up the changes.

Comment 26

[ISHIKAWA, Chiaki]

(In reply to comment #25)
> Given that this is a patch to toolkit functionality, the test should be
> developed to be ran in the browser (which is where we run tests in
> mozilla-central).  Thunderbird will still pick up the changes.

Thank you for the pointer. Many test documents describe the testing of Firefox and
so I will try to figure out how to create a test case that touches the situation
in the browser case.

TIA

Comment 27

[ISHIKAWA, Chiaki]

Hi,

Since September/October last year, I could not get the compilation/linking of TB trunk succeed on my Debian GNU/Linux  due to various errors : Debian GNU/Linux toolchain issues, and some shuffle due to XPCOM framework changes, etc.  And thus, I could not
get to improve the patch (to address issues raised by Paulo not to mention the eventual creation of test cases for testing framework.) during that time. (I was forced to doing other things such as looking and counting the warnings generated during the failed compilation, etc.  http://bugzilla.mozilla.org/show_bug.cgi?id=609210

Lately, however, I was able to compile/link TB trunk again finally, and will try to create a new patch that hopefully addresses issued raised by Paulo.

Hmm. Creating testing in browser as noted by comment 25, "the test should be developed to be ran in the browser"
will be the biggest challenge for me fixing TB.

Also, the demise of the javascript debugger (?) mentioned below
in the add-ons page for TB 

https://addons.mozilla.org/ja/thunderbird/addon/javascript-debugger/

is a blow to me since
nsHelperAppDlg.js is after all a javascript and
already I have a problem in javascript interpreter 
seemingly raising a mysterious error which I can't fathom after I modified the patch somewhat, but have no way to figure it out yet. :-(
(The above URL is a Japanese page, please search for javascript debugger
in the search box to reach your preferred language page.)

So please don't hold your breath, but I am progressing again and hope to produce a patch (and the test) soon.

TIA

PS: The latest trunk (Shredder) seems to have changed the UI for attachment saving somewhat: Now you can't double click (LEFT clicking) the attachment directly to raise a pop-up, but rather the first clicking expands the attachment to show the name, etc. and then on it, you can click further to open the dialog for saving. 
For right-clicking, you already have a "SAVE" button in the right of the row where the attachment is shown, and you can click on the button to invoke the same functionality which was previously invoked by right-clicking on the attachment (and still is).  

So the UI changes somewhat, but the functionality remains the same, and the bug is there, too.
(Silent failure to save to a non-writable directory [via left-click], and showing of error dialog TWICE [via right-click].)

Comment 28

[ISHIKAWA, Chiaki]

Hi, almost 9 months after the last review of my patch (later part of Comment 19), I finally 
could compile/link TB trunk (hg repository) on my Debian GNU/Linux installation again, and am uploading a new patch against the latest TB trunk
addressing the issues mentioned in comment 19.

Here is my comment on the review.
The patch will be uploaded in a few minutes.

>That said, here are my comments on the patch itself. I'll try to
>answer to your in-line questions, let me know if you need some other
>clarification. For the next version, however, please make sure that
>the patch is in its "final" form, so that the result is how you intend
>it to appear when checked in.

Below is the
numbered response and the original review comment, followed by some
additional comments of mine.


1: I left the code in the comment.

   The reason is that 
   - this file has been the source of many problems over the yeas, and
     I am sure someone wants to hack this code sooner or later again.
     I think for would-be programmer, the hint about using one's own
     debug output routine is very important. Without this, I would not
     have been able to debug this code. JavaScript debugger is not
     quite user-friendly when I tried it.

>+ * Helper for debugging in XPCOM component.
>[...]
>+function mydump(msg)
>+{
>+  Components.utils.reportError(msg);
>+}
>
>In fact, debugging functions should not be present in the final form.

2. I have changed the name to `auxDisplayFilePickerForSaveToFile()'

>+///////////////////////////////////////////////////////////////////////////////
>+//// auxpromptForSaveToFile()

If I understand correctly, this displays the file picker and deletes an existing
file if the user chose to overwrite. Please name the function accordingly.

3. I used the JSdoc-style comments for new functions. 

DONE : 
>+// @aLauncher               same as in promptForSaveToFile
>
>For new functions, use JSDoc-style comments. Example:
>
>http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/mozapps/downloads/DownloadUtils.jsm#206

>
>Also, provide an explicit explanation of the meaning of the parameters.
>
   Provided the type and meaning for a few of them. 

   However, for a few parameters to original promotForFileToSave, I
   have not been able to figure out exactly what they are supposed to
   be. I never bothered.


4: Fixed.
>+function  auxpromptForSaveToFile (aLauncher, aContext, aDefaultFile, aSuggestedFileExtension, baseobject) {
>

>Why a global function with a "baseobject" parameter? This should be a
> member function inside the object prototype, and continue to use
> "this" instead of "baseobject".

Indeed. So I changed this into proper member functions, and 
use `this' instead of passing it as a parameter.

I was under a mistaken impression that an unexported (or hidden)
member function in a class that is accessed via XPCOM mechanism ought
to be declared (as hidden) in the corresponding *.idl file even if
this function is hidden (and not exported.). So I thought to move this
function as a global because I didn't want to touch *.idl file.

Of course, I was wrong(!).

And so the functions are now proper member functions and base is used
there. (It seemed the last time I tried to use member function a
couple of years ago, when I experimented with tinkering this file, it
didn't work because I put the function into the wrong class that
appeared in this file earlier than the correct one by mistake:

E.g.: BAD  nsUnkownContentTypeDialogProgressListener.prototype = ...
      GOOD nsUnknownContentTypeDialog.prototype = ...

This time, I made the same mistake again, and figured out the problem
by accident.  Hmm... I need a better pair of glasses, or need to set
larger font for Emacs session, I suppose.

(By the way the first prototype name seems to be misspelled, but
I am afraid that it must stay as is since it seems it is exported and
used in other files(?).)


5.

>+  baseobject.debug = true;
>Debug code to be removed.

Again, I left it as a comment so that future would-be hacker find it
easier to start working on this file.


6. Empty clauses.

>+  // The last directory preference may not exist, which will throw.
>+  try {
>+    var lastDir = gDownloadLastDir.file;
>+    if (isUsableDirectory(lastDir))
>+      picker.displayDirectory = lastDir;
>+  }
>+  catch (ex) {
>+  } // TODO/FIXME: empty catch-clause.  VERY BAD CODING PRACTICE.

>Empty catch clauses are necessary sometimes, especially when they wrap
>a single statement that is expected to raise an exception in some usual
>cases but we don't know exactly which exception (it's the case of the
>MIME method calls, for instance).
>
>In this case, given the comment in the code, the try-catch block may be
>limited to getting "gDownloadLastDir.file". Make sure that exceptions
>with isUsableDirectory are handled correctly, however.

The points are well taken.

However, the rampant use is not limited to legitimate cases, and as I
noted earlier, this type of wholesale catching of errors interfered
with catching the legitimate errors of coding (such as syntax errors
noticed by the interpreter) in some cases.  So I still mark some
places as "{E|e}mpty catch-clause" to alert the future would-be
hackers.

Please understand that I *mark* them for future would-be hackers as a
possible trouble spot in the long run.

7. I tried to document the return type of a few functions and
   the flow.

>>+  if (picker.show() == nsIFilePicker.returnCancel) {
>>+    // null result means user cancelled.
>>+    // TODO/FIXME: this type of overloading of meaning should
>>+    // be avoided. We should have an out parameter that tells
>>+    // us the user action explicitly. (cancel in this case.)
>>+    return null;
>>+  }
>

>As long as this is documented, having the function return null for an
>expected outcome is acceptable, even recommended to avoid the
>overhead of having out parameters in JavaScript. Exceptions can
>instead be used for unlikely failure cases.


8.

>>+    catch (e) {
>>+      // TODO/FIXME: remove above can fail due to NS_ERROR_FILE_ACCESS_DENIED here,
>>+      // or even some timing-related race conditions between the exists() check
>>+      // (which returns true) and remove() operation if somebody removes
>>+      // the file in-between.

>Then you should check for the corresponding exception and filter it.

For this, I tried to check the error(s) returned when a file already
exists and is not writable, and the directory that holds it unwritable.
The current code handles it very well.

However, the recent GUI library, GTK 2, under Debian GNU/Linux has
become very clever since last year, and I can not test unusual
cases any more.
E.g.
FilePicker under Gnome? is much wiser.
It seems that it checks internally whether the pathname up to the last file
component exists or not (for example, non-existing directory is checked!).

So previously, last year, I could create /tmp/b-dir, then trying to
save the attachment there, and seeing the file picker start for saving
the attachment and choose /tmp/b-dir, and then before hitting SAVE, I
could remove /tmp/b-dir and then hitting SAVE and see the action of
TB3 to handle the missing directory.
(/tmp/b-dir/saved-attachment-file-name is passed back to TB3 but
/tmp/b-dir/ is no longer existing, and thus should fail.)

But now, I think before returning the /tmp/b-dir/saved-attachment-file-name
to TB3, file picker on its own checks that /tmp/b-dir/ exists and
if b-dir doesn't exist any more, hitting SAVE is a basically a noop, but
it changes the location to save the file, and we are now
moved to immediately below /tmp to continue specifying where to save the
attachment.

A good improvement in the external GUI library.
 
>>+      if (e.result == Components.results.NS_ERROR_FILE_ACCESS_DENIED) {
>>+        // TODO/FIXME
>>+        // Here we want to see the save operation dialog re-displayed.
>+        // How do we achieve this?
>
>Leave the responsibility for handling exceptions to the outer function
>(as you are correctly doing already).

Noted and commented as such.

9.

//>+//// nsUnknownContentTypeDialog  : a typo!
//
//Just fix it! :-)

Fixed.

10:
>>       let autodownload = false;
>>       try {
>>         autodownload = prefs.getBoolPref(PREF_BD_USEDOWNLOADDIR);
>>-      } catch (e) { }
>>+      } catch (e) { } // TODO/FIXME: empty catch-clause. VERY BAD CODING PRACTICE.
>
>Example of appropriate empty catch clause. We don't know the exact result code,
>and we don't want to report the error to the console.

OK.

11.

//>+            // TODO/FIXME: So what do we want to return here?
//>+            // At least under linux, there doesn't seem to be an ill-effect
//>+            // with "return" only.
//>             return;
//
//"return null" is better, good catch.

now, return null.

12.

>>+            // Display error alert (using text supplied by back-end)
>>+            prompter.alert(this.dialog,
>>+                           bundle.GetStringFromName("badPermissions.title"),
>>+                           bundle.GetStringFromName("badPermissions"));
>
>I've seen the same lines in other parts of the code. The code should instead
>be structured to have a single instance of this call.

Done. See   displayBadPermissionAlert()


13.
//>+        // WE BREAK OUT OF LOOP of here.
//>+        // Actually a simple "return result" will do
//>+        // this is not very clear enough! So I wrote the code as below.
//>+        if(result == null) // user cancelled.
//>+        {
//>+          return null;
//>+        }
//>+        else
//>+        {
//>+          break;
//>+        }
//>+      } // end while
//>+      return result;
//
//Just move "return result" in the infinite loop, and ensure that there are no break statements inside.

done.


14. I am not sure.

NOT SURE/TODO/FIXME

>Also, the fact that the save dialog is re-displayed in case of early
>error detection is a separate change. It certainly needs a separate
>test, and we have to decide if this is actually the behavior we want,
>as in most other cases we can't detect the error so early and the
>dialog isn't re-displayed in any case, so the behavior might seem
>inconsistent from the user's point of view.

As I noted above, the better FileChooser offered by GUI library makes
it difficult for applications to pick up a file pathname with
non-existiing intermediate directory name and such, and so we have
less chance of getting into trouble for better or worse.  (And as a
matter of fact, creating such situation for testing is difficult now.)

15. In one place I added try/catch because when I cancel the
file picker, that code raises an error that is not caught by any
outside routine and recorded in the Error console.
(This may have never been a problem before in the old code?)


TIA

PS: Now on to the test script if the patch looks OK.

Comment 29

[ISHIKAWA, Chiaki]

Attachment: Proposed patch to nsHelperAppDlg.js (Trunk)

The new patch against Trunk (!)

Comment 30

[:Paolo Amadini]

Comment on attachment 537791 [details] [diff] [review]
Proposed patch to nsHelperAppDlg.js  (Trunk)

(In reply to comment #28)
> Hi, almost 9 months after the last review of my patch (later part of Comment
> 19), I finally 
> could compile/link TB trunk (hg repository) on my Debian GNU/Linux
> installation again, and am uploading a new patch against the latest TB trunk
> addressing the issues mentioned in comment 19.

Welcome back, Chiaki! And thank you for your renewed interest in improving
Thunderbird and Firefox, even for users with less common configurations.
I'll try and follow this patch closely, though I've a lot of other work
to do in these days, thus reviews and comments on my part might require
some more time than usual.

Given that there's still a lot of work to do to reach the final form of
the patch, including writing the automated test cases, I recommend you
use a few common strategies that facilitate review, thus improving the
time to completion.

Most importantly, the current patch contains the changes required to fix
this bug, plus other changes related to showing the file picker multiple
times in case of errors. The latter modifications should really be handled
in a separate patch and bug, firstly to facilitate review, and secondly
because there will be discussions to understand if this is what we want
or not, which might require input from User Experience people (ui-review).

Secondly, the size of the patch should be kept to the shortest possible.
Sometimes, review iterations that go in the right direction will see the
patched file becoming shorter which each review (for example, because
common code is shared in a single function). We also keep discussions
as meta-data, in the bug, and only leave TODO comments in the code if
there is an open task that's likely to result in a specific action (with
an associated bug number). We can do this because anyone can find the bug
associated with a change using the Mercurial version control interfaces,
thus finding out the reasoning behind some of the choices made in the code.

Finally, with regard to debugging aids and references to best practices left
in the code, they're mostly unnecessary. Working on an in-house project and
working on one with thousands of active developers is very different. Consider
for example that the Mozilla development community is diverse, and not
everyone uses the same tools and techniques for debugging. I sometimes used Chromebug as my debugger, and in that case dump functions served no purpose
for me. In other cases I added ad-hoc reportError calls. Recommendations on
how to debug or best coding practices should be explained in the Mozilla
Developer Center documentation, where they can be found regardless of the
area of the code one's working on.

In the light of that, I'm quite confident that fixing this specific bug
(no error message in the explained cases) only requires a few lines of
modifications. Of course, feel free to raise any other issues you find with
the code as separate comments, or file other bugs.

Most of the time, actually, will be required to write the test case for the
change, as explained in comment 19. It's quite usual for a test case to be
up to ten times more complex than the change it is going to test. I'm
available if you have any question during the process.

A few more specific comments follow - other changes not noted here or covered
by the above general principles are fine!

> nsUnkownContentTypeDialogProgressListener.prototype = ...
> 
> (By the way the first prototype name seems to be misspelled, but
> I am afraid that it must stay as is since it seems it is exported and
> used in other files(?).)

Used where?

http://mxr.mozilla.org/comm-central/search?string=nsUnkownContentTypeDialogProgressListener

It doesn't seem to be exported, we can probably rename it.

> 6. Empty clauses.
> Please understand that I *mark* them for future would-be hackers as a
> possible trouble spot in the long run.

Not necessary, especially because as I said they are legitimate in some
cases. If you want to investigate and fix the unnecessary ones yourself,
by restricting the clauses to specific error codes, feel free to file a
separate bug. But it's unlikely that just marking them in the code would
result in an action, also given the effort to reward ratio (there's the
risk of breaking correct behavior for no practical gain). For new code,
the reviewers ensure that best practices are followed - no risk of
copy-and-paste code reaching the main repository, even if not marked :-)

> As I noted above, the better FileChooser offered by GUI library makes
> it difficult for applications to pick up a file pathname with
> non-existiing intermediate directory name and such, and so we have
> less chance of getting into trouble for better or worse.  (And as a
> matter of fact, creating such situation for testing is difficult now.)

Automated test cases won't use the file picker, so you should be able to
emulate the behavior (which might still happen on other platforms). More
generally, I think now it's up to you to judge if fixing this bug is still
relevant for Thunderbird users, given the evolution of the hosting platforms.

> 15. In one place I added try/catch because when I cancel the
> file picker, that code raises an error that is not caught by any
> outside routine and recorded in the Error console.
> (This may have never been a problem before in the old code?)

We should check that eating the error is not affecting other callers or code
paths, which might assume that the error is propagated. So this small change
might also be worth of being handled in a separate bug.

Comment 31

[ISHIKAWA, Chiaki]

(In reply to comment #30)
...

> Welcome back, Chiaki! And thank you for your renewed interest in improving
> Thunderbird and Firefox, even for users with less common configurations.
> I'll try and follow this patch closely, though I've a lot of other work
> to do in these days, thus reviews and comments on my part might require
> some more time than usual.
> 
> Given that there's still a lot of work to do to reach the final form of
> the patch, including writing the automated test cases, I recommend you
> use a few common strategies that facilitate review, thus improving the
> time to completion.
> 
> Most importantly, the current patch contains the changes required to fix
> this bug, plus other changes related to showing the file picker multiple
> times in case of errors. The latter modifications should really be handled
> in a separate patch and bug, firstly to facilitate review, and secondly
> because there will be discussions to understand if this is what we want
> or not, which might require input from User Experience people (ui-review).

Thank you for Paulo for quick run down on the problems of the current patch.

I will address the issues and produce a minimal patch, and possibly filing a separate bug entry to shorten the patch.

I should be able to do this over the weekend or until next Tuesday.

TIA.

Comment 32

[ISHIKAWA, Chiaki]

I filed a separate bug,

Bug 663667 - TB throws an uncaught error when user cancels file picker during saving of mail attachment (left-clicking)

to take care of the following.

>> 15. In one place I added try/catch because when I cancel the
>> file picker, that code raises an error that is not caught by any
>> outside routine and recorded in the Error console.
>> (This may have never been a problem before in the old code?)

>We should check that eating the error is not affecting other callers or code
>paths, which might assume that the error is propagated. So this small change
>might also be worth of being handled in a separate bug.>

The uncaught error is generated with the original pristine nsHelperAppDlg.js
from trunk distribution.

Separating this bug will shorten my next proposed patch.

Comment 33

[ISHIKAWA, Chiaki]

I am uploading a patch that contains only the
 - spelling fixes
 - proper js doc description, and
 - in one place, change aLocalFile => aLocalFolder changes.

I am hoping that this patch is short and less controversial than the previous one so that
it can be applied in advance for the
main patch to fix the problem.

Any comment?

Comment 34

[ISHIKAWA, Chiaki]

Attachment: [checked in] Spelling fix, proper JS documentation fix, one ID change.

This is a short patch to solve minor spelling, comment, and a issue concerning the appropriateness of an ID.
It is hoped that this patch is applied before the main patch to fix the problem so that the final patch would be shorter and easier to review.

Comment 35

[:Paolo Amadini]

Comment on attachment 538754 [details] [diff] [review]
[checked in] Spelling fix, proper JS documentation fix, one ID change.

Excellent! :-) Looks good to me. I suppose you tested the affected code paths
and everything worked fine?

The Downloads module owner should also review the patch before check-in, but
since these are just spelling fixes I don't expect it will take too long.
I've already set the additional review flag.

Comment 36

[Mark Banner (:standard8)]

Moving to the toolkit product, as the actual issue here is in the toolkit code.

Comment 37

[ISHIKAWA, Chiaki]

(In reply to comment #35)
> Comment on attachment 538754 [details] [diff] [review] [review]
> Spelling fix, proper JS documentation fix, one ID change.
> 
> Excellent! :-) Looks good to me. I suppose you tested the affected code paths
> and everything worked fine?
> 

Yes, I did. Saving to writable directory, and [failed] writing to non-writable directory [which fails silently], etc.


> The Downloads module owner should also review the patch before check-in, but
> since these are just spelling fixes I don't expect it will take too long.
> I've already set the additional review flag.

Thank you!

Comment 38

[ISHIKAWA, Chiaki]

Ping?

Has the patch mentioned above in comment #34 been reviewed by
the toolkit module owner?
I wish that it is included in the comm-central repository (and the mozilla subdirectory below it), so that the
major patch for the fix can now be uploaded with less cluttering.

TIA

Comment 39

[Shawn Wilsher :sdwilsh]

Comment on attachment 538754 [details] [diff] [review]
[checked in] Spelling fix, proper JS documentation fix, one ID change.

r=sdwilsh

Comment 40

[ISHIKAWA, Chiaki]

(In reply to Shawn Wilsher :sdwilsh from comment #39)
> Comment on attachment 538754 [details] [diff] [review]
> Spelling fix, proper JS documentation fix, one ID change.
> 
> r=sdwilsh

Thank you! I am keeping my fingers crossed.

Comment 41

[ISHIKAWA, Chiaki]

Attachment: Current WHOLE patch in progress (including the previous patch) and extra debug output.

While waiting for the previous patch to fix spelling errors, and others,
here is the current WHOLE patch in the works.

The reason I am posting is that I have finally covered all the major error cases that are visible to the users using a generic I/O error message function.
(You can check what is going on by looking at post that was sent to the
mozilla.dev.platform newsgroup on Oct 26, 2011 and some follow-ups.)
In a nutshell, nsIErrorService which should give us a hook to look for
error message strings for typical error code returned by XPCOM routines, etc.
doesn't work today very well.
So I had to create a function to map an error code to short English message.
This will do for now.

It can now handle the case of disappearing directory
(e.g., trying to save to /tmp/c-dir/attachment, and suddenly /tmp/c-dir
disappears since it is deleted by another process),
and writing to a CDROM device [which returns a very strange error 
indeed.]

I thought some users waiting for the final fix to land may want to see how the
current final patch looks like.

I intend to remove the debug output from the current patch and re-submit.
But this will have to wait for the previous lighter patch to land 
to fix the minor spelling issues, etc.

Comment 42

[ISHIKAWA, Chiaki]

Attachment: An error dialog when the directory disappears and error console output

This is a screen capture after an error dialog appears when
trying to save to /tmp/c-dir/attachment from the file picker, but
before the real writing takes place, the directory was removed
from another terminal.
You can see the debug output in the error console (thanks to mydump() function
in the patch) and figure out how the execution proceed in the proposed patch.

Comment 43

[:aceman]

How general is this patch? The bug was moved into Download manager. Is it possible this will help also with bug 700114 in Firefox?

Can any of your older patches be marked obsolete (superseded by newer ones)? There are currently 5 active attachments and 3 obsolete ones.

Comment 44

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #43)
> How general is this patch? The bug was moved into Download manager. Is it
> possible this will help also with bug 700114 in Firefox?
> 

I think this will solve the case when download manager 
gives the user the false impression that the saving occured
while there was an error underneath and nothing happens.

I will post a comment there.
The earlier check before the saving doesn't happen
on linux because I think iswritable() and isreadable()
implementations are bogus on linux.

> Can any of your older patches be marked obsolete (superseded by newer ones)?
> There are currently 5 active attachments and 3 obsolete ones.

I will try to see what I can.

I am waiting for the spelling fix to land first so that the
final patch is much easier to evaluate by people in the know.
But the spelling patch has somehow took much longer than usual to land :-(

Comment 45

[:aceman]

OK, if you can't land the first patch yourself I have added the "checkin-needed" keyword so that people with permissions will find it and land it for you into comm-central. It seems it has proper reviews.

Also, the platform is set to Win XP, but I have the impression you talk about linux in the comments. What platform does the bug and fix apply to?

Comment 46

[ISHIKAWA, Chiaki]

> Also, the platform is set to Win XP, but I have the impression you talk
> about linux in the comments. What platform does the bug and fix apply to?

Thank you.

The problem was noticed under linux, but the fix should be applicable to all other platforms.

Comment 47

[:aceman]

I'll set the link to bug 700114 so that it doesn't get missed. If your final patch gets approved I'll check if it helped in any way in Firefox.

Comment 48

[:aceman]

Also, after you clean up (mark obsolete) the patches here, can you request review for your current patch?

Comment 49

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #48)
> Also, after you clean up (mark obsolete) the patches here, can you request
> review for your current patch?

Will do. I hope to finish it very soon.

Comment 50

[:aceman]

I have tried to mark the patches for you. I hope I got it right.
I understand it only the "Spelling fix" patch should be landed right now. And then you will update the rest of the patch and get the reviews.

Comment 51

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #50)
> I have tried to mark the patches for you. I hope I got it right.
> I understand it only the "Spelling fix" patch should be landed right now.
> And then you will update the rest of the patch and get the reviews.

That is correct. Thank you very much.
TIA

Comment 52

[Mark Banner (:standard8)]

Comment on attachment 538754 [details] [diff] [review]
[checked in] Spelling fix, proper JS documentation fix, one ID change.

Landed on inbound, should be merged to central soon:

https://hg.mozilla.org/integration/mozilla-inbound/rev/031a6b22d380

Comment 53

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/031a6b22d380

Comment 54

[:aceman]

ISHIKAWA chiaki, it seems your first patch was checked into mozilla. You can now update your source tree and update your main patch.

Comment 55

[ISHIKAWA, Chiaki]

Will do! TIA.

Comment 56

[ISHIKAWA, Chiaki]

Attachment: Fix the failure to show the error message after a failure. (1st version: request for comment)

This fix includes the change to properly show the error message, I/O error message support function, a few fixes for typos, etc.

Comment 57

[ISHIKAWA, Chiaki]

I noticed that surrounding a problematic code around line 287 (line 583 in
new code) causes too much diff output and confusing.
Maybe I should have staged patches now.
One that shows only the enclosing of "try { ... } catch (e) { }"
 - this will produce only a few lines and a lot of lines with only
   whitespace changes (indentation changes due to the nesting.)

Then I can submit the whole new changes.

Thought?

Comment 58

[:aceman]

Comment on attachment 593323 [details] [diff] [review]
Fix the failure to show the error message after a failure. (1st version: request for comment)

For requests for comment you can use the "feedback?" flag on the attachment. I have added it for you.

Comment 59

[ISHIKAWA, Chiaki]

I am afraid that the patch was a little screwed up.
(Diff was created to a locally modified hg archive instead of
the true comm-central version, and so a few lines were
not in sync with the current comm-central and thus the diff interface from the web was difficult to read.)

I am trying to re-create the patch right now.

Comment 60

[ISHIKAWA, Chiaki]

Attachment: patch to fix the failure to notify the I/O error during saving of an attachment.

Clean up version (Rev 01: request for comment.)

Comment 61

[ISHIKAWA, Chiaki]

I am uploading a patch from a divided patch set.
This is to make the changes more understandable.
The following is a first patch that
only adds the top-level helper functions, and
add a try/catch clause so that
the main processing is only indented by "try { ".

The future second patch shows the remaining main change
on top of this. This should make reading the diff /comparison much easier to understand.

I wonder if such two stage patch is acceptable or not.

Comment 62

[ISHIKAWA, Chiaki]

Attachment: Adding top-level helper function, surround the main portion to be changed by try/catch for indention change.

1st part of split patch.

Comment 63

[ISHIKAWA, Chiaki]

Comment on attachment 593720 [details] [diff] [review]
Adding top-level helper function, surround the main portion to be changed by try/catch for indention change.

It is too bad  that web diff tool doesn't show 
the difference neatly :=(, but if you apply the patch, you will see that
adding try{} only re-indented the portion (which is to be modified in the second part of the split patch)

Comment 64

[ISHIKAWA, Chiaki]

Attachment: manual output of diff -cibw of the difference after apply the 1st part of the split patchset.

Here, diff -cibw was performed on the nsHelperAppDlg.js after the first patch set of the split patch set.
This shows clearly that the main portion of promptForSaveToFile
is only re-indented and a pure addition of catch() is also done.

Comment 65

[Shawn Wilsher :sdwilsh]

Comment on attachment 593720 [details] [diff] [review]
Adding top-level helper function, surround the main portion to be changed by try/catch for indention change.

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

Sorry this has taken me so long to look at.  Gavin should probably be the one giving you feedback at this point.

::: toolkit/mozapps/downloads/nsHelperAppDlg.js
@@ +133,5 @@
> +//
> +// #define NS_ERROR_GET_CODE(err)     ((err) & 0xffff)
> +// #define NS_ERROR_GET_MODULE(err)   (((((err) >> 16) - NS_ERROR_MODULE_BASE_OFFSET) & 0x1fff))
> +// #define NS_ERROR_GET_SEVERITY(err) (((err) >> 31) & 0x1)
> +//

This long comment isn't needed for the actual checkin.  The commit will reference the bug which will have all the background information in it :)

@@ +208,5 @@
> +    msg = "unknown error = " + code ; 
> +    break;
> +  }
> +  return "I/O error: " + msg;
> +}

gavin may have a better idea here

Comment 66

[ISHIKAWA, Chiaki]

(In reply to Shawn Wilsher :sdwilsh from comment #65)
> Comment on attachment 593720 [details] [diff] [review]
> Adding top-level helper function, surround the main portion to be changed by
> try/catch for indention change.
> 
> Review of attachment 593720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry this has taken me so long to look at.  Gavin should probably be the
> one giving you feedback at this point.
> 

Thank you for talking a look in your busy schedule.

> ::: toolkit/mozapps/downloads/nsHelperAppDlg.js
> @@ +133,5 @@
> > +//
> > +// #define NS_ERROR_GET_CODE(err)     ((err) & 0xffff)
> > +// #define NS_ERROR_GET_MODULE(err)   (((((err) >> 16) - NS_ERROR_MODULE_BASE_OFFSET) & 0x1fff))
> > +// #define NS_ERROR_GET_SEVERITY(err) (((err) >> 31) & 0x1)
> > +//
> 
> This long comment isn't needed for the actual checkin.  The commit will
> reference the bug which will have all the background information in it :)
> 


I will clean this up to reduce the clutter.

> @@ +208,5 @@
> > +    msg = "unknown error = " + code ; 
> > +    break;
> > +  }
> > +  return "I/O error: " + msg;
> > +}
> 
> gavin may have a better idea here

I will await the comment.

The patch above is the 1st stage.

After the 1st patch goes in, I will add another patch that
shows the processing the in the body of the major function 
to which some more changes are necessary.
(I split the patch into two for easier comparison.)

Combined majo patch is in the above uploadr: https://bugzilla.mozilla.org/attachment.cgi?id=593713

But anyway, I will wait for the comment for the error message function as noted above.

TIA

Comment 67

[ISHIKAWA, Chiaki]

Attachment: 1st patch slightly cleaned up.

The comment mentioned as frivolous was taken out.
Followed by another patch (2-patch) as the second stage: it turns out there are
so many additions in the 2-patch. So dividing the patch may not have been that
useful :-(

Comment 68

[ISHIKAWA, Chiaki]

Attachment: 2nd patch that follows the 1st stage patch.

Second stage patch adds a few more helper function within the class (error message output for a few known cases), and many error-related processing.

Comment 69

[ISHIKAWA, Chiaki]

I uploaded the 1st stage and 2nd stage patch after downloading the latest nsHelperAppDlg.js from comm-central and compare it against my local copy. 
Hope this helps.

TIA

Comment 70

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

Hello Chiaki-san, I'm sorry I haven't been able to provide feedback on this yet.

Can you perhaps re-summarize the current state of things, and clean up the attachments list by marking the now-obsolete patches as "obsolete", and then request review on the latest patch? I will try to get you some feedback (either from myself or someone else).

Comment 71

[:aceman]

It looks like only the newest patches (attachment 601242 [details] [diff] [review] and attachment 601243 [details] [diff] [review]) should be valid for review. I am not sure about attachment 593720 [details] [diff] [review]. Ischikawa, can you look at it and mark old patches as obsolete (in the Details link of each attachment).

Comment 72

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #71)
> It looks like only the newest patches (attachment 601242 [details] [diff] [review]
> [review] and attachment 601243 [details] [diff] [review]) should be valid
> for review. 

Thank you for the review.
The above is correct!

>I am not sure about attachment 593720 [details] [diff] [review].
> Ischikawa, can you look at it and mark old patches as obsolete (in the
> Details link of each attachment).
593720 is just to show the manual diff output and is not necessary. (The usually useful bugzilla diff output is 
a little confusing 
thus I created the manual output just to show the difference in
a neat manner.)

So I am obsoleting the old attachment(s).

Comment 73

[ISHIKAWA, Chiaki]

I obsoleted three previous patches.
Bugzilla interface was not very clear how to make an attachment obsolete (I remember seeing the reference to "obsolete" only when I posted an attachment and was asked to make previous attachments obsolete, and I am afraid that I could have done the changes in one go instead of obsoleting one attachment at a time. Sorry for the noise :-(

Comment 74

[:aceman]

I think you did it right now, there was no other choice currently.

Yes, when adding a new attachment you can obsolete many older ones in one go.

Comment 75

[ISHIKAWA, Chiaki]

Comment on attachment 601242 [details] [diff] [review]
1st patch slightly cleaned up.

This is the first patch in the two staged patches (split in two for easier
understanding.)

Helper functions for better error output, and the change in the 
flow especially for addition of try{} catch {}.

The second patch will add the details to the catch clause.

Comment 76

[ISHIKAWA, Chiaki]

Comment on attachment 601243 [details] [diff] [review]
2nd patch that follows the 1st stage patch.

A few more top-level addition of helper functions and
the details of additional error handling in catch{} clause.

TIA.

Comment 77

[ISHIKAWA, Chiaki]

OK, the problem is still there with 15.0.1 under linux.
(I have no idea why not many people complain who use TB under Windows. It seems that, unless the file system is on a remote CIFS or Samba sever with strict ACL setting, this problem may not show up under windows. Many users probably have administrative privilege like me on a local PC.)

Any chance of seeing this land in the comm-central tree before the next release?

TIA

Comment 78

[:aceman]

Well, gavin sharp has not made the review of the patch. Any way to nag him about it?

Comment 79

[ISHIKAWA, Chiaki]

Will try, but not sure how to do it :=)

Comment 80

[:aceman]

You can send him a personal email:)
Or add a new reviewer for Toolkit from here:
https://bugzilla.mozilla.org/show_bug.cgi?id=567585

Comment 81

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #80)
Sent an e-mail, and will wait for his response. If he seems too busy, then I will go for option B.
TIA

Comment 82

[ISHIKAWA, Chiaki]

FF users, please try the following procedure to experience this bug.

In your preference, mark "Always ask me where to save files" 
and try downloading an archive from sourceforge.org.
When asked, specify a write-protected directory and see
that the saving silently fails.

---

There was a question about how this problem noticed in Thunderbird
could be reproduced in FireFox.

I answered that it would be easily reproducible ASSUMING that
Firefox DOES USE this function defined in nsHelperAppDlg.js.

I tinkered with the binary download of FF for a while.
But it was not quite clear where nsHelperAppDlg.js is invoked in FF.
(I simply tried the obvious "Save Page as", but it did not seem to
invoke the routine(s) in nsHelperAppDlg.js).

This weekend, I downloaded FF source tree from
mozilla-central, and checked by compiling FF under linux.  And
tinkered with nsHelperAppDlg.js and tried to see where it is invoked.

Now, my tentative conclusion.

Firefox no longer uses nsHelperAppDlg.js for (the most often used ?)
"Save Page as" operation: For that it seems to use routines in
nsWebBrowserPersist.{h,cpp}.

Firefox DOES USE nsHelperAppDlg.js when Firefox downloads a page,
say, an archive from sourceforge.org, and your preference for
download location is "Always ask me where to save files" before
downloading begins, THEN nsHelperAppDlg.js IS INVOKED and the failure
reported here is observed in FF, too.

Complication / black magic  under the hood

If I specify write-protected directory, say, /tmp/d-dir as the
default save location AND unmark "Always ask me where to save files",
then what happens? 
Do I get a error message?
No I do not. 

Instead, when the real download begins the offered directory to save
is automagically reverted to my default Download directory under my
home directory. Presumably unless users tinkered with it, usually
built-in default directory is write-enabled, 
and so writing will succeed! 

FF checks for the writability of the directory specified by the user,
and if it is not writable, reset the specified directory to the
built-in Default. There is a code to do this. I was not sure where
this would be meaningful/useful, but now I know.

My guess is that only developer-types bother to change the default
setting or mark "Always ask me where to save files", and thus not much
complaint from FF users about the bug here.
(And EVEN I have not changed this, and usually save file to a default
directory and then copy/move it to the final destination.)

And, again, under Windows, it seems that only write-protected remote
server directory is truely unwritable(?). Under Windows 7, as a user
with administrative privilege, I could not make TB to observe the
write-protected characteristics of directory (set by
attrib or chmod from cygwin). That is why again, this problem is not
voiced much from Windows users, I think.
(Maybe users without administrative privilege may experience the problem? I have not bothered to check. This problem under linux is significant enough as it is.)


If I recall correctly, the users who reported the similar experience
were corporate linux users who got bitten when they try to write to
remote servers (where the directory permission may not be what you
assume it to be always: admin may change it from time to time.)

Details:

I tested whether the saving to a write protected directory of a web
page fails in FF.

[File] -> [Save Page as], and then I chose /tmp/d-dir which is
write-protected, and voila!  I got a proper error message indicating
that the writing could not be performed and choose a different
directory or change the permission of the directory. Good

The error message seems to be this:
./dom/locales/en-US/chrome/nsWebBrowserPersist.properties:accessError=%S could not be saved, because you cannot change the contents of that folder.\n\nChange the folder properties and try again, or try saving in a different location.

---

But where does the error message come from?

Looking for nsWebBrowserPersist, I found many hits in files 
nsWebBrowserPersist.{h,cpp}.
But no mention of it in the obvious files with "Download" in it.
I have a hunch, that nsWebBrowserPersist-related files are where the
message comes from.

So looking further for accessError in mozilla-central 
using mxr.mozilla, I found a usages in 
/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp 

    line 1075 -- msgId.AssignLiteral("accessError");

This turns out to be a usage in the following private method
http://mxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
//
// nsWebBrowserPersist private methods
//*****************************************************************************
// Convert error info into proper message text and send OnStatusChange notification
// to the web progress listener.
nsresult nsWebBrowserPersist::SendErrorStatusChange( 
     bool aIsReadError, nsresult aResult, nsIRequest *aRequest, nsIURI *aURI)

So where is this SendErrorStatusChange called?

There is only one usage:
        line 143 -- nsresult SendErrorStatusChange( 

Referenced in:

    embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
        line 3712 -- SendErrorStatusChange(false, rv, nullptr, aFile); 

And, this call is in :
    embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp 
        line 3686 -- nsWebBrowserPersist::SaveDocumentWithFixup( 

Now, my tentative conclusion.

Firefox does not use nsHelperAppDlg.js for ordinary
"Save Page as" operation.

Oh, wait, the following C++ file references  "PromptForSaveToFile". Maybe this
is where the javascript version of promptForSaveToFile is invoked
through IDL interface. But I am not sure how users can invoke this
function from UI of FF.

Referenced (in 3 files total) in:

    uriloader/exthandler/nsExternalHelperAppService.cpp 
        line 1987 -- nsresult nsExternalAppHandler::PromptForSaveToFile(nsIFile ** aNewFile, const nsAFlatString &aDefaultFile, const nsAFlatString &aFileExtension)
        line 2010 -- rv = mDialog->PromptForSaveToFile(this,
        line 2097 -- rv = PromptForSaveToFile(getter_AddRefs(fileToUse), leafName, mTempFileExtension);
        line 2107 -- rv = PromptForSaveToFile(getter_AddRefs(fileToUse), mSuggestedFileName, fileExt); 

To find out where nsHelperAppDlg.js is possibly invoked, I tried
downloading a few archives from sourceforge, and found that the
automatic saving invoked by accessing a download link invokes
nsHelperAppDlg.js. And, after a few changes to preferences, basically, mark "Always ask me where to save files", I can experience the same failure to warn the
user about the failing of writing to a write-protected directory in
FF. nsHelperAppDlg.js should be fixed!

[end of memo]

Comment 83

[Thomas D. (:thomas8)]

Gavin, I think this patch has been waiting for your feedback since March.

ISHIKAWA, chiaki, to write private mail to Gavin, pls use gavin -#at#- gavinsharp -#dot#- com.

Comment 84

[ISHIKAWA, Chiaki]

(In reply to Thomas D. from comment #83)
> Gavin, I think this patch has been waiting for your feedback since March.
> 
> ISHIKAWA, chiaki, to write private mail to Gavin, pls use gavin -#at#-
> gavinsharp -#dot#- com.

Just did.

Maybe we should wait until the holiday season is over.

A peaceful holiday season to everybody.

TIA

Comment 85

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

Comment on attachment 601242 [details] [diff] [review]
1st patch slightly cleaned up.

Chiaki - thank you for taking the time to prepare these patches, and for being persistent despite the delays. I'm apologize that I have not been responsive here. Generally the overall approach seems good, though the patches will need a good amount of "cleaning up" before they are ready to be committed. Here are some comments on this patch: 

>diff --git a/toolkit/mozapps/downloads/nsHelperAppDlg.js b/toolkit/mozapps/downloads/nsHelperAppDlg.js

>+// Lack of error messages for FILE I/O errors.

This comment is a little verbose :) We generally rely on much of this context being stored in bugs rather than putting it in the code. It's pretty easy to identify the bug in which a line of code was added, and so the more detailed context can just live there. Similarly, if you want to get the idea across during the review comment, you can just mention this stuff in the Bugzilla comment when attaching your patch, rather than including it in the patch.

>+// This I did after I tried to use nsIErrorService by re-defining
>+// NS_ERROR_GET_CODE() and NS_ERROR_GET_MODULE() in javascript anyway
>+// and failed.  It is attempted inside
>+// getErrMsgFromIErrorService(module_code, error_code).

As you've discovered, nsIErrorService is mostly not useful - nothing useful actually registers with the service, and there are no existing users, and so it's not really suitable for use here. We should really just remove it, since I don't think we're going to extend its use. I will file a separate bug on doing this.

>+// run grep NS_ERROR_FILE mozilla/xpcom/base/nsError.h | awk '{printf("// case  %s : msg = \"%s\" ; break; \n", substr($4, 1, length($4)-1), $2)}'
>+// to obtain the template for the following code.

These instructions seem out of date now... looks like this stuff is in xpcom/base/ErrorList.h now?

>+function myownIOErrorString(code) {

This helper function seems useful, and would be good to put in a JS module. Maybe in FileUtils.jsm.

>+function getErrMsgFromIErrorService(module_code, error_code)

As mentioned, I think you should just remove this from the patch.

>+function mydump(msg)

As Paolo mentioned a while ago, this should also be removed.

>+    // We enclose the processing to catch (previously uncaught) errors.

I think we need to be careful to only wrap the sections that would potentially throw the errors we're concerned about in the try/catch. The existing patch just puts a whole big block of code in the try/catch, and that can be confusing.

>+    catch (ex)  {  // Coping with I/O errors: We had an error exception.
>+                   // Show an error message.

This code should probably be simplified to just be:

let errorMsg = FileUtils.getMessageFromResult(ex.result);
this.displayError(errorMsg);

or somesuch.

Comment 86

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

Comment on attachment 601243 [details] [diff] [review]
2nd patch that follows the 1st stage patch.

>diff --git a/toolkit/mozapps/downloads/nsHelperAppDlg.js b/toolkit/mozapps/downloads/nsHelperAppDlg.js

Similarly, I think this patch will need a lot of the "meta" comments removed to be landable. It probably also makes sense to combine the two patches at this point, since they're pretty closely intertwined.

>+  displayOtherErrorAlert: function(msg) {

>+    prompter.alert(this.dialog,
>+                   "I/O Error (saving a file or an attachment)", // Shown in window frame/pane
>+                   msg);

This will need to be a localized string, similar to how displayBadPermissionAlert does it.

>         // Do not store the last save directory as a pref inside the private browsing mode
>+        // (This condition is checked  inside gDownloadLastDir.setFile.)
>         gDownloadLastDir.setFile(aLauncher.source, newDir);

This comment should probably just be removed.

One thing that might also be useful is to focus on simplifying the patch, where possible. We don't necessarily need to be exhaustive about detailing the specific reasons for the failures in all cases, as long as we have suitable default behavior for handling "generic" failures. Depending on how likely these cases are hit in practice, it may be better to take a simpler approach.

Let me know if you have any questions about how to proceed, given these comments.

Comment 87

[ISHIKAWA, Chiaki]

Dear Gavin,

Thank you for taking a look at these patches.

The patches have accumulated these "meta" comments over the months while I was
investigating the code (not only in this patch), but other issues such as
nsIErrorService (at one point it was suggested that maybe I could use it to
obtain meaningful error messages, but turned out be useless as is today, etc.).

Yes, if the general consensus seems to favor ditching nsIErrorService, I am more than happy to do it :-)

I will work on the cleaning up of the patch in the next few weeks.
I believe the general logic of the patch itself is OK since I have been using the locally modified version for quite some time now.

TIA

Comment 88

[:aceman]

Great that we finally have some life here ;)
It is interesting how much code this fix needs.
Thanks for working on this Chiaki. When you have a merged/cleaned up patch I can test it on Linux too.

Comment 89

[ISHIKAWA, Chiaki]

Attachment: A work-in-progress patch

I am obsoleting the previous two stage patches, and produced a simpler patch today.

It is still work in progress and I have not been
able to include all the suggested changes yet.

But I have created the patch against the
comm-central's latest (only a few days old) mozilla subdirectory original.
(This has been tested manually and by running "make mozmill" of TB).

We can base the discussion in the future on this patch.

TIA

Comment 90

[ISHIKAWA, Chiaki]

Attachment: A work-in-progress patch (slightly improved)

Slightly improved patch.

Comment 91

[ISHIKAWA, Chiaki]

>Created attachment 705863 [details] [diff] [review] [diff] [review]
>A work-in-progress patch (slightly improved)

My explanation.

The new modeline with "javascript". --- I could not get it to work as expected, so during the editing reverted to the old C++ mode line. That is not correct, but it works... I will revert it in the end when other parts of the patch are cleaned up.

First comments to Comment 85 :

>>+// Lack of error messages for FILE I/O errors.
>
>This comment is a little verbose :) We generally rely on much of this context >being stored in bugs rather than putting it in the code. 

Yes, I know it is verbose. I tried to cut it down. I will do so more.
I am afraid this is the generation gap: I would prefer to leave as much information in the code itself since network access may not be always available.
I suppose this is the different attitude of "before/after the web" generations.

>>+// This I did after I tried to use nsIErrorService by re-defining
>>+// NS_ERROR_GET_CODE() and NS_ERROR_GET_MODULE() in javascript anyway
>>+// and failed.  It is attempted inside
>>+// getErrMsgFromIErrorService(module_code, error_code).

>As you've discovered, nsIErrorService is mostly not useful - nothing useful >actually registers with the service, and there are no existing users, and so >it's not really suitable for use here. We should really just remove it, since I >don't think we're going to extend its use. I will file a separate bug on doing >this.

OK, I took it out. nsIErrorService is a premature good idea which not many people used then.

 
>>+// run grep NS_ERROR_FILE mozilla/xpcom/base/nsError.h | awk '{printf("// case  %s : msg = \"%s\" ; break; \n", substr($4, 1, length($4)-1), $2)}'
>>+// to obtain the template for the following code.

>These instructions seem out of date now... looks like this stuff is in >xpcom/base/ErrorList.h now?

Right. Updated the comment though once the routine is created, how this is produced is irrelevant. But again, the source of the constants ought to be
clearly visible, and so I updated the comment and made sure that the proper template could be generated.

>>+function myownIOErrorString(code) {
>
>This helper function seems useful, and would be good to put in a JS module. >Maybe in FileUtils.jsm.

I believe that this function (and extended ones to retrieve a short English
message from system error codes ) would be very useful.
I am not sure which FileUtils.jsm file you mention.
I checked and there are two files with such name under comm-central. 
I am not sure if FileUtils.jsm is the right choice, but I have no idea about this. Anyway, the function is there and so can be moved to another file later and
nsHelperAppDlg.js can be modified accordingly.

My first priority right now is to get this patch out to fix this really nasty bug.

>>+function getErrMsgFromIErrorService(module_code, error_code)
>
>As mentioned, I think you should just remove this from the patch.

Did so.

>>+function mydump(msg)
>
>As Paolo mentioned a while ago, this should also be removed.

Well, for this function, I enclosed it in #if 0/#endif pair.
This is because I found this function SO useful in debugging javascript.
(I am not sure what is the status of javascript debugger right now. During
my debugging efforts I saw a debugger getting out of sync with the thunderbird development and dumping variables here and there was the most vialble method unfortunately and mydump() here proved its worth. 
The particular information about the output not visible easily was not documented
in web pages very well. So I think this function has the merit of
staying here within "#if 0/#endif" block.

Well, of course, we can possibly remove this and leave a reference to a URL, but
again I am from the "before the web" generation.

>>+    // We enclose the processing to catch (previously uncaught) errors.
>
> I think we need to be careful to only wrap the sections that would potentially > throw the errors we're concerned about in the try/catch. The existing patch >just puts a whole big block of code in the try/catch, and that can be confusing.

I agree with you in general.
Basically, all I did was to make sure that, where relevant errors are thrown,
I insert try/catch construct so that the error is caught and examined.
I am throwing non-handled error immediately and so there should be no clipped error handling functionality IMHO.
The problem with the old code and its coding style was we never could tell what errors, if any, were thrown. By using try/catch and verbose comment, I hope that
there is now enough documented clue to what is going on in this file (at least
for the interactive saving operation. I still don't understand what goes on
ONCE the save button is hit.)

If you can tell me which try/catch is offensive I can possibly modify it one by one.


>>+    catch (ex)  {  // Coping with I/O errors: We had an error exception.
>>+                   // Show an error message.

>This code should probably be simplified to just be:
>
>let errorMsg = FileUtils.getMessageFromResult(ex.result);
>this.displayError(errorMsg);
>
>or somesuch.

I tried to modify the patch in this direction.
Right now, I simply moved the code that displays the error messages out of
the  function, |PromptToSaveForFile| and names it as "display_error_my_own" and pass the exception to it.
How unimaginative. But this is a work-in-progress patch. 
We can massage these routines into something that interfaces with functions in
an external file (maybe FileUtils.jsm) in the future.

Now the comments to Comment 86:


>>+  displayOtherErrorAlert: function(msg) {
>
>>+    prompter.alert(this.dialog,
>>+                   "I/O Error (saving a file or an attachment)", // Shown in window frame/pane
>>+                   msg);

>This will need to be a localized string, similar to how >displayBadPermissionAlert does it.

I am not sure of the proper steps to do this. I hope this won't take a long time, but I recall reading about the freezing of string bundle before releases.
Anyway, tips about how to get such message localized will be appreciated.
(I do hope, though, this won't delay pushing this patch out much longer.)

>>         // Do not store the last save directory as a pref inside the private browsing mode
>>+        // (This condition is checked  inside gDownloadLastDir.setFile.)
>>         gDownloadLastDir.setFile(aLauncher.source, newDir);
>
> This comment should probably just be removed.

At least, I took my addition here.

> One thing that might also be useful is to focus on simplifying the patch,  >where possible. We don't necessarily need to be exhaustive about detailing the >specific reasons for the failures in all cases, as long as we have suitable >default behavior for handling "generic" failures. Depending on how likely these >cases are hit in practice, it may be better to take a simpler approach.

Again, in general terms I agree with you.
However, I had too much time on my hands while this patch lingered on in bugzilla. The more I looked at the original code, the more problems I found with it and this is why the patch has bloated to this stage.
The current processing is what I would have for a semi rock-solid mail-client (although I would have done away with the superficial check for writability in advance since this is race-prone and not so much useful.)
Also, I used the term "semi" because I found so many places in the code base where the return value of lower-level function is not checked :-(
This lack of checking of return value (or in this particular file, the failure to catch the error thrown) will haunt TB for years, but I digress.

Anyway, here is the much streamlined patch along with your suggestion although
it still has verbose comment and functions are not quite polished yet.

TIA

Comment 92

[ISHIKAWA, Chiaki]

Additional comment:
Because of my patch was created long time ago, and my current 
source tree is a few weeks old, I may have screwed up when I tried to modify the
file using more up-to-date source tree on a different PC.

The patches near
(old line) 373 - (new line) 847,
517            -           1003,
537            -           1013,

may not be quite right. I will look into them.

TIA

Comment 93

[ISHIKAWA, Chiaki]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #85)
> Comment on attachment 601242 [details] [diff] [review]
> 1st patch slightly cleaned up.
> 
> Chiaki - thank you for taking the time to prepare these patches, and for
> being persistent despite the delays. I'm apologize that I have not been
> responsive here. Generally the overall approach seems good, though the
> patches will need a good amount of "cleaning up" before they are ready to be
> committed. Here are some comments on this patch: 

Dear Gavin and all,

I am cloning this bug to another one:

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 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.)
//
// (B) Use of of myownIOErrorString() for better I/O error reporting
// other than access permission issues.
//
// (C) Caution about the racy-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 operation, and may be confusing.
---------


Thank you for your patience to cope with these comments and the fluid patches, but
the core issue of the failure to warn the user when the saving of an attachment to 
a write-protected directory is so serious that it warrants FIXING NOW IMHO.
(Actually, I put my modified nsHelperAppDlg.js in MY OWN copy of linux for a while to cope
with the problem, but it has become too much bother to do so due to the fundamental change in the distributed code.)

So I said to myself even if the distributed code addresses only 80 % of my concern, it should be
distributed widely.

TIA