Closed Bug 444440 Opened 11 years ago Closed 7 years ago

Unexpected application launched when $HOME/.mailcap contains an entry for the handled mime type

Categories

(Core Graveyard :: File Handling, defect)

1.9.0 Branch
x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla22

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Keywords: regression)

Attachments

(2 files, 6 obsolete files)

Attached patch patch (obsolete) — Splinter Review
When you have a handler defined in $HOME/mailcap for some mime type, the download/save-as dialog show you the proper application name, but when you select it and click on ok, the wrong one is launched. What is happening is that the GNOME handler for the mime type is being run, despite of what is set in $HOME/mailcap.

Is is kind of related to bug 423776, and it appears the fix there was not the right answer to the problem, and the underlying problem was not detected.

The problem is that nsMIMEInfoUnix is a mix between GNOME and mailcap entries, and that GNOME entries have some magic behaviour.

The attached patch just changes this by having mailcap entries be handled by "dumb" nsMIMEInfoImpl, while GNOME entries are handled by nsMIMEInfoUnix.

To be honest, with such a change, nsMIMEInfoUnix could just be renamed to nsMIMEInfoGNOME, but well, I didn't want a too intrusive patch.

At the same time, I do some cleanup in handlers for protocols, that just happened to work because of some side effects of the current code, which could have become prone to unexpected breakage with future changes.
Attachment #328749 - Flags: review?(cbiesinger)
The previous patch failed for protocol handlers where there is no handler app in prefs nor in GNOME registry, in which case a nsIHandlerInfo is still supposed to be returned.
Assignee: nobody → mh+mozilla
Attachment #328749 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #328766 - Flags: review?(cbiesinger)
Attachment #328749 - Flags: review?(cbiesinger)
Attachment #328766 - Flags: review?(bzbarsky)
So was the issue that nsMIMEInfoUnix::GetHasDefaultHandler was returning the GNOME handler instead of the mailcap one, even if the mailcap handler was what was used to set the description initially?

Probably one of dolske, sdwilsh, dmose should review this patch...
The problem is in nsMIMEInfoUnix::LaunchDefaultWithFile, which tries the gnome handler before fallback to mailcap entry. But trying mailcap entry first would only create a new issue: that /etc/mailcap would then take precedence over the gnome handler.

I think the hole thing should get a big overhaul, but the current patch is enough to get something at least working as users would expect it to work, without disrupting the code, with risks to introduce new bugs.
Duplicate of this bug: 443153
Comment on attachment 328766 [details] [diff] [review]
patch that actually passes the testsuite

>+++ b/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp
> nsMIMEInfoUnix::GetHasDefaultHandler(PRBool >+  if (mClass == eMIMEInfo) {
...
>+  } else {
>+    if (nsGNOMERegistry::HandlerExists(mType.get()))

Does mType actually mean something if we're a protocol info?  If so, can you document it on the member?

>+++ b/uriloader/exthandler/unix/nsOSHelperAppService.cpp
>+/* Returns 0 for no handler, 1 for prefs handler and 2 for GNOME handler */

I'd put the comment in the header, not next to the implementation.

>@@ -1662,19 +1668,21 @@ nsOSHelperAppService::GetProtocolHandlerInfoFromOS(const nsACString &aScheme,
>+  if (handlerType != 1) {

So either not found at all, or not found in prefs, right?  Why are we treating those two cases the same here?

>+    // We also don't need to SetDefaultDescription for nsMIMEInfoUnix

Why not?

Other than those nits, this looks good to me.  dmose should probably sr.
Attachment #328766 - Flags: superreview?(dmose)
(In reply to comment #5)
> (From update of attachment 328766 [details] [diff] [review])
> >+++ b/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp
> > nsMIMEInfoUnix::GetHasDefaultHandler(PRBool >+  if (mClass == eMIMEInfo) {
> ...
> >+  } else {
> >+    if (nsGNOMERegistry::HandlerExists(mType.get()))
> 
> Does mType actually mean something if we're a protocol info?  If so, can you
> document it on the member?

Protocol info is initialized with 
handlerInfo = new nsMIMEInfoUnix(aScheme, nsMIMEInfoBase::eProtocolInfo), which is no different than the nsMIMEInfoImpl constructor, which means mType gets aScheme.

> >+++ b/uriloader/exthandler/unix/nsOSHelperAppService.cpp
> >+/* Returns 0 for no handler, 1 for prefs handler and 2 for GNOME handler */
> 
> I'd put the comment in the header, not next to the implementation.
> 
> >@@ -1662,19 +1668,21 @@ nsOSHelperAppService::GetProtocolHandlerInfoFromOS(const nsACString &aScheme,
> >+  if (handlerType != 1) {
> 
> So either not found at all, or not found in prefs, right?  Why are we treating
> those two cases the same here?

Exactly because of:
 
> >+    // We also don't need to SetDefaultDescription for nsMIMEInfoUnix
> 
> Why not?

Because now, having nsMIMEInfoUnix means that its default is to use the GNOME handler; no need to SetDefaultDescription, it won't be used.
Do you want me to add this in the comment ?
Might be good to document that mType is the scheme if we're a protocol info (or maybe even name it mSchemeOrType, or something).

> Do you want me to add this in the comment ?

Yes, please.
(In reply to comment #7)
> Might be good to document that mType is the scheme if we're a protocol info (or
> maybe even name it mSchemeOrType, or something).

But this is totally irrelevant to the present bug. This is how the API is already.
Yes, I know the code is confusing already.  I can't help that.  But if it's easy to make it not confusing, please do so.  Someone needs to at some point, and you're already in this code.  If you want to do it in a separate patch, that's fine.
Attached patch patch v3 (obsolete) — Splinter Review
New patch with requested comment changes
Attachment #328766 - Attachment is obsolete: true
Attachment #329956 - Flags: superreview?
Attachment #329956 - Flags: review?(cbiesinger)
Attachment #328766 - Flags: superreview?(dmose)
Attachment #328766 - Flags: review?(cbiesinger)
Attachment #328766 - Flags: review?(bzbarsky)
Attachment #329956 - Flags: superreview?(dmose)
Attachment #329956 - Flags: superreview?
Attachment #329956 - Flags: review?(bzbarsky)
Flags: wanted1.9.0.x?
(In reply to comment #9)
> Yes, I know the code is confusing already.  I can't help that.  But if it's
> easy to make it not confusing, please do so.  Someone needs to at some point,
> and you're already in this code.  If you want to do it in a separate patch,
> that's fine.

I'll file a new bug for that, and will change the variable name as you suggested. I should probably wait for the present patch to land, though.

> I'll file a new bug for that, and will change the variable name as you
> suggested. I should probably wait for the present patch to land, though.

Filed bug 445673
> I'll file a new bug for that,

Sounds good.
Blocks: 445673
Comment on attachment 329956 [details] [diff] [review]
patch v3

r=bzbarsky.  No need to wait on biesi if dmose srs, I think.
Attachment #329956 - Flags: review?(bzbarsky) → review+
Comment on attachment 329956 [details] [diff] [review]
patch v3

can you diff with more context next time?

+    // Now that nsMIMEInfoUnix is only about GNOME handlers, we don't need to
+    // SetDefaultDescription in this case, as it won't be used anyway.


I'd just remove the "Now" in that sentence, that this used to be different isn't really interesting.

+  // Returns 0 for no handler, 1 for mailcap handler and 2 for GNOME handler */
+  int GetProtocolHandlerType(const char * aProtocolScheme);

can you make this return an enum instead? having to remember what the numbers mean isn't ideal

also, remove the */ at the end of that comment
Attachment #329956 - Flags: review?(cbiesinger) → review+
This patch (v3) introduces a regression, discovered whilst researching Ubuntu LP bug #239952 "the associated helper application does not exist". For the wider explanation see that bug.

https://bugs.launchpad.net/ubuntu/+bug/239952

The specifics are that this (v3) patch was suggested as a possible fix for LP bug #239952. When I tested it I discovered a regression in that when the Download Helper Chooser dialog is first displayed the listed entry would be different to the expected offering, but if "Other..." was chosen and then the file-chooser dialog cancelled, the expected helper would be listed.

This happens in the scenario that the web-server has provided an invalid "Content-Type" and xulrunner is trying to figure it out from the file extension.

.txt file
Content-Type: plain/text

Helper chooser dialog suggests to open with "gedit.desktop". However, if "Other..." is selected from the list, and the file-browser dialog cancelled, Open With displays "Text Editor (default)".

If the "OK" button is pressed when "gedit-desktop" is displayed no error is reported but no helper is launched. If the "OK" button is pressed when "Text Editor (default)" is displayed, the "Download Error" dialog appears and reports that the helper application can't be launched - in other words, the same problem as reported by this bug originally.

.gz file
Content-Type: unknown/unknown

Helper choose dialog suggests to open "firefox 3.0.1". However, if "Other..." is selected from the list, and the file-browser dialog cancelled, Open with displays "Archive Manager (default)".

If the "OK" button is pressed when either "firefox 3.0.1" or "Archive Manager (default)" is displayed the "Download Error" dialog appears and reports that the helper application can't be launched.

.pdf
Content-Type: application/pdf

Helper chooser dialog suggests to open "Document Viewer (default)". Pressing the "OK" button launches the helper application (evince) as expected.

I'm working on a modification of this 'v3' patch to solve that and then also to fix the LP #239952 issue, which are closely related.

My question is, should I do them as two separate patches or deal with both symptoms in one patch (since they have the same root cause) ?
If the root cause is the same, one patch sounds fine.
Does it happen with fresh new profile ? (or with a profile with purged mimeTypes.rdf ?)
The regression report in comment #16 applies to a new user account, or a new firefox profile.

With the same .txt test-file as in comment #16 (Content-Type: plain/text) (yes! the server admins have mis-configured that - it should be text/plain)

The only difference is, the initial Open With suggestion is empty (obviously, since no handlers have yet been declared for .txt). Selecting "Other..." and then cancelling the resulting file-chooser dialog, the Open With suggestion displays "Text Editor (default)".
Interestingly, there are no HelperAppService logs when displaying and closing the file-chooser dialog...
Okay, I spotted the problem. It lies in nsOSHelperAppService::GetMIMEInfoFromOS, which does a SetMIMEType on the MimeInfo it gets.

To get this MimeInfo, first, we try to get it from the mime type, and fail because the mime type is wrong. Then we try by extension, from which we get a mime type with /etc/mime.types and friends. We then get again from mime type, but from the guessed one.

The thing is that this mime type is used by the nsMIMEInfoUnix instance to know what to call from gnome. Overriding this as SetMIMEType does makes it forget about it and do the wrong thing afterwards.

I see two possible solutions for this:
- store the mime type used for gnome calls in another variable than mType, so that we have a "display" mime type and a functional mime type.
- add a constructor to nsMIMEInfoUnix, that would take a mime type and a gnome vfs handler (which we actually already get in GetFromType) and keep all that in the mimeinfo.

I'll try the second approach, which seems better to me.

That it worked before is almost a miracle, actually.
> Then we try by extension, from which we get a
> mime type with /etc/mime.types and friends.

Actually, this is gotten from gnome vfs, too.
Attached patch supplementary (test) patch (obsolete) — Splinter Review
This patch implements the first idea, because I don't have much time right now and it was the quickest to come up with.
Can you test if this patch (on top of patch v3) solves your regression ? That would at least validate my theory, before I can come up with a proper patch.
FWIW, thanks to the regression, I spotted another problem with mime handling when $HOME/.mailcap contains some entries. For example, in the plain/text .txt case, if $HOME/.mailcap contains entries for text/plain, which is what .txt will resolve to, they are ignored while they shouldn't, because nsGNOMERegistry is doing both by extension and by type resolutions while it shouldn't. I'll address this as well, since it fits the original problem.
Is there an infrastructure to fake some components in the test suite ? I just thought testing the GNOME stuff would probably only properly work if the gconf and gnome vfs services were diverted and controlled by the test script...
If it's xpcom, you can try to register a factory object for it in order to control it, but if it's not xpcom, you are out of luck
Attached patch patch v4 (obsolete) — Splinter Review
This patch includes patch v3 + suggested changes by Christian + reimplementation of the supplementary patch with initialization of the nsMIMEInfoUnix by giving it a nsIGnomeVFSMimeApp for file handlers (as opposed to protocol handlers) + finally, have nsOSHelperAppService handle GetFromExtension fallback to GetFromType ; this has the extra advantage of simplifying the code.

The patch is experimental, please test it as much as you can. I'm trying to get a unit test in shape that will finally test most if not all known corner cases.
Asking for review anyway, because I'm not confident with coding style. If this patch appears to be good, I'll just attach the unit test as another attachment instead of sending the whole squashed.

I can already confirm that the current unt test passes with this patch, and adding the following small test passes too (this one doesn't pass without the patch, cf. comment #24):
--- a/uriloader/exthandler/tests/unit/test_handlerService.js
+++ b/uriloader/exthandler/tests/unit/test_handlerService.js
@@ -391,5 +391,9 @@ function run_test() {
     handlerInfo = mimeSvc.getFromTypeAndExtension("text/plain", null);
     do_check_eq(handlerInfo.preferredAction, Ci.nsIHandlerInfo.useSystemDefault);
     do_check_eq(handlerInfo.defaultDescription, "sed");
+
+    handlerInfo = mimeSvc.getFromTypeAndExtension("plain/text", "txt");
+    do_check_eq(handlerInfo.preferredAction, Ci.nsIHandlerInfo.useSystemDefault);
+    do_check_eq(handlerInfo.defaultDescription, "sed");
   }
 }
Attachment #329956 - Attachment is obsolete: true
Attachment #338551 - Attachment is obsolete: true
Attachment #338702 - Flags: review?(bzbarsky)
Attachment #329956 - Flags: superreview?(dmose)
Attachment #338702 - Flags: review?(cbiesinger)
Okay, I already spotted something wrong:
+  if (mVFSApp)
+    mVFSApp->Launch(nativePath);

There should be a return before mVFSApp->Launch.
I started to write some testcase, that both passes before and after the patch, but I have strange results...
With a custom mime.types containing:
application/postscript	ps ai eps espi eps2
text/plain txt asc

getFromTypeAndExtension('application/postscript', 'eps').getFileExtensions() gives [ 'ps', 'ai', 'eps', 'espi', 'eps2' ] (order kept) while 
getFromTypeAndExtension('text/plain', 'asc').getFileExtensions() gives [ 'asc', 'txt' ] (reordered). When using the primary extension, order is kept in both cases. Requesting the primary extension in both cases gives the first item, and requesting the primary extension hinting an extension (getPrimaryExtension('text/plain', 'asc'), for instance), the given extension is returned in both cases, which means there is somehow something inconsistent, too, because there are cases where getFromTypeAndExtension(type, ext).getFileExtensions()[0] != getPrimaryExtension(type, ext), which is quite unexpected...

I haven't taken a look at why it happens, though. I'm trying to focus on avoiding regressions, already.
getFromTypeAndExtension() should ensure that the given extension is first in the list if it's somewhere in the list.  There is UI code (e.g. file naming in the save dialog) that depends on this behavior to get extensions right, and code in nsExternalHelperAppService::GetFromTypeAndExtension that should be ensuring it.  So sounds like the behavior you see with PostScript is somehow buggy.
What should we do, then?  Knowingly add a unit test that does the wrong thing, but garantees the same result before and after the patch, then fix the issue in another patch, or the other way around ?

Another question. Can there be cases where the content type given to mimeSvc.getFromTypeAndExtension is null, but extension is not ? The patch introduces a difference there, but if it is not supposed to happen, it is not a real problem...

Finally, I also spotted that tests line 155 and 174 in uriloader/exthandler/tests/unit/test_handlerService.js only work when the mozgnome component works. Maybe they should be special cased.
I'm OK with adding a unit test for the current behavior and a comment or better yet with a todo() for the correct behavior and filing a followup bug to fix.

There can certainly be getFromTypeAndExtension calls with a null type and non-null extension, I would think.  Nothing preventing them...
Mike, can you hold off for a day or so?

I've almost finished a patch for the "the associated helper application does not exist" case. I'm testing it against the ~/.mailcap entry case you describe here as well. It solves both issues and is minimally invasive - four lines of code.

There is one minor irritation (not caused by the patch but the convoluted existing code): when the same mime-type is found in the firefox profile's mimeTypes.rdf it over-rides the mime-type description already found, it causes the user-friendly mime-type description in the Helper dialog to show the incorrect human-readable string.

I'm just trying to figure out a simple way to fix that now. It gets overwritten due to a call to the Javascript nsHandlerService.js::fillHandlerInfo in nsExternalHelperAppService.cpp::GetFromTypeAndExtension(). I'm hoping to solve it with a simple additional clause in the statement:

if (NS_SUCCEEDED(rv) && !overrideType.IsEmpty()) {
I've created a new report for the specific issue I'm addressing: bug #455626

There is a patch attached to that report which also appears to solve this issue.

The root cause of both is the duplication of the handler-determination process. Once to fill the Helper Application chooser and allow the user to make a decision, and again later after the download completes.

The two sets of code do different things and therefore the action at completion of download can be different to that determined and/or chosen by the launch/dialog code.
There are various problems with mimeTypes.rdf, see bug #444477
So do you still want reviews on the v4 patch?  I'd rather not iterate reviews along with continuing development, so if it's at all in flux, please reask for review once it's done?  If you do want review on it, I'd love an interdiff against the code I already reviewed in v3.
Attachment #338702 - Flags: review?(cbiesinger)
Attachment #338702 - Flags: review?(bzbarsky)
Doesn't really meet the "wanted" criteria (security, stability, regression from maintenance release) for 1.9.0.x. However, we'll look at a reviewed and baked patch.
Flags: wanted1.9.0.x? → wanted1.9.0.x-
FWIW, it's a regression compared to firefox 2.x
(In reply to comment #38)
> FWIW, it's a regression compared to firefox 2.x

Yeah, and a bad regression indeed. Firefox 3 has horrible download/file handling on Linux. :(

Mike, how you doing on getting a final patch that can be landed?
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x-
Flags: blocking1.9.1?
Keywords: regression
I wouldn't say it's that bad a regression. Not a lot of people have entries in their $HOME/.mailcap.

Anyways, I hope to have enough time this week to come up with proper testing and non regressive patch.
This isn't exactly the same bug, but when the file name contains a blank, I'm
getting the correct name for the temp file:

/tmp/FUNdaMENTALs Topic 7-1.PDF

but an escaped name passed to the helper:

Error: Couldn't open file '/tmp/FUNdaMENTALs%20Topic%207-1.PDF'

Have I misconfigured something? This is 3.0.3 on ubuntu. I'm using the workaround and the helper is xpdf.
Jim, that sounds like a separate bug that should be filed.  Please cc me on it; that's almost certainly a regression that needs fixing.
I think we'd like to take this, but we're not going to block on it and will definitely require tests.
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: in-testsuite?
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Shouldn't this be a blocker because of the regression from stable release and the security implications?
I'm going to rework the patch this week, with testsuite.
Now *this* is weird.  My .mailcap says to use xpdf to view pdfs.  Using the
20081028 trunk build, the 32-bit Linux *does* invoke xpdf, the 64-bit one ends
up saying it will use xpdf, but actually launches evince (what Gnome has
configured).

% which xpdf
/usr/bin/xpdf
% file /usr/bin/xpdf
/usr/bin/xpdf: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically
linked (uses shared libs), for GNU/Linux 2.6.9, stripped
% which evince
/usr/bin/evince
% file /usr/bin/evince
/usr/bin/evince: ELF 64-bit LSB executable, x86-64, version 1 (SYSV),
dynamically linked (uses shared libs), for GNU/Linux 2.6.9, stripped

Only thing I can think of is that I'm missing some "optional" Gnome libraries 
because I have a very stripped-down 32-bit runtime on here (basically, only
those needed for Firefox to startup without crashing).  Firefox, the Flash and
Java plugins, and one PKCS11 library are the only 32-bit apps left on here.
Attached patch possible patch v5 (obsolete) — Splinter Review
This is a possible replacement patch that has the advantage of also working with the new hildon code in trunk (at least theorically), though the context diff might not apply cleanly.
I haven't had time to clean up my test suite, though :( I also haven't checked the status of bug #455626 with this patch.
Not "wanted" in the sense that drivers want this on the list of things we're tracking/pushing, but if you get a branch-safe patch we'll look at approval requests.
Flags: wanted1.9.0.x?
Given that this bug is unlikely to be fixed any time soon, and apparently we're expected to configure Firefox via gconf now, what's really needed is a way to set the gnome handler for pdf (or any other mime type) without having a gnome desktop running.

I spent some time trying to figure this out, and as far as I can tell, there is no way to do this even with the desktop running. It doesn't show up in any of the preference menu items.

gconftool-2 is used to set most gnome preferences. You can dump them all like this: gconftool-2 -R /

But mime types aren't there. Gnome has several different prefs databases, and the mime handlers are not stored in the default one. There is a file that overrides the standard mime handlers, and you can edit it with this:

gconftool-2 --direct --config-source xml:readwrite:.local/share/mime/packages/Overrides.xml -t str -s something??

But at this point I got stuck. Any help would be appreciated.
This bug still occurs with:

  Mozilla/5.0 (X11; Linux x86_64; rv:10.0.4) Gecko/20100101 Firefox/10.0.4 Iceweasel/10.0.4

Though my $HOME/.mailcap says to use xpdf for PDF files, gv is launched.
Attached file pdf helper app
This is the helper app I've been using. I name it "evince" and put it somewhere in my PATH (~/bin for example).
Duplicate of this bug: 816000
So let me see if I have this straight:

- When you click on a PDF link, Firefox offers you a choice of applications with which to open it.

- Firefox then ignores whatever choice you make, and instead asks gconf which application to use.

- Nobody seems to know how to control what gconf tells Firefox.

- The best solution that has been offered is "replace the application Firefox chooses with a dummy that opens the one you actually want."

Surely I'm not the only one who sees a teensy problem with this situation?

The bug was first reported 4.5 YEARS ago, and there hasn't been a substantive post in nearly a year.  Perhaps it's time to fix it.
Geoff: Yes, I believe that sums up the current status, but I would add:

1. We've had a fix available for over 4 years, but it's been ignored.
2. This used to work but was broken in 3.x.

I agree it's time to fix it, but I doubt that will happen any time soon.
> - When you click on a PDF link, Firefox offers you a choice of applications with which to
> open it.

No, Firefox offers a choice of "save" and "open", with an application listed next to "open".  You can also select a different application via a third option.

> - Firefox then ignores whatever choice you make, and instead asks gconf which
> application to use.

If you select a different application, that application is used.  If you select "open" without changing anything, the application used may not match the one that was listed next to the option.  Or put another way, the UI was displaying the wrong thing to start with.

> - Nobody seems to know how to control what gconf tells Firefox.

That's correct.

> - The best solution that has been offered is "replace the application Firefox chooses
> with a dummy that opens the one you actually want."

I don't think so.

> 1. We've had a fix available for over 4 years, but it's been ignored.

Excuse me?  Both Mike and I have spent a good bit of time on that fix.  It's not clear to me what the state of it is at the moment; last I asked Mike whether it was ready for review he said it wasn't.

If someone is interested in fixing this, I recommend contacting Mike and asking him what remains to be done, then doing it, then asking me for review.
I know that Mike and Boris both put a lot of work into this patch. What I meant was that after all that work went in to it, it got dropped. That was over four years ago. The patch does me no good in its current state.

The best solution I know of, and the one I'm still using, is to "replace the application Firefox chooses with a dummy that opens the one you actually want." If a better solution has been offered, I don't see it in the comment log.

I appreciate the effort you have put in to get this fixed. I just think it's a shame that all that effort was wasted.
I just tested it: Boris is technically correct.  On the machine I'm using at the moment, the default option is "xpdf (default)."  If I choose that, acroread gets launched.  But if I choose "other" and select /usr/bin/xpdf, I get xpdf as desired.

Of course, there's still a bug and a flaw.  The bug is what Boris noted: what's listed as the default application is incorrect.  It would be better to just be honest and say "default" without attempting to identify it.  Or even (gasp!) to query gconf using the same code that does the launch, and display the name of the application that actually would be launched.  It can't be *that* hard.

The flaw is that if you don't like any of the applications that are offered on the list and you choose "other", Firefox doesn't remember the one you chose.  The nice design would be add that application to the list of choices so that you don't have to go out of your way the next time you want to use your favorite application.  Even nicer would be to make the most recently chosen application into the default for next time.

If Mike thinks Jim's patch isn't "ready for review", then perhaps he could update this bug with a description of what needs to happen to make it ready.
It's Mike's patch.  But yes, having concrete info here about what's left to do would be good.  Mike?
Flags: needinfo?(mh+mozilla)
If Firefox doesn't remember the choice you make, then that's not a solution at all. Geoff's summary is correct, and we're back to making a dummy "evince" that runs the real application. And yes, it's Mike's patch, not mine.

If we're not going to fix this, we should do what Geoff suggests, change the name of the default to "evince" or "default." It doesn't seem like that should be hard. Or better yet, remove the ability to choose an application.
Four years after the fact, I'm not sure what more was needed. However, I've always had this patch applied on the Debian package, so I know it doesn't break things in awful ways, and it also can't make things worse than they are, so let's just get this landed and see what happens from there.
Attachment #718954 - Flags: review?(bzbarsky)
Flags: needinfo?(mh+mozilla)
Attachment #338702 - Attachment is obsolete: true
Attachment #349554 - Attachment is obsolete: true
I've filed a related bug #845817.
Comment on attachment 718954 [details] [diff] [review]
Prioritize mailcap application over others in nsMIMEInfoUnix

Ah, very nice.  Let's do that.
Attachment #718954 - Flags: review?(bzbarsky) → review+
(In reply to Jim Rees from comment #59)

> should be hard. Or better yet, remove the ability to choose an application.

No, that's *not* acceptable.

Consider the case of a security flaw being found in evince.  The user may wish to use
xpdf instead until a patch becomes available for evince.

Or they're testing how various apps behave.

Or any one of a lot of other reasons that a user may legitimately want to choose a different application on a one-off basis.
It's easy to say it's "not acceptable" but the fact is we're stuck with the inability to configure a pdf helper app. Given that we can't configure this, we should remove the UI for it. It's bad enough that we can't select a helper, but it's even worse that we provide the illusion that we can configure a helper, then go ahead and run a different app. The UI should be removed until and unless we provide some way to actually configure the app. That's why I filed bug #845817.

And yes, I agree this is a security risk. That's why I'd like to see the UI removed right away.
(In reply to Valdis Kletnieks from comment #64)
> (In reply to Jim Rees from comment #59)
> > should be hard. Or better yet, remove the ability to choose an application.
> 
> No, that's *not* acceptable.

I agree that's not acceptable. On my Debian machines, the default for PDF files is gv (I don't know why, I've never made such a choice), while gv can't read some PDF files, and when it can, the rendering is usually horrible.

The UI may be removed if there's another way to choose the application, e.g. via about:config.

But what would the behavior be without the UI? Starting some default application without telling the user first would not be acceptable at all. It would be better to ask the user to save the file, nothing else.
As I understand it, there isn't any way for FF to know what app is going to run. So I think removing the UI is the best we can do. Or we could leave the UI in, but have the only choice be "default" (with no particular app specified), grayed out so it can't be changed.

This is separate from the dialog that pops up when you actually click on a pdf link. If possible, the dialog should pop up every time, with the choice of either running the default app (whatever that is) or saving the file.
(In reply to Jim Rees from comment #68)
> As I understand it, there isn't any way for FF to know what app is going to
> run. So I think removing the UI is the best we can do. Or we could leave the
> UI in, but have the only choice be "default" (with no particular app
> specified), grayed out so it can't be changed.

Actually, that's backwards.  Currently in the 22-whateveritis nightlies, if I click on a PDF link, I get 3 choices:  "xpdf", "xpdf (default)", and "save as..".  If I select 'xpdf', FF actually *does* know what app is going to run, and runs the app I told it to run.  It's in the "xpdf (default)" case that FF disregards the xpdf and pulls some other "default" app out of an orifice and runs it - which is contrary to the Principle of Least Surprise.  That would suggest that "xpdf (default)" has the semantics "run xpdf if it's available, else call the default app".  As such, it's the "(default)" one that should be nuked and/or greyed out....
https://hg.mozilla.org/mozilla-central/rev/f118eb2326f7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I strongly urge people to try tomorrow's nightlies, which have Mike's patch.  Please report bugs on any weird behavior _those_ have and cc me on those bugs, ok?
Is it normal to close a bug report before the fix has been tested?

I'm getting unexpected behavior with the nightly, not sure it's a bug. If I get ambitious I'll open a new bug on it.
> Is it normal to close a bug report before the fix has been tested?

Well, Mike has tested the fix locally.  It's normal to resolve the bug when the fix is checked in, yes.

You can't be seeing unexpected behavior with a nightly that doesn't exist yet!  Whatever you're testing now doesn't have this fix in it.
I'm testing the builds Mike asked me to test in Comment 66. I was probably wrong to call that a "nightly".
OK.  Please do file whatever bugs you're seeing, then.
Depends on: 852540
Depends on: 947868
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.