128668 - Use GNOME application associations

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Brian Ryner (not reading)]

We should use GNOME's MIME type associations as defaults for helper applications.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ok... I looked into this.  There is no way to tell where the GNOME stuff is
installed without using the GNOME calls into it (which makes Mozilla depend on
gnome.h, which seems unacceptable).  We could pref it and default to
/usr/share/mime-info which is where the stuff lives...

There is the separate issue of parsing these files, of course.  :)

Would it make sense to make this a compile-time switch?  That way we could just
use the canned GNOME routines to access this info without having to completely
duplicate them....

Comment 2

[Boris Zbarsky [:bzbarsky]]

ccing blizzard for his insight... It'd be nice if we didn't have to compile
against gnome.h to get this info....

Comment 3

[Brian Ryner (not reading)]

I don't have a problem with a compile-time header dependency, but adding a
_runtime_ dependency on libgnome is something we'd want to think carefully about.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah.  Ideally, there would be a small standalone app (daemon?) that could be
called to handle this stuff...

Comment 5

[Christopher Blizzard (:blizzard)]

Yeah, I don't want to add the run time associations right now, especially
considering gnome 1.x is on its way out the door.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Can't we simply dlopen libgnome and call the necessary functions using dlsym?

Comment 7

[Brian Ryner (not reading)]

Ok, here comes a preliminary implementation of my GNOME 2 integration component.

Comment 8

[Brian Ryner (not reading)]

Attachment: tar.gz file of extensions/gnome-integration

untar this in your mozilla directory

Comment 9

[Brian Ryner (not reading)]

Attachment: patch to existing files to hook up gnome-integration

note that you'll need to run autoconf to regenerate configure after applying
this.

Comment 10

[Brian Ryner (not reading)]

Boris or Blizzard, can I get a review? or at least some comments?

Comment 11

[Christopher Blizzard (:blizzard)]

I'll try to get to this later today.

Comment 12

[Philip Langdale]

Brian,

Interesting patch; couple of comments:

1) everything built on glib2/gtk2 (including gconf-2 and gnome-vfs-2) works
in UTF8, so your NS_ConvertASCIItoUTF8 call is bad.

2) Your gnome-vfs code assumes sanity in the gnome-vfs database. We used to do 
this, but we learnt our lesson. eg: Some mimetypes will have a list of helpers 
but none of them will be marked as the default...

Our approach in galeon has been rather different:

We overide nsIExternalHelperAppService directly to handle external protocol 
handlers. We haven't ported this to gnome2, but our code basically looks like 
yours.

Helper apps are handled at an earlier stage in the process where we bypass
mozilla's helper app db completely. I won't repeat all the details but scribbled
down in bug 147142; and it's worth noting that we're currently broken - hence
that bug. The key stuff is that we don't rely blinding on the contents of the
gnome-vfs database. We have our own private helper db (simple xml file) which
tracks which helper the user wants to use and it's running characteristics.
When the user clicks on a link to a file that uses a helper, we pop up a dialog
(our nsIHelperAppLauncherDialog) and ask which helper the user wants to use.
We provide the list of helpers from the gnome-vfs db and also give them the
option of typing one in. We also give them a chance to use that helper again
in future without confirmation. Our private db stores the user's decision.

I would like your opinion on the embedder overriding mechanism I proposed in
bug 147142. It may not be appropriate for your work, but I think something along
those lines is necessary for embedder's to do this properly.

Comment 13

[Brian Ryner (not reading)]

I assume you mean the NS_ConvertASCIItoUCS2() call.  I'll change that to
NS_ConvertUTF8toUCS2().

This approach does allow for the helper app association to be overridden since
there is a call to the base class implementation before we call into
nsIDesktopIntegration (the base class impl checks the RDF datasource where these
settings are stored).

Since galeon stores its helper associations in a different way, that obviously
won't work, but we'll take that discussion to bug 147142.

Comment 14

[Boris Zbarsky [:bzbarsky]]

1.1alpha is frozen.  Unsetting milestone and will retriage in a few days when I
can make a realistic assessment of the situation.

Comment 15

[Christopher Blizzard (:blizzard)]

r/sr=blizzard

Comment 16

[Boris Zbarsky [:bzbarsky]]

What Philip said about the charset stuff.

-- nsGNOMEIntegration.cpp::LaunchAppWithTempFile

