This should be a bit less of a problem after applying the patch "Keep similar theme nodes so we don't have to recompute CSS so often" from Bug #687465: realpath() is still called once for each StThemeNode that has a background/border/etc. image, but that patch reduces the number of distinct StThemeNodes, and hence the number of realpath() calls.

I didn't know that realpath actually called stat(); I thought it just did path manipulation only. Is there something like abspath or similar which has the effect of canonicalizing the path without the additional stat?

(In reply to comment #2)
> I didn't know that realpath actually called stat(); I thought it just did path
> manipulation only.

It's documented to chase symlinks, which can't be done without I/O (either lstat() or readlink()). I don't know a direct C equivalent of Python's os.path.abspath().

Created attachment 229610 [details] [review]
St: avoid blocking IO to resolve relative urls

realpath() does a series of lstat() on each path component to resolve
symbolic links, but we just want to get an absolute path, and we don't
really care if it is physical or not. Going through a GFile does the
canonicalization we need, and is a lot faster.

Turns out that there is a C equivalent of os.path.abspath().
I'm using GFile in _resolve_url() because eventually I want to support
arbitrary GFiles as stylesheets and textures, and most importantly
GResourceFiles

Review of attachment 229610 [details] [review]:

::: src/st/st-theme-node.c
@@ +1750,3 @@
+                                                term->content.str->stryng->str);
+
+                  node->background_image = g_strdup (g_file_get_path (file));

Ditto here.

@@ +1860,3 @@
+                                            decl->value->content.str->stryng->str);
+
+              node->background_image = g_strdup (g_file_get_path (file));

And here

@@ +2757,3 @@
 
+          file = _st_theme_resolve_url (node->theme, base_stylesheet, url);
+          filename = g_strdup (g_file_get_path (file));

And here

::: src/st/st-theme.c
@@ +856,3 @@
+                                                  a_nodesheet,
+                                                  import_rule->url->stryng->str);
+                    filename = g_strdup (g_file_get_path (file));

g_file_get_path () already copies (unfortunately...)

See also http://git.gnome.org/browse/ostree/tree/src/libotutil/ot-gio-utils.c?id=f2b1be6a1eb8a42b2f5b1f187d4b5d37b7510b88#n264

Ok, I'll get at the review comments, but just in case someone else was about to work on this, I started a big rewriting of the StTheme code, to reduce the number of string comparisons and linear searches required to build a StThemeNode.

Created attachment 229751 [details] [review]
St: avoid blocking IO to resolve relative urls

realpath() does a series of lstat() on each path component to resolve
symbolic links, but we just want to get an absolute path, and we don't
really care if it is physical or not. Going through a GFile does the
canonicalization we need, and is a lot faster.

Review of attachment 229751 [details] [review]:

::: src/st/st-theme.c
@@ +1032,2 @@
+      dirname = g_path_get_dirname (base_filename);
+      stylesheet = g_file_new_for_path (dirname);

I wonder if we should cache this in the stylesheet private instead...

@@ +1039,3 @@
   else
     {
+      resource = g_file_new_for_path (url);

When could this happen? We really shouldn't allow arbitrary images from HTTP or other URLs, or else somebody could make a goatse theme or something.

Is this trying to support a use case like Gravatars in the user menu or similar?

(In reply to comment #8)
> @@ +1039,3 @@
>    else
>      {
> +      resource = g_file_new_for_path (url);
> 
> When could this happen? We really shouldn't allow arbitrary images from HTTP or
> other URLs, or else somebody could make a goatse theme or something.
> 
> Is this trying to support a use case like Gravatars in the user menu or
> similar?

Gravatar support would have copied the image locally, so that you didn't have to have a working network to see your face in GDM.

In this patch, there seem to be three cases. In pseudocode:

if (scheme)
  {
    the stylesheet said something like file:///usr/.../foo.png
    (OK in principle) or possibly a http or other URI (bad)
  }
else if (base_stylesheet)
  {
    the stylesheet said something relative like bar/foo.png or ../foo.png,
    or something absolute like /usr/.../foo.png; either way,
    resolve it relative to the stylesheet
  }
else
  {
    ... if we have no stylesheet, where did we get this image from?
  }

I would expect this to work more like this:

if (scheme)
  {
    if (scheme is file, case-insensitively)
      {
        the path part is an absolute filename
      }
    else
      {
        fatal error ("your theme tries to access the internet or something")
        or perhaps just ignore this image
      }
  }
else if (path is absolute)
  {
    use it
  }
else if (base_stylesheet)
  {
    the path part is a filename relative to the base_stylesheet, resolve
    it to absolute
  }
else
  {
    either fatal error ("we shouldn't get here without a stylesheet")
    or assume the path is relative to getcwd()?
  }

Uhm, for some reason I didn't get bugzilla mails for this...
Anyway, as I said earlier, the point of using GFile in place of paths is that it allows us to use GResource for the main theme, and bundle the One and Only theme in a fat binary, with one i-node and zero system calls to load it.
That's pending on using GFile to load and feed libcroco the CSS file, which I started to write but then turned in essentially a rewrite of the St theming system.

OK, that makes sense. I'd still like to prevent users loading from http:// URLs or similar. Lock down URLs to file://

I don't understand why this lockdown. Of course it's a stupid idea to load images from http://, but the theme author would notice it the moment the shell takes 20 seconds of synchronous HTTP code just to load (or worse, fails horribly if network is not available...).
I mean, there's nothing wrong with letting someone shoot on his foot. It's his (questionable) decision.

(In reply to comment #13)
> I don't understand why this lockdown. Of course it's a stupid idea to load
> images from http://, but the theme author would notice it the moment the shell
> takes 20 seconds of synchronous HTTP code just to load (or worse, fails
> horribly if network is not available...).
> I mean, there's nothing wrong with letting someone shoot on his foot. It's his
> (questionable) decision.

I've already seen multiple extensions that do this with Gio.FileIcon, but yeah, maybe it's worth letting him shoot his foot off.

I was concerned about goatse, not slowness.

(In reply to comment #10)
> else if (path is absolute)
>   {
>     use it
>   }

This is the only path missing at this point, and I'm not sure it's worthwhile to keep around. Will re-review the patch.

Review of attachment 229751 [details] [review]:

OK.

Comment on attachment 229751 [details] [review]
St: avoid blocking IO to resolve relative urls

Attachment 229751 [details] pushed as f7af96d - St: avoid blocking IO to resolve relative urls
Leaving the bug open for the announced GResource work, and maybe something
more...

The committed patch does

+                  node->background_image = g_strdup (g_file_get_path (file));

in a lot of places - AFAICS this will leak the file path every time since g_file_get_path() allocates a new string.

Gah, Giovanni! You commited the wrong patch!

Created attachment 231658 [details] [review]
St: fix regression from f7af96dbb2152c796c5877585e7d1e555471d6ce

A bad rebase caused the wrong patch to be pushed.

Review of attachment 231658 [details] [review]:

OK.

Attachment 231658 [details] pushed as 9860b1c - St: fix regression from f7af96dbb2152c796c5877585e7d1e555471d6ce