556369 - Choosen helper application not displayed in "Opening ..." dialog window

Status: NEW

(Toolkit :: Downloads API, defect)

Description

[Manuel Reimer]

Attachment: Patch to fix this bug

In SeaMonkey, the unchanged "Opening ..." dialog (unknownContentType.xul) is used (I don't know if this is also the case for Firefox).

On my Linux system, no "default applications" are found. Perhaps as I don't use a mailcap file and/or as I'm not using gnome.

If I select a helper application, then the "Browse..." button hides and the name of my helper application is shown.

As soon as I open a file of the same type, some time later, I get the "Browse..." button again. If I don't change anything (keep the "Browse..." button where it is), then my previously selected helper application is used, but I expect the name of the helper application to be visible here.


The old code had a mix of "hide handler menulist" and "show handler menulist". My code changes this to *one* "hide handler menulist" (which is the default status, now) and switches it to visible whenever it is required.

Comment 1

[:Paolo Amadini]

Comment on attachment 436285 [details] [diff] [review]
Patch to fix this bug

> The old code had a mix of "hide handler menulist" and "show handler menulist".
> My code changes this to *one* "hide handler menulist" (which is the default
> status, now) and switches it to visible whenever it is required.

It seems like a sensible approach, a little step towards making this code more linear.
I have a couple of suggestions and should point out a few issues with the patch:

>     initAppAndSaveToDiskValues: function() {
>+
>+      // Hide "handler" menulist by default.
>+      this.dialogElement("modeDeck").setAttribute("selectedIndex", "1");
>+
>       var modeGroup = this.dialogElement("mode");

I suggest you just add the attribute to the XUL file. The rest of the code makes
assumptions on the initial state of the controls, so it should be fine to simply
change the initial state.

The issue with this is, apparently, that the code inside this "if" condition...


      if (this.mLauncher.targetFileIsExecutable || (
          (mimeType == "application/octet-stream" ||
           mimeType == "application/x-msdownload") && 
           !openWithDefaultOK)) {


...assumes that the menulist is initially visible.

However, on closer inspection it seems that hitting this code path (which shows a
user interface with a disabled "open" choice) is improbable, if not impossible. For
executables, in fact, we normally use the "basic" version of the user interface:


      var shouldntRememberChoice = (mimeType == "application/octet-stream" || 
                                    mimeType == "application/x-msdownload" ||
                                    this.mLauncher.targetFileIsExecutable);
      if (shouldntRememberChoice && !this.openWithDefaultOK()) {
        // ...... show basic UI ......
      }
      else {
        this.initAppAndSaveToDiskValues();


A bit of boolean expression simplification shows that the not-basic-but-open-disabled-UI
is only shown when targetFileIsExecutable AND openWithDefaultOK are BOTH true. Weird!
On Windows, this is impossible; on other platforms, digging through the code reveals
that targetFileIsExecutable is generally false (except rarely, undeterministically,
on OS2) as the executable permission bit is not set on the downloaded files.

Long story short: if Shawn, that originally asked me to look at this patch, agrees that
the not-basic-but-open-disabled-UI is unneeded, just remove that code instead of fixing
it (but ensure that we use the "basic" user interface in that case).

>       } else if (this.mLauncher.MIMEInfo.preferredAction == this.nsIMIMEInfo.useHelperApp) {
>         // Open with given helper app.
>         modeGroup.selectedItem = this.dialogElement("open");
>         openHandler.selectedIndex = 1;
>+        // Show the "handler" menulist since we have a (user-specified)
>+        // application now.
>+        this.dialogElement("modeDeck").setAttribute("selectedIndex", "0");

The initAppAndSaveToDiskValues function has two parts, one that determines the visibility
of the controls and one that selects the appropriate control depending on the previous
user choices. This is the second path; the visibility change should go in the first part
instead.

If you have any question feel free to ask :-)

Comment 2

[Manuel Reimer]

Attachment: Second patch

- Moved default selection index to XUL
- Dropped duplicated code for case of executable files

(Comment #1)
> The initAppAndSaveToDiskValues function has two parts, one that determines
> the visibility of the controls and one that selects the appropriate control
> depending on the previous user choices. This is the second path; the
> visibility change should go in the first part instead.

For me this whole code is just complicated. I can't see any logical separation into "parts" there. Please tell me, what exactly I have to change.

Do you think I should commit a second patch with useless trailing whitespaces removed? This code has many of them and maybe it would be a good idea to get rid of them. I configured Emacs to highlight trailing whitespaces in red and in this code file, I have much red stuff...

Comment 3

[:Paolo Amadini]

Comment on attachment 437044 [details] [diff] [review]
Second patch

(In reply to comment #2)
> For me this whole code is just complicated.

Yes, it is much more complicated than it could be :-)

> Please tell me, what exactly I have to change.

>       } else if (this.mLauncher.MIMEInfo.preferredAction == this.nsIMIMEInfo.useHelperApp) {
>+        // Show the "handler" menulist since we have a (user-specified)
>+        // application now.
>+        this.dialogElement("modeDeck").setAttribute("selectedIndex", "0");

Move this code inside this "if" block instead:

      if (this.chosenApp && this.chosenApp.executable && 
          this.chosenApp.executable.path) {

This condition is about the availability of a user-chosen helper application,
while the other is about the choice of using that application by default.

>       // If we don't have a "default app" then disable that choice.
>-      if (!openWithDefaultOK) {
>+      if (!this.openWithDefaultOK) {

openWithDefaultOK()

>       var mimeType = this.mLauncher.MIMEInfo.MIMEType;
>       var shouldntRememberChoice = (mimeType == "application/octet-stream" || 
>                                     mimeType == "application/x-msdownload" ||
>                                     this.mLauncher.targetFileIsExecutable);
>       if (shouldntRememberChoice && !this.openWithDefaultOK()) {

When this.openWithDefaultOK() and targetFileIsExecutable are both true,
we end up displaying the full user interface, with the ability to open
the executable directly from the browser (not on Windows, however). I
think this may be correct, however I'll ask for a second opinion just
in case.

> Do you think I should commit a second patch with useless trailing whitespaces
> removed? This code has many of them and maybe it would be a good idea to get
> rid of them. I configured Emacs to highlight trailing whitespaces in red and
> in this code file, I have much red stuff...

That would be nice! Just open a separate bug for it, and attach a patch
after this one has been checked in. We want to do that kind of changes
when there are no recent pending patches from other people for the same
file, as that might force them to regenerate their pacthes before check-in.

Comment 4

[:Paolo Amadini]

Manuel, can you provide a patch with the above issues fixed?

With regard to the rare case where openWithDefaultOK() and
targetFileIsExecutable are both true, showing the full UI is
probably fine. The not-basic-but-open-disabled-UI is actually
a leftover from an old bug (see bug 347230 comment 14).

Comment 5

[Manuel Reimer]

I'm currently working on my patch, but I've re-added the case for "targetFileIsExecutable" and "openWithDefaultOK". I now just disable the possibility to open with default application (which would allow to directly execute) if "targetFileIsExecutable" is true. This handles the last case with just a few code lines. A helper application for this type would still work, then.

If we want to simplify the code there, then this maybe should get another bug report to discuss about this and possible security problems. It would be *bad* if there is even a small possibility to accidently execute a file "from web".

My hope is to work out a patch, which may easily land on HG, so there is a chance this annoying bug is fixed in SeaMonkey 2.1. This evening, I'll attach another patch.

Comment 6

[Manuel Reimer]

Attachment: Third patch

Added all the fixes, you suggested, without one:

>>       // If we don't have a "default app" then disable that choice.
>>-      if (!openWithDefaultOK) {
>>+      if (!this.openWithDefaultOK) {
>
> openWithDefaultOK()

As I dropped the "var openWithDefaultOK = this.openWithDefaultOK;" in this function (only used once), the "this" is required here. IMHO there is no need to make things more complicated here as needed.

My try to fix the problem which *may* lead to a small security hole is in the "openWithDefaultOK" function. I added the "this.mLauncher.targetFileIsExecutable" for all "non Windows" platforms, so this function now does what it should: It tells us if it is a good idea to open the given file with the default application.

I did a short try with a freshly built SeaMonkey and the patch seems to do what it should. Maybe further tests are required to get sure this bug is fixed with this patch.

Comment 7

[Manuel Reimer]

Missed a small mistake...
https://bugzilla.mozilla.org/attachment.cgi?id=438550&action=diff#a/toolkit/mozapps/downloads/nsHelperAppDlg.js_sec6

This one:

if (!this.openWithDefaultOK) {

should be:

if (!this.openWithDefaultOK()) {
                           ^^
If required, I'll upload another patch with this mistake fixed, tomorrow.

Comment 8

[:Paolo Amadini]

(In reply to comment #5)
> I'm currently working on my patch, but I've re-added the case for
> "targetFileIsExecutable" and "openWithDefaultOK". I now just disable the
> possibility to open with default application (which would allow to directly
> execute) if "targetFileIsExecutable" is true. This handles the last case with
> just a few code lines.

Excellent, this preserves exactly the current behavior, and makes it clear.

> A helper application for this type would still work, then.

Only if the file isn't executable and, on non-Windows, has a default
handler; otherwise, the "basic", save-only user interface is shown. This
is consistent with the current behavior but I'm not sure if this is what
we want. See also the "NOTE" in bug 369431 comment 0.

> If we want to simplify the code there, then this maybe should get another bug
> report to discuss about this and possible security problems. It would be *bad*
> if there is even a small possibility to accidently execute a file "from web".

Yes, if we're changing the behavior of the user interface, we should be
careful and get feedback from the UI experts. I'd appreciate if you could
open a bug in order to fix the case above; in that case, link to it from
bug 369431 so that interested people can know what we're working on.

Comment 9

[:Paolo Amadini]

Comment on attachment 438550 [details] [diff] [review]
Third patch

(In reply to comment #7)
> If required, I'll upload another patch with this mistake fixed, tomorrow.

This patch looks fine, with the parentheses mistake fixed. You should
upload a new patch, but there's no need to request review from me again.

Since you have to create a new patch anyway, please fix these small
cosmetic issues while you're at it (we don't fix those on old code,
but generally we try to keep this style for new code in this area):

>+      // If choice shouldn't be remembered and opening with default isn't
>+      // OK (we don't let users open executable files for security concerns),
>+      // then set up simple ui

Dot at end of sentence (even if only one sentence or only one line).

>+            // On other platforms, default is Ok if there is a default app
>+            // and target file is not executable
>             // Note that nsIMIMEInfo providers need to ensure that this holds true
>             // on each platform.

Reduce indentation of the entire block, and add dot at end of sentence.

>-        // Hide the default handler item too, in case the user picks a 
>-        // custom handler at a later date which triggers the menulist to show.
>+        // Hide the default handler item in the "handler" menulist

As above.

Comment 10

[Manuel Reimer]

Attachment: Fourth patch

Fixed mistakes.

Comment 11

[:Paolo Amadini]

Comment on attachment 439021 [details] [diff] [review]
Fourth patch

Thank you Manuel, I think this is polished and ready for check-in. We just
need a second-pass review from Shawn, which is the real expert on this code
and originally requested my attention, as maybe I'm still missing something.

Comment 12

[:Paolo Amadini]

Final review for should come in a week or two. With regard to removing the
trailing whitespaces, we're opening another bug to do that plus re-indenting
the code, but probably we'll do that after your patch is checked in.

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 439021 [details] [diff] [review]
Fourth patch

For review comments with context, please see http://reviews.visophyte.org/r/439021/

on file: toolkit/mozapps/downloads/nsHelperAppDlg.js line 485
>       // If choice shouldn't be remembered and opening with default isn't
>       // OK (we don't let users open executable files for security concerns),
>       // then set up simple ui.

nit: the phrasing is a little off.  I'm suggesting this:
If the choice should not be remembered and opening with the default is not OK
(e.g. we do not let users open executable files for security reasons), then
setup the simple UI.


on file: toolkit/mozapps/downloads/nsHelperAppDlg.js line 684
>         // On other platforms, default is Ok if there is a default app and
>         // target file is not executable.

phrasing nit:  lets try this:
On non-windows platforms, the default is OK if there is a default application
and the target file is not executable.


on file: toolkit/mozapps/downloads/nsHelperAppDlg.js line 688
>         return this.mLauncher.MIMEInfo.hasDefaultHandler &&
>                !this.mLauncher.targetFileIsExecutable;

nit: we should store this.mLauncher in a local variable since we access it
twice here.


on file: toolkit/mozapps/downloads/nsHelperAppDlg.js line 700
>         // Show the "handler" menulist, since we have a (default)
>         // application now.

nit: not sure why you have parens around default.


on file: toolkit/mozapps/downloads/nsHelperAppDlg.js line 749
>         // Show the "handler" menulist, since we have a (user-specified)
>         // application now.

ditto here about parens around user-specified


r=sdwilsh

This should get a test though.  You can probably add to http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_dialog_layout.xul to accomplish this.

Comment 14

[Shawn Wilsher :sdwilsh]

(Paolo may be able to offer assistance on the test)

Comment 15

[:Paolo Amadini]

I'd be glad to help you with the test, which has to be updated before the
patch can be checked in. I think we can start with testing that we don't
cause regressions in the visibility of the controls under default conditions,
which can easily done with the existing infrastructure in
test_unknownContentType_dialog_layout.xul.

While developing it should be sufficient to run this test on its own.
For reference, I use this command line from the objdir:

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

If you need information while updating the test, feel free to ask!

Comment 16

[:Paolo Amadini]

Manuel, I just wanted you to know that we're proceeding with bug 559833
(fixing indentation on the file), probably you'll have to build the fifth
patch over the re-indented version.

Comment 17

[Manuel Reimer]

So I'll wait for this bug to get fixed and will continue after that, trying if my patch still works and changing it where needed.

I hoped to have this fix in SeaMonkey 2.1, as this really sucks on Linux to not see where files get opened with, but it seems to get pretty unlikely to get this into SM 2.1 as the whole thing seems to get more difficult as I expected at first, so waiting for the fix in bug 559833 is no problem.

As soon as possible, I'll try to add the missing changes, but maybe it would be a good idea if someone else would do the "test part", as I would have to learn anything about them from scratch. I never used them, so far (disabled in .mozconfig to speed up build), so I don't know how they are used and how they work.

Comment 18

[:Paolo Amadini]

(In reply to comment #17)
> As soon as possible, I'll try to add the missing changes, but maybe it would be
> a good idea if someone else would do the "test part", as I would have to learn
> anything about them from scratch. I never used them, so far (disabled in
> .mozconfig to speed up build), so I don't know how they are used and how they
> work.

I can definitely help in updating this test, which probably requires just
a small change, while you can begin learning about the testing framework
by running the test on your computer. A good start would be to make a
build with tests enabled, and skim through the documentation at
https://developer.mozilla.org/en/Mozilla_automated_testing and
https://developer.mozilla.org/en/Mochitest (for this test case).

Running all the tests takes a lot of time, but while you develop or test a
specific feature you can generally run the single test you are interested in.
For this test, it should be as simple as executing the command-line in
comment 15 (it is a single line, with a space between ".xul" and "make"
but not between "chrome" and "/").