+      // if we were given an application to use then use it....otherwise
+      // make the registry call to launch the app

What is this registry you speak of?

No need to redeclare |nsresult rv;| in the if-block.

+      local->IsExecutable(&executable);
+      if (!executable)
+        rv = local->Launch();

nsLocalFileUnix::Launch() is not implemented yet... so this won't hurt anything,
but it won't do anything useful either.

Since this whole method just does what
nsOSHelperAppService::LaunchAppWithTempFile does, is there a good reason to not
just return an error from it and let the nsOSHelperAppService deal?

-- nsGNOMEIntegration::ExternalProtocolHandlerExists

+        !strcmp(protocol_name + 1, aProtocolScheme)) {

I think you want strcasecmp

-- nsGNOMEIntegration::GetFromExtension

If I read this right, this will assume any unknown extension is text/plain and
look for a handler for that, no?  That seems wrong to me; if there is no mapping
for that extension I would rather we just said so (returned an error).

-- nsGNOMEIntegration::GetFromMIMEType

This will reverse the order of the extension list... I don't think that matters,
since we should not really be depending on that list being in a particular order
anywhere, but one never knows... For one thing, this will change what the
"default" extension is for the type.

Could this set a useful ApplicationDescription on the MIME info?  If that's not
set, we end up using the leaf name of the nsIFile, iirc.

Please make sure you don't set "useHelperApp" when the nsILocalFile is null (out
of memory, or whatever).. that will do bizarre things down the line.

-- nsGNOMEIntegration::ExpandHelperAppTemplate

OOM check on the strdup?

The changes to nsOSHelperAppService.cpp look good; I'm not really qualified to
review the autoconf changes or the Makefile.in (though the latter looks good as
far as I can tell).

