120380 - needsterminal flag in mailcap must be respected

Status: NEW

(Firefox :: File Handling, defect)

Description

[robbe]

Some handlers in my mailcap file output to stdout, i.e. they need to be run
inside an X terminal. Example:

  text/*; less '%s'; needsterminal

Mozilla 0.9.7 (Linux) just runs the command passing on whatever stdout it might
have itself (often a log file or /dev/null)

I think the 'x-mozilla-flag' is already supported, so grokking another one
should not be too difficult. Sticking "xterm -e" before the respective command
should be neither. (Actually the exact terminal to use would better come from a
pref.)

Comment 1

[Christopher Aillon (sabbatical, not receiving bugmail)]

install the psm package (via installer or download the mozilla-psm.rpm)

*** This bug has been marked as a duplicate of 47617 ***

Comment 2

[sairuh (rarely reading bugmail)]

v

Comment 3

[Boris Zbarsky [:bzbarsky]]

reopening.  Caillon, what does this have to do with PSM?

Comment 4

[Boris Zbarsky [:bzbarsky]]

I've been meaning to do this anyway.  As you note, parsing for the flag is
trivial (that system is already in place).  The hard part is finding which
terminal to use....

I suppose we could look for "xterm", "eterm", "gterm", "kterm" in PATH?

Targeting to 1.0, but I don't really have the time to figure out how to do this
in a decently XP way.  Whichever method we use, it should work reasonably on all
Unices, BeOS, QNX, and VMS (these are the platforms that use the
mailcap/mime.types code).  Granted, BeOS should be using a separate module
anyway.... [e-mail], any help you can offer in defining the correct
behavior would be much appreciated.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Ummm, weird....  when I duped this the bug had something about unknown socket
type errors.  Bugzilla has been displaying bugs incorrectly today (as bkor and
rkaa both have seen in #mozillazine).

Comment 6

[tenthumbs]

Wouldn't it be better to look at the TERM environment variable?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yes, it would.  What do we do when it's empty?

The final default, of course, is that we simply ignore the needsterminal
directive... or that the mailcap entry does not get counted as "matching". 
Which is better?

Comment 8

[tenthumbs]

The more I think about it the more I realize that environment variables 
won't work. TERM might say xterm even for rxvt, etc. I think mozilla 
will have to have a pref with xterm -e as a default.

You might be able to shoehorn this in as a "helper app." That's what 4.x 
does. See the entries for telnet, etc.

Comment 9

[Boris Zbarsky [:bzbarsky]]

TERM is completely out.  sshing into another machine will set TERM to vt100,
which is utterly divorced from what we want.

I would rather have mozilla do "the right thing" without user interaction as
much as possible, but allow users to override.  So if we have a pref it would
simply be an override for the codepath used by default and would be unset in new
profiles...

"The right thing" is starting up _some_ terminal with the app running inside. 
So we want to walk the PATH and look for commonly-used terminal apps.

I don't really like this solution, though... any other ideas?  :)

Comment 10

[tenthumbs]

Walking the PATH is treacherous.

First, there's no reason to assume that mozilla's PATH is the same as 
the user's. You might find the wrong version of some program or not find 
what the user wants.

Then there's the question of actually walking it. There's no library 
function that let's you find a file. You either have to do an execvp (or 
one of its relatives) and deal with failures or you have to roll your 
own code. (See my latest attachment to bug 57866 for one way to do it. 
It's not as easy as it looks.)

Personally, I see nothing wrong with a default of "xterm -e" that can be 
overridden by the user. Aren't user prefs applied on a per-profile basis 
anyway?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Walking the PATH is treacherous.

That's why I dislike my solution.  :)

Unfortunately, there is no good way to do the "xterm -e" solution without
running into the same problems...

Let me put it this way.  To start up an application from within mozilla in a
reasonable way I need two things:

1)  A full path to the application
2)  A char** of arguments to pass to the application

So for "xterm -e" these would be "/usr/X11R6/bin/xterm" and ["-e", handler]

The problem is storing this info in the prefs.  Storing full path is easy, but
this will vary by platform.  Storing switches is not....

Both xterm, rxvt, gnome-terminal use -e for starting up an app different from
the shell.  I have no other terminals here to test.  What do eterm/kterm use? 
Would assuming that it's always "-e" be valid enough to implement (possibly as a
separate pref)?

Comment 12

[tenthumbs]

Why do you need absolute paths? The location of any executable is 
system-dependent and changeable. You can't assume that someone won't 
change the location of a given file. That's why PATH exists in the first 
place. As I said before, there are standard ways to do that.

I don't see the problem with prefs either. You can store an arbitrary
string in one and parse it any way you like.

I suspect that most xterm equivalents honor -e because xterm does. Now 
that I've thought on it a little more, it too simplistic to say that 
xteem -e will suffice. Many users find they can't read an default xterm. 
The fonts are too small/large. The window's the wrong size. It's the 
wrong color, etc.

BTW, you should consider passing your own environment on an exec.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Why do you need absolute paths?

Because the accepted way to spawn off subprocesses in Mozilla is nsIProcess,
which uses an nsIFile, which needs an absolute path.  So at some point I need an
absolute path.

> That's why PATH exists in the first place.

Right.  Which is why I would need to look for the executable in the PATH just
like, say, the shell would.  See the code in 
uriloader/exthandler/unix/nsOSHelperAppService.cpp that already does this for
mailcap stuff...

> You can store an arbitrary string in one and parse it any way you like.

Sure.  Until someone comes along and assumes they can stick arbitrary shell
stuff in that pref.... :)  And no, code that uses system() is _not_ likely to
pass review.

I would rather not do a full tokenizer and parser to break up things like

"xterm -title 'foo bar baz' -geometry 500x30+20+20 -display :1.0 -e"

into things I can pass to the system.

> Many users find they can't read an default xterm. 

Isn't that what X resources are for?  So that all that info does not have to be
respecified on the command line by every friggin' thing that wants to use an xterm?

Comment 14

[robbe]

My opinion is that walking the PATH trying to find one of a set of popular X
terminal emulators is probably still your best bet. Since the mailcap code
already has to find absolute paths for the real handler applications (cf. my
example which specifies "less" not "/usr/bin/less") this should not be too much
work.

Providing pref overrides is a must because if more than one emulator is
installed, the one we end up picking will invariably be the one some users hate.
Of course, if more distributors would adopt Debian's "x-terminal-emulator"
symlink ...

I'm sure most emulators support "-e" for compatibility, but there may be older
apps around that don't (CDE, SunView, OpenLook, whatever). So another pref for
the correct switch may be in order. Default: "-e". Finally, I agree with Boris
that providing for arbitrary switches for geometry etc. is too much. xterm
supports resources well, and many modern terminals have persistent preference
settings themselves. If all else fails, users should write a short shell wrapper
and use that.

Comment 15

[tenthumbs]

Boris: where does mozilla spawn a process? If there's no way to use a
well-known library call like execvp then that's a bug. This is all
unix-specific code and it should be as efficient as possible.

I agree that parsing strings is a waste of time. System() _would_ be
the quickest solution but I know why people dislike it. You could do
something like this:
  const char *shell = PR_getenv("SHELL");
  if (!shell) shell = "/bin/sh";
  Argv[0] = shell;
  Argv[1] = "-c";
  Argv[2] = "exec xterm -random_args ...";
which handles all the shell stuff, but it *is* sleazy.

You're gonna hate me but I think the PATH stuff in
nsOSHelperAppService.cpp is buggy. I'll file a bug and assign it to you.

As for X resources, I don't think you can assume that resources for
user-initiated apps are always applicable to mozilla-initiated ones,
e.g. the resource equivalents of -sl, -tn, -wf, or -ziconbeep. Since
there are command line equivalents for all resources, the user may be
using command line args and the resources may well be suffering bitrot.

robbe: if you're going to require the user to write wrapper scripts in
certain circumstances then it's a simple step to require wrappers in all
circumstances and not handle needsterminal at all. :-)

Comment 16

[Boris Zbarsky [:bzbarsky]]

> where does mozilla spawn a process?

First you Init() it:
http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsProcessCommon.cpp#66

Then you Run() it :
http://lxr.mozilla.org/seamonkey/source/xpcom/threads/nsProcessCommon.cpp#194

The code is pretty heavily macro-ized and #ifdef-ed

> If there's no way to use a well-known library call like execvp

I didn't say there is no way.... just that it requires more work to make sure
it's right.  I don't know all that would go into properly forking and exec-ing a
multi-threaded program on various unix flavors.  I don't even know if it's any
different than forking/exec-ing anything else.

> This is all unix-specific code and it should be as efficient as possible.

It's not exactly unix-specific.  This code also runs on VMS and others. 
Basically, all mozilla platforms other than Win32/MacOS/OS2 use this code.

> You're gonna hate me but I think the PATH stuff in nsOSHelperAppService.cpp is 
> buggy.

Why would I hate you?  It was a pretty quick hack at the time....  bugs on that
are welcome.  I _do_ think we should have all that in a library somewhere, though.

There are basically two parts to this bug:

1)  Add a flag to nsIMIMEInfo that indicates that a terminal is needed, parse
    for "needsterminal", and set the flag.  This part I can do easily.
2)  When spawning the helper, if the flag is set do something special.  This is
    the part we're discussing, and any help on this (including contributed or
    example code) would be much appreciated.  I just don't have all that much
    time to spend on this, unfortunately.  :(  But I _would_ like it to happen,
    for my own use.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Not happening until I finish thesis unless someone steps up and does it.

Comment 18

[Nathan Stratton Treadway]

This is really a significant issue: without a terminal, running "less" as a
helper app doesn't do anyone any good (and can cause trouble, since the process
is running without any way to communicate with the user).

It would be great if Mozilla could be configured to spawm a terminal to run this
sort of helper (as discussed above) -- but in the meantime Mozilla should just
ignore "mailcap" entries with the "newsterminal" flag.  That way, it would skip
the entry for "less" and move on to some X-aware application that can handle the
display on it's own (i.e a later mailcap entry that doesn't have "needsterminal"
set).  Hopefully, that solution is a lot easier to implement.

Nathan

Comment 19

[Boris Zbarsky [:bzbarsky]]

Actually, now that bug 78919 is fixed this might not be too bad to do (just
hardcoding xterm for the time being).  You're right that just having Mozilla
skip entries that have the needsterminal flag would be pretty simple too; if
someone is interested in implementing that I can point them to the code in question.

Comment 22

[Mike Hommey [:glandium] OoO until aug 27]

Attachment: skip needsterminal entries

While I was digging the mailcap handling code for some other issue, I saw this could be done very easily.

Comment 23

[Vincent Lefevre]

Bug 296199 has been marked as a duplicate of this bug, but fixing bug 57342 would also be a solution (and even better in this particular case of a text file). Now, I think these bugs are closely related.

One problem is that mailcap is used for different things. For instance, for a MUA with a text interface, something like

  text/*; less '%s'; needsterminal

is useful. Now, Firefox doesn't need a terminal to display some text: it could treat it like text/plain (possibly with the way to apply some filter in order to convert it into HTML/XML + CSS to benefit from coloring and so on). So, by default, instead of looking for a terminal, it could use its own window. Of course, some file formats need a conversion before they can be displayed; this is handled in 3 different ways in a mailcap file:

1. An entry with often a test like
    test=test -n "$DISPLAY"
No problem for Firefox: it can execute the application without needing a terminal...

2. An entry with the copiousoutput flag. In such a case, Firefox should run the tool and display the output in its own window.

3. An entry with the needsterminal flag, generally using an interactive text application (e.g. a pager like "less"). In such a case, Firefox needs a terminal.

Comment 24

[Boris Zbarsky [:bzbarsky]]

So...  That doesn't actually fix this bug as filed (which is about running such commands in a terminal, not skipping them completely).

I agree that it's an improvement over the current behavior, but it should really go in a different bug.

Also, the indentation in the patch is clearly off (tabs?) and it would make it a lot easier to review if it had at last 8 lines of context.

Comment 25

[Reed Loden [:reed]]

Oh, I just saw bz's comment... Mike, would you mind opening a new bug for your patch, as per bz's request? That will allow this fix to be reviewed quicker, and we can get it in the tree sooner. Thanks!

Comment 26

[Reed Loden [:reed]]

... oh, and thanks again for the fix. Really appreciate it! I had been wondering why "less" and other things were being proposed to me as open with handlers. ;)

Comment 27

[Mike Hommey [:glandium] OoO until aug 27]

Filed bug #442629