After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 566747 - URIs opened with firefox %u load as local files (file:///...)
URIs opened with firefox %u load as local files (file:///...)
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
2.19.x
Other Linux
: Normal normal
: ---
Assigned To: gvfs-maint
gvfs-maint
Depends on:
Blocks:
 
 
Reported: 2009-01-06 10:02 UTC by Chris Coulson
Modified: 2009-02-04 08:02 UTC
See Also:
GNOME target: ---
GNOME version: 2.25/2.26


Attachments
Make g_file_get_path() return NULL for a GDummyFile that has a URI other than file:/// (840 bytes, patch)
2009-01-30 16:29 UTC, Chris Coulson
none Details | Review

Description Chris Coulson 2009-01-06 10:02:44 UTC
This was reported at https://bugs.edge.launchpad.net/ubuntu/+source/glib2.0/+bug/314263:

"[Problem]
'Open Link' in GNOME apps like gnome-terminal or xchat-gnome loads a "http://foo.com/bar" URI in firefox as "file:///bar".

[Analysis]
Between 2.18.3 and 2.19.2, the expand_macro() routine in glib's gdesktopappinfo.c altered how the %u macro is handled. It now treats them as %f macros instead. The new code that does this is shown below.

00558 /* On %u and %U, pass POSIX file path pointing to the URI via
00559 * the FUSE mount in ~/.gvfs. Note that if the FUSE daemon isn't
00560 * running or the URI doesn't have a POSIX file path via FUSE
00561 * we'll just pass the URI.
00562 */
00563 switch (macro)
00564 {
00565 case 'u':
00566 force_file_uri_macro = 'f';
00567 force_file_uri = TRUE;
00568 break;
00569 case 'U':
00570 force_file_uri_macro = 'F';
00571 force_file_uri = TRUE;
00572 break;
00573 default:
00574 force_file_uri_macro = macro;
00575 force_file_uri = FALSE;
00576 break;
00577 }

According to ps, I do have gvfs running:
bryce 5678 0.0 0.0 68884 2280 ? Ssl Jan04 0:00 /usr/lib/gvfs//gvfs-fuse-daemon /home/bryce/.gvfs

The ~/.gvfs directory is empty, and lsof does not show it in use. From the comment in the code above it sounds as if shutting down gvfs-fuse-daemon and/or gvfsd would cause it to not switch %u for %f, however killing either or both of them seems to have no effect."
Comment 1 Bryce Harrington 2009-01-06 17:01:10 UTC
As a test, I disabled the aforementioned code, and was not able to reproduce the problem subsequently.
Comment 2 Matthias Clasen 2009-01-08 02:16:39 UTC
Hmm, works fine here. CCing David, who did this stuff
Comment 3 David Zeuthen (not reading bugmail) 2009-01-08 02:49:53 UTC
Works fine for me too; maybe it's some patch in your downstream package or a version skew between GVfs and GIO versions (if you just updated your packages do remember to kill the gvfs daemons too). E.g., I think the upstream code is fine.

Either way, looks like you need to debug this some more. Can you add some debug code to the expand_macro_single() function to print the result of g_file_get_path()? It sounds like it's not returning NULL as it should (keep in mind GFile is an interface so the actual code being run is in GVfs)
Comment 4 Bryce Harrington 2009-01-08 23:24:40 UTC
> Works fine for me too; maybe it's some patch in your downstream package or a

Doubtful; ubuntu carries few patches against glib, none of which appear to have
anything to do with this issue.

> version skew between GVfs and GIO versions (if you just updated your packages
> do remember to kill the gvfs daemons too).

No, neither rebooting nor restarting gvfs have any affect; the bug occurs consistently
until I remove the code.  I also did an upgrade from glib2.0_2.19.3 to glib2.0_2.19.4,
rebooted, and still see the bug.

> E.g., I think the upstream code is fine.
> 
> Either way, looks like you need to debug this some more. Can you add some debug
> code to the expand_macro_single() function to print the result of
> g_file_get_path()? It sounds like it's not returning NULL as it should (keep in
> mind GFile is an interface so the actual code being run is in GVfs)

What sort of debug code would you like me to add?  I tried sticking some g_warnings()
in there, but it just makes stuff crash when I try loading URLs.
Comment 5 David Zeuthen (not reading bugmail) 2009-01-08 23:34:08 UTC
(In reply to comment #4)
> What sort of debug code would you like me to add?  I tried sticking some
> g_warnings()
> in there, but it just makes stuff crash when I try loading URLs.
> 

Try

 g_debug ("uri=%s -> path=%s", uri, path);

just after

 file = g_file_new_for_uri (uri);
 path = g_file_get_path (file);

in expand_macro_single().
Comment 6 Bryce Harrington 2009-01-09 01:28:59 UTC
(xchat-gnome:17656): GLib-GIO-DEBUG: uri=http://paste.ubuntu.com/102471/ -> path=/102471/

Comment 7 David Zeuthen (not reading bugmail) 2009-01-09 01:46:27 UTC
(In reply to comment #6)
> (xchat-gnome:17656): GLib-GIO-DEBUG: uri=http://paste.ubuntu.com/102471/ ->
> path=/102471/
> 

This is a bug in GVfs (since the GFile in question must be a GDaemonFile); g_file_get_path() should be returning NULL since there is no corresponding FUSE mount. It's interesting that it only happens on Ubuntu and not Fedora. Either way, reassigning to GVfs.
Comment 8 David Zeuthen (not reading bugmail) 2009-01-09 01:49:01 UTC
Bryce, also

 - How can I find broken-out patches for your gvfs package? I found only
   this page http://packages.ubuntu.com/jaunty/gvfs which wasn't helpful
   at all.

 - Any chance you can test with gvfs trunk?

Thanks.
Comment 9 David Zeuthen (not reading bugmail) 2009-01-09 01:59:07 UTC
Ah, I discovered

 http://patches.ubuntu.com/g/gvfs/extracted/

Nifty. Specifically, try dropping this patch

http://patches.ubuntu.com/g/gvfs/extracted/90_upstream_change_fix_rhythmbox_crasher.patch

I didn't look at it in detail but it looks kinda wrong and it's the only patch touching any code.
Comment 10 Bryce Harrington 2009-01-09 09:24:54 UTC
David,

Nope, I went through rebuilding gvfs with that patch omitted and unfortunately I still get the same behavior:

(xchat-gnome:9051): GLib-GIO-DEBUG: uri=https://wiki.ubuntu.com/X -> path=/X

Two of the patches were already disabled, so the only delta from your code is the ltmain.sh patch.
#01_maintainer_mode.patch
#90_relibtoolize.patch
99_ltmain_as-needed.patch
#90_upstream_change_fix_rhythmbox_crasher.patch
Comment 11 Martin Pitt 2009-01-30 09:48:31 UTC
Just talked to Bryce, and he says that the workaround he applied a while ago [1] was killed by a recent gvfs update, and the bug hasn't happened for him any more, so he considers this fixed.

[1] http://launchpadlibrarian.net/20935318/disable_posix_u_to_f.patch
Comment 12 Martin Pitt 2009-01-30 10:34:45 UTC
Sorry, that was a little premature. I can still reproduce it to 100% if I run gnome-open as user from root:

 # su -c 'gnome-open http://www.gnome.org' martin

or, if you are using sudo,

 sudo sudo -H -u '#1000' gnome-open http://www.gnome.org

This opens file:///home/martin for me.

I appreciate that this is not the most straightforward use case of gnome-open, but it worked in previous releases, and we use it to open web pages from applications which run as root through gksu (such as our crash reporting agent), in order to get the actual user's web browser preferences instead of root's.
Comment 13 Chris Coulson 2009-01-30 16:28:19 UTC
I've done some debugging on this and I think it is actually a glib bug and not a gvfs bug.

When we use gnome-open with sudo, g_vfs_get_default returns a GLocalVfs (not a GDaemonVfs), and g_file_new_for_uri returns a GDummyFile (not a GDaemonFile). I think the reason for this is that sudo scrubs DBUS_SESSION_BUS_ADDRESS from the environment, so the GVFS daemon is not contactable.

When you pass the GDummyFile to g_file_get_path in expand_macro_single, it returns a value even if the file doesn't exist in the filesystem. This causes the %U macro to be replaced with %F when it shouldn't be

In the normal use case when the GVFS daemon is contactable, a GDaemonFile is passed to g_file_get_path, and correctly returns NULL in the case of our http URI.

I've written a patch for glib which makes g_file_get_path return NULL when you pass it a GDummyFile that has a URI other than file:///, which seems to fix this problem
Comment 14 Chris Coulson 2009-01-30 16:29:30 UTC
Created attachment 127552 [details] [review]
Make g_file_get_path() return NULL for a GDummyFile that has a URI other than file:///
Comment 15 A. Walton 2009-01-30 16:54:00 UTC
Your patch essentially breaks GDummyFile's raison d'etre; GDummyFile should never be seeing local URIs anyways, as those are handled by GLocalFile, so essentially the only time GDummyFile gets used is when no other GFile implementation will handle the URI scheme, and something needs to be done with the URI.

Likely a better fix is determining whether we have a DummyFile on hand before we attempt to expand the macro; if we do, then we know to pass the URI as it is (which we should always be doing anyways for GDummyFiles) and not try the GVFS workaround. Piling up workarounds for bugs is counterproductive.
Comment 16 Chris Coulson 2009-01-30 17:11:51 UTC
I agree that piling up workarounds for bugs is counterproductive. The only reason I wrote the patch like this is because g_file_get_path returns NULL for GDaemonFile's that don't have a local path name, and this is what the gio documentation says that g_file_get_path does ("Gets the local pathname for GFile, if one exists")

So, my interpretation of the gio documentation was that the behaviour of g_file_get_path behaved incorrectly when passed a GDummyFile that didn't have a local pathname (as it returned a broken path for a http URI, when I expected it to return NULL). If what you're saying is true (that local URI's will be handled by a GLocalFile instead and GDummyFile won't handle local URI's), then shouldn't g_dummy_file_path just return NULL?

Sorry if that is interpreted incorrectly, and I'm open to a better way of fixing it :)
Comment 17 A. Walton 2009-01-30 17:26:32 UTC
Except it's not a broken path, it's exactly the correct path; the "Generic URI" case in the RFC says "scheme://authority/path/", and we're returning the "path" part, when we can make sense of the URI. Perhaps "local" is bad in the documentation (which is my fault) ;).

All file:/// URIs are handled by GLocalFile. Everything else is handled by GDummyFile /unless/ some other GFile claims it. When GVFS is properly loaded (GIO loads the GVFS module, the module communicates with the GVFS daemon and sets itself as the active VFS), then the GVFS daemon takes over for all of the URI schemes it understands, but in the case that there is one that even it doesn't understand, it passes back control to GIO, GDummyFile.

The GDummyFile class is the "catch-all-else" class; it tries to work when nothing else does. For "generic URIs", it parses them and returns the parts as expected. GDummyFile should only return NULL on get_path() when there is nothing else sensible for it to return (e.g. if someone tried to use a data: or mailto: URI through GIO).

When you walk through the list in GDesktopAppInfo, if you get a GDummyFile, just put it in as it is, since that's the right thing to do (esp. in the case of a "mailto:" URI, though I don't think anyone has actually tried doing this through GIO yet). Don't do the whole get_uri()->get_path()->new_for_path() chain.
Comment 18 A. Walton 2009-01-30 17:28:40 UTC
ahem, chain dyslexia, get_path()->new_for_path()->get_uri().
Comment 19 Chris Coulson 2009-01-30 17:39:23 UTC
I think I understand now :) So, my patch works around it, but is still wrong.

So, the better way to do this then is to just leave the macro's as they are in expand_macro_single when the GFile is a GDummyFile?

Comment 20 David Zeuthen (not reading bugmail) 2009-01-31 16:43:54 UTC
(In reply to comment #17)
> Except it's not a broken path, it's exactly the correct path; the "Generic URI"
> case in the RFC says "scheme://authority/path/", and we're returning the "path"
> part, when we can make sense of the URI. Perhaps "local" is bad in the
> documentation (which is my fault) ;).

I'm not exactly sure what you are saying here but it sounds like it contradicts how g_file_get_path() is supposed to work: it MUST return NULL unless it returns a POSIX path for the file that can be used with regular POSIX calls, e.g. open(2).

The docs sorta-kinda states this

http://library.gnome.org/devel/gio/unstable/GFile.html#g-file-get-path

since it says "local pathname". Perhaps we need to fix the docs so it's completely clear that this is the semantics of g_file_get_path().
Comment 21 A. Walton 2009-01-31 17:33:08 UTC
(In reply to comment #20)
> (In reply to comment #17)
> > Except it's not a broken path, it's exactly the correct path; the "Generic URI"
> > case in the RFC says "scheme://authority/path/", and we're returning the "path"
> > part, when we can make sense of the URI. Perhaps "local" is bad in the
> > documentation (which is my fault) ;).
> 
> I'm not exactly sure what you are saying here but it sounds like it contradicts
> how g_file_get_path() is supposed to work: it MUST return NULL unless it
> returns a POSIX path for the file that can be used with regular POSIX calls,
> e.g. open(2).
> 
> The docs sorta-kinda states this
> 
> http://library.gnome.org/devel/gio/unstable/GFile.html#g-file-get-path
> 
> since it says "local pathname". Perhaps we need to fix the docs so it's
> completely clear that this is the semantics of g_file_get_path().
> 

Seeing as I think I remember writing that myself, I was convinced that the documentation is wrong there and didn't accurately reflect what the code does. In every other case the dummy file implementation acts as a "generic URI" parser, which makes the path part of the URI seem more correct to return.

But if I'm totally wrong here, the patch needs work to just simply return NULL unconditionally and to document that's what we really want to do.
Comment 22 Alexander Larsson 2009-02-04 07:56:19 UTC
Davidz is right, g_file_get_path() should only return non-NULL for local files and for things that can convert to local files (i.e. fuse mounts).

So, g_dummy_file_get_path() should just return NULL.

Comment 23 Alexander Larsson 2009-02-04 08:02:39 UTC
2009-02-04  Alexander Larsson  <alexl@redhat.com>

        Bug 566747 - URIs opened with firefox %u load as local files

        * gdummyfile.c (g_dummy_file_get_path):
        Dummy files are never used for local paths, so always return NULL
        in get_path().