My apologies this took so long... finding docs on the GNOME stuff proved
difficult.  :(

Comment 17

[Boris Zbarsky [:bzbarsky]]

To Brian, since he's got the patch

Comment 18

[Nicolás Lichtmaier]

I don't know if it's relevant, but... In GCONF2/GTK2 they had the arrogance of
deciding that every filename in the filesystem is UTF8. This is not true for
everybody who has been using Linux for years. To overcome this, one must set the
environment variable G_BROKEN_FILENAMES. I don't know, but perhaps Gnome sends a
different thing if this variable is set and Mozilla should handle that.

Check http://www.gtk.org/gtk-2.0.0-notes.html .

Comment 19

[Brian Ryner (not reading)]

Boris: I don't follow what you mean about GetFromMIMEType reversing the order of
the extensions.  It appends them to the mimeInfo object in the same order
they're returned by GNOME.

There is a 'name' field in the GnomeVFSMimeApplication struct that we get back
that could provide an applicationDescription.  I'll have to check it out to see
how useful it is.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> It appends them to the mimeInfo object in the same order they're returned by
> GNOME.

So it does... I'm not sure what I meant by that comment; I think I was wrong.

Comment 21

[Ben Bucksch (:BenB)]

This is surely nice, thanks for the code.

Did anybody conside the security implications?
The GNOME associations are intended for *local* use, not use with untrusted
files. On Windows, I want to open .doc on my harddrive files with MS Word, but
not those from the Internet, I'd rather open them with another app that doesn't
execute the macros and is generally safer. Similar examples surely also exist on
Linux.
Yes, the user will be prompted (I hope?) before any external apps will be
launched, but we should not assume that the user is aware of the security
implications or that the GNOME helper apps are all secure.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> The GNOME associations are intended for *local* use, not use with untrusted
> files.

Why?  That's a very short-sighted view that restricts their use to file managers
and locks out mail clients, browsers, etc.

Furthermore, "local" and "untrusted" are not mutually exclusive, especially in
any environment that uses NFS/AFS/etc.

> Yes, the user will be prompted

For MIME types, the first time the type is encountered (as is done right now for
types with handlers set via mailcap).  For protocols, not unless some additional
code is written (that should probably be written XP, not just on Linux).

If we can't depend on helper apps to be secure, then we can't use helper apps at
all is what you're saying.  I fail to see how the GNOME helper apps are any
different in this respect from helper apps set via mailcap or in the
mimeTypes.rdf file that the system administrator decides to distribute with
Mozilla.  Except for the fact that the user is more likely to be comfortable
with editing the GNOME app associations than either of the other sources of this
information, of course.

Comment 23

[Brian Ryner (not reading)]

Attachment: updated work-in-progress

I reconsidered how to do this and I'd prefer to it this way, which doesn't rely
on having the GNOME packages available on the build system, and doesn't add an
extra library.

I gave some thought to how fix the default handler XXX comment in the patch
(that's the case where we invoke the default handler, which turns out to be our
own application).  The best idea I've come up with so far is to set an
environment variable, which will be propagated to the child process, and if
it's present in the environment when Mozilla starts, we know we've hit the
problem.  We could throw a native dialog without starting up the actual app
saying that "This file type is not supported" or somesuch, then simply exit
when the user hits OK.	Other ideas welcome.

Along with this, we should really add support for registering
Mozilla/Firebird/Thunderbird in the GConf registry for the protocols it is
capable of handling.

Comment 24

[Brian Ryner (not reading)]

Comment on attachment 132120 [details] [diff] [review]
updated work-in-progress

Boris, Chris, I'm not declaring this ready to check in or anything, but
wondered if you guys had any comments.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132120 [details] [diff] [review]
updated work-in-progress

>Index: nsGNOMERegistry.cpp
>+ * Contributor(s):
>+ *

You should list yourself under contributors as "original author".

>+  #define GET_LIB_FUNCTION(lib, func) \
>+    _##func = (_##func##_fn) PR_FindFunctionSymbol(lib##Lib, #func); \
>+    do { \

PR_BEGIN_MACRO instead of "do { \" (expands to the same thing, but makes it a
little clearer what's going on).

>+      if (!_##func) { \
>+        PR_Free(libName); \
>+        CleanUp(); \
>+        return; \
>+      } \
>+    } while (0)

PR_END_MACRO instead of "} while (0)"

>+nsGNOMERegistry::HandlerExists(const char *aProtocolScheme)
>+  nsCAutoString gconfPath(NS_LITERAL_CSTRING("/desktop/gnome/url-handlers/") +
>+			  nsDependentCString(aProtocolScheme) +
>+			  NS_LITERAL_CSTRING("/command"));

Weird indent.

>+  // XXX should we look for the default handler?

What does that comment mean in this context?

>+nsGNOMERegistry::LoadURL(nsIURI *aURL)

>+  if (_gnome_url_show(spec.get(), NULL))
>+    return NS_OK;

Does gnome_url_show deal with URLs that contain raw UTF-8 chars?  If not, you
may need GetAsciiSpec here, not GetSpec....

>+nsGNOMERegistry::GetFromExtension(const char *aFileExt)

I guess it's OK to do it this way, but is it possible for the GNOME database to
be set up in a way where an extension maps to some type but is not present in
the list of extensions for that type?  In those cases, GetFromExtension would
return a MIMEInfo object which in fact did not list the extension that was used
to do the lookup.... (I don't know anything about the GNOME MIME database and
what states it could be in, hence the question).

>+nsGNOMERegistry::GetFromType(const char *aMIMEType)

>+    if (appFile) {
>+      mimeInfo->SetPreferredApplicationHandler(appFile);

You want to be setting DefaultApplicationHandler, not
PreferredApplicationHandler.


>+      // XXX the preferred action could be "component" - what should we do?
>+      mimeInfo->SetPreferredAction(nsIMIMEInfo::useHelperApp);

nsIMIMEInfo::useSystemDefault, not useHelperApp.

>+  } else {
>+    mimeInfo->SetPreferredAction(nsIMIMEInfo::saveToDisk);
>+  }

Doesn't this mean the mailcap code will _never_ get accessed?  If there is no
handler, wouldn't it make more sense to return null?

>Index: nsOSHelperAppService.cpp

>@@ -1262,6 +1264,10 @@ NS_IMETHODIMP nsOSHelperAppService::Exte
>     *aHandlerExists = (NS_SUCCEEDED(rv1) && exists && NS_SUCCEEDED(rv2) && isExecutable);
>     LOG(("   handler exists: %s\n", *aHandlerExists ? "yes" : "no"));
>   }
>+
>+  // Check the GConf registry for a protocol handler
>+  *aHandlerExists = nsGNOMERegistry::HandlerExists(aProtocolScheme);
>+
>   return NS_OK;
> }

This is wrong.	It'll reset *aHandlerExists and override our pref setting, if
any.  Which is no good.  This code should only ask the GNOME registry if
*aHandlerExists is false.

>@@ -1370,6 +1377,10 @@ nsOSHelperAppService::GetFromExtension(c

>+  nsIMIMEInfo *gnomeInfo = nsGNOMERegistry::GetFromExtension(aFileExt).get();
>+  if (gnomeInfo)
>+    return gnomeInfo;

It would be good to log the fact that we got a MIMEInfo from the GNOME registry
before returning.

>@@ -1443,6 +1454,10 @@ nsOSHelperAppService::GetFromType(const 

>+  nsIMIMEInfo *gnomeInfo = nsGNOMERegistry::GetFromType(aMIMEType).get();
>+  if (gnomeInfo)
>+    return gnomeInfo;

Same.

>   LOG(("Here we do a mimetype lookup for '%s'\n", aMIMEType));

And the position of this log statement should be consistently before or after
the GNOME registry call (I prefer before).

Comment 26

[Brian Ryner (not reading)]

>>+  // XXX should we look for the default handler?

> What does that comment mean in this context?

It means that in theory, we could always claim that a handler exists, because we
can throw it to the default handler.  Common sense says that we probably don't
want to do that, so I'll remove the comment.

> Does gnome_url_show deal with URLs that contain raw UTF-8 chars?  If not, you
> may need GetAsciiSpec here, not GetSpec....

That's not clear to me.  It does assume the protocol is ASCII, I think.  Other
than that, it just strdup()s the URL into the child process's argv.  What
encoding do we assume is being used when a URL is given to an application on the
command line?

> I guess it's OK to do it this way, but is it possible for the GNOME database
> to be set up in a way where an extension maps to some type but is not present
> in the list of extensions for that type?  In those cases, GetFromExtension 

I did a bit of digging, and I don't believe it's possible for this situation to
happen.  Both types of lookups get their data from the same file, which is of
the form:

application/foo:
    ext: bar baz

which means that mime type application/foo is associated with extensions .bar
and .baz.  So, an extension can certainly be listed under more than one MIME
type.  However, you're guaranteed that once you've gotten a MIME type from an
extension, that extension is indeed in the list for that MIME type.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> What encoding do we assume is being used when a URL is given to an application
> on the command line?

Good question.  In Mozilla, we actually assume nothing useful, in part because
the command line service is totally not intl-aware...  So we try it as UTF-8,
and if that fails, try current locale charset.  ccing darin in case he has any
thoughts on this.

Thanks for looking up the storage format for this info.  Given that, the logic
in that patch is perfectly fine.

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 132120 [details] [diff] [review]
updated work-in-progress

+#include <glib.h>
+#include <glib-object.h>

Hm. You add a glib dependency here...
Did glib-object.h exist in Gtk+ 1?
And, won't it break Xlib builds?

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 132120 [details] [diff] [review]
updated work-in-progress

oh, some more things...
+  nsCOMPtr<nsIMIMEInfo> mimeInfo = do_CreateInstance(NS_MIMEINFO_CONTRACTID);
as you have no early returns in this function, maybe you should use a raw
pointer and CallCreateInstance here (to save an addref/release pair that's
unnecessary)

+    gchar *commandPath = g_find_program_in_path(handlerApp->command);
...
+    NS_NewNativeLocalFile(handlerAppStr, PR_TRUE, getter_AddRefs(appFile));

aren't glib strings always in utf-8? so, shouldn't you use NS_NewLocalFile and
convert the string to ucs2?

Comment 30

[Brian Ryner (not reading)]

Attachment: patch addressing reviewer comments

Just a quick note on biesi's nsCOMPtr comment - I originally agreed, but I
ended up adding an early return when I fixed the encoding for the path string,
so I decided it was easier to keep the nsCOMPtr.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132281 [details] [diff] [review]
patch addressing reviewer comments

>Index: uriloader/exthandler/unix/nsGNOMERegistry.cpp
>+    mimeInfo->SetApplicationDescription(NS_ConvertUTF8toUCS2(handlerApp->name).get());

SetDefaultDescription(), please.

r=bzbarsky with that.

Comment 32

[Brian Ryner (not reading)]

checked in.

Comment 33

[Matthias Versen [:Matti]]

Comment on attachment 132120 [details] [diff] [review]
updated work-in-progress

removing obsolete review request