Ugh. This is a regression introduced by recent accountsservice changes. When changing the icon, rather than passing the new url around (which could be unavailable when the icon is needed), accountsservice copies the file to /var/lib/AccountsService/icons/$USER and emits the user::changed signal. We handle this signal and update the icon as expected - so far, so good. But for performance reasons we cache texture data, and this is what is triggering the issue here - all of texture type, file name and image size being equal, we return the cached texture.

Not sure off-hand what would be the best fix, I see various alternatives:

 - change accountsservice to not use a single per-user icon file, e.g.
   copy icons to /var/lib/AccountsService/icons/$USER-$UNIQUE_ID instead;
   this would require accountsservice to add some policy for cleaning up
   older icons, which might be undesirable

 - also use mtime in the cache key for URLs; while this should fix the issue
   rather transparently, stat isn't free ...

 - add explicit API to StTextureCache to enforce a cache policy for specific
   uris; I'm not a big fan of this approach, but I can't think of anything
   better if (1) is not an option ...

We could also have the texture cache automatically GFileMonitor watch on local paths.

True, though we would still need to expose some API to StThemeNode (we try to only emit widget::style-changed signals on actual style changes, which means we'll miss a "change" from "background-image: url(/var/lib/AccountsService/icons/$USER);" to "background-image: url(/var/lib/AccountsService/icons/$USER);")

No we won't -- as soon as accountsservice updates the picture, the file monitor will fire, the texture-cache will invalidate the picture in the cache, fire a cache-invalidated (a renamed icon-theme-changed signal), and we should be good.

Ugh, so you are suggesting to throw away *all* cached textures because a *single* file changed?

No? It will only evict one texture. It will traverse the entire scene graph sending "UPDATED", though. I don't see any other way to do it, other than tying a backreference to a widget to a texture.

> But for
> performance reasons we cache texture data, and this is what is triggering the
> issue here - all of texture type, file name and image size being equal, we
> return the cached texture.

Presumably the mtime of the file does change though, no ?

With the avatar in the lock dialog, this is rather visible. Can we not do the simple fix of including the timestamp in the key here, please ?

(In reply to comment #8)
> With the avatar in the lock dialog, this is rather visible. Can we not do the
> simple fix of including the timestamp in the key here, please ?

No - it is not that simple. Apart from disk accesses that a cache is supposed to avoid, we do not necessarily hit the texture cache at all - on style changes, before the old style is cleared, state that is reusable is copied over to the new style. For background-images, that is obviously based on the the paths being the same.

Now the good news is that an implementation based on GFileMonitor is actually easier than the mtime approach - patch coming (once I write up the commit message).

Created attachment 224858 [details] [review]
st-texture-cache: Add texture-file-changed signal

For textures loaded from files, the cache might hide image changes
by keeping the data of a previous version around indefinitely. For
instance AccountsService will notify of avatar changes, but as new
image is copied over the old one, we will continue to use the old
image data.
Install a file monitor for each file resource we load and clear
the corresponding data from the cache on changes, emitting the
new StTextureCache::texture-file-changed signal.

Created attachment 224859 [details] [review]
st-theme-node: Add method to invalidate drawing state

StThemeNode caches its resources aggressively to keep the required
work on paint to a minimum - right now, resources are only recreated
on allocation changes.
In order to update the background-image property correctly when the
underlying file changes, resources need to be recreated without a
size change, so add an explicit method for that.

Created attachment 224860 [details] [review]
st-widget: Keep background-image and border-image updated

Currently we miss changes to a file referenced in background-image
or border-image.
Connect to the StTextureCache::texture-file-changed signal to keep
up with file changes and update the drawing state if necessary.

*** Bug 644125 has been marked as a duplicate of this bug. ***

Created attachment 224867 [details] [review]
st-widget: Keep background-image and border-image updated

Gah, did some last-minute changes to apply to border-images as well and didn't retest.

This still seems much more complicated and expensive than necessary to fix avatars. 

What I would have expected: When you get a user::changed signal from accountsservice, you invalidate the cache entry for the user avatar. Problem solved.

What am I missing ?

(In reply to comment #15)
> What I would have expected: When you get a user::changed signal from
> accountsservice, you invalidate the cache entry for the user avatar. Problem
> solved.

If you mean for 3.6, then I'm OK with that. I still like the more general approach, as it has the side effect of fixing a long-standing annoyance for the design team (e.g. having to restart the shell continuously while working on shell artwork).

Comment on attachment 224858 [details] [review]
st-texture-cache: Add texture-file-changed signal

Marking "reviewed" to get this off the list for now

Comment on attachment 224867 [details] [review]
st-widget: Keep background-image and border-image updated

Dto.

Created attachment 224929 [details] [review]
st-texture-cache: Add API to remove cache data

The current API assumes that image data loaded from files remains
valid during the life time of the shell. This assumption is mostly
valid for image files we provide ourselves (with the exception being
designers working on those files), but not necessarily for "external"
files - provide API to explicitly remove cached data associated with
a URI for those cases.

Created attachment 224930 [details] [review]
st-widget: Add method to clear background-image

For performance reasons, resources required to paint a widget are
aggressively cached; we know of at least one case where our caching
prevents updating the used background-image correctly, so add explicit
API to clear all associated cache data.

Created attachment 224931 [details] [review]
userMenu: Force reload of background-image on icon changes

When changing the user's avatar image, AccountsService will
overwrite the old image with the new one, so the location
returned by get_icon_file() is always the same.
In order to pick up the change, we need to make sure to clear the
previous image from both StTextureCache and StThemeNode's paint
cache.

Review of attachment 224929 [details] [review]:

::: src/st/st-texture-cache.c
@@ +97,3 @@
+void
+st_texture_cache_clear_uri (StTextureCache *cache,
+                            const char     *uri)

Could use a bit of gtk-doc, like:

/**
 * st_texture_cache_clear_uri:
 * @cache: a #StTextureCache
 * @uri: URI of cached object
 *
 * If the given @uri is known to have been modified
 * externally, this function may be used to invalidate
 * the in-memory cache.
 */

Review of attachment 224930 [details] [review]:

Makes sense.

Review of attachment 224931 [details] [review]:

Could use a comment above (link to this bug?)

Otherwise looks obviously right.

(In reply to comment #22)
> Could use a bit of gtk-doc

Yeah, though I'd like to switch to the generic approach post-3.6, which would make this function obsolete ...

Created attachment 224956 [details] [review]
st-texture-cache: Add API to remove cache data

Added the suggested comment.

Created attachment 224957 [details] [review]
userMenu: Force reload of background-image on icon changes

Added the suggested comment - note that

  st-theme-node: Add method to invalidate drawing state

is still required by the non-generic approach, so review appreciated :-)

Review of attachment 224956 [details] [review]:

Sane enough.

Review of attachment 224859 [details] [review]:

Can we make this part of st-theme-node-private.h?

Review of attachment 224957 [details] [review]:

Sure.

Works fine in brief testing.

Attachment 224859 [details] pushed as c4c470c - st-theme-node: Add method to invalidate drawing state
Attachment 224930 [details] pushed as cd024e2 - st-widget: Add method to clear background-image
Attachment 224956 [details] pushed as dc9ad8d - st-texture-cache: Add API to remove cache data
Attachment 224957 [details] pushed as 6f3cf0a - userMenu: Force reload of background-image on icon changes

(In reply to comment #29)
> Can we make this part of st-theme-node-private.h?

st-theme-node-private.h is used to share code between st-theme-node.c and st-theme-node-drawing.c - the method in question is intended to be used from st-widget.c, so I'm leaving it in the regular header.

Created attachment 225098 [details] [review]
userMenu: Rely on automatic texture changes

This reverts commit 6f3cf0ae50af0f7a9cd68760abe695009605c0d7.

Created attachment 225099 [details] [review]
st: Remove unused methods

This reverts commits cd024e21f0d and dc9ad8df8074.

Review of attachment 224867 [details] [review]:

::: src/st/st-widget.c
@@ +284,3 @@
+    return;
+
+  path = strstr (uri, "://") + strlen ("://");

g_filename_from_uri? _st_theme_resolve_url? This won't unescape the URI.

@@ +286,3 @@
+  path = strstr (uri, "://") + strlen ("://");
+
+  changed = g_strcmp0 (st_theme_node_get_background_image (node), path) == 0;

st_theme_node_get_background_image is a direct translation of the CSS. If it's relative, then this won't work. We should probably make _st_theme_resolve_url call realpath.

Review of attachment 224858 [details] [review]:

::: src/st/st-texture-cache.c
@@ +45,3 @@
+
+  /* File monitors to evict cache data on changes */
+  GHashTable *file_monitors; /* GFile * -> GFileMonitor * */

char * -> GFileMonitor *

@@ +1034,3 @@
+  char *uri, *key;
+
+  if (event_type != G_FILE_MONITOR_EVENT_CREATED &&

EVENT_CHANGED?

Review of attachment 225099 [details] [review]:

Sure.

Review of attachment 225098 [details] [review]:

Sure.

Created attachment 226676 [details] [review]
st-texture-cache: Add texture-file-changed signal

Updated according to review.

Review of attachment 226676 [details] [review]:

Sure.

Created attachment 226678 [details] [review]
st-widget: Keep background-image and border-image updated

(In reply to comment #35)
> g_filename_from_uri? _st_theme_resolve_url? This won't unescape the URI.

Sure.


> @@ +286,3 @@
> +  path = strstr (uri, "://") + strlen ("://");
> +
> +  changed = g_strcmp0 (st_theme_node_get_background_image (node), path) == 0;
> 
> st_theme_node_get_background_image is a direct translation of the CSS. If it's
> relative, then this won't work. We should probably make _st_theme_resolve_url
> call realpath.

Uh? Either _st_theme_resolve_url() translates the CSS to an absolute path, or returns NULL on failure - so st_theme_node_get_background_image() will never be relative (FWIW, given our current style, the patch was *only* tested with relative URLs ...)

Created attachment 226679 [details] [review]
st: Fix handling of file:// URIs in _st_theme_resolve_uri()

Unrelated drive-by fix.

Review of attachment 226679 [details] [review]:

Sure.

(In reply to comment #41)
> Uh? Either _st_theme_resolve_url() translates the CSS to an absolute path, or
> returns NULL on failure - so st_theme_node_get_background_image() will never be
> relative (FWIW, given our current style, the patch was *only* tested with
> relative URLs ...)

As far as I can tell, _st_theme_resolve_url does not actually translate to an absolute path. If we have url(../one/two.png);, it will lookup the stylesheet URL, get the basedir of that, and then use g_build_filename on that, which doesn't do any sort of resolution:

>>> GLib.build_filenamev(['directory/containing/a/stylesheet', '../one/two.png'])
'directory/containing/a/stylesheet/../one/two.png'

Created attachment 226688 [details] [review]
st: Canonicalize URLs in stylesheets

Make _st_theme_resolve_url() a bit smarter by canonicalizing the
resulting path (e.g. resolving references to /./ and /../).


(In reply to comment #44)
> As far as I can tell, _st_theme_resolve_url does not actually translate to an
> absolute path. If we have url(../one/two.png);, it will lookup the stylesheet
> URL, get the basedir of that, and then use g_build_filename on that, which
> doesn't do any sort of resolution:
> 
> >>> GLib.build_filenamev(['directory/containing/a/stylesheet', '../one/two.png'])
> 'directory/containing/a/stylesheet/../one/two.png'

Which is an absolute path (e.g. it's actually /directory/containing ...) :-)

But yes, there's no canonicalization, so we miss updates in this case. To be honest, I don't think that'll be much of a problem in practice - images changing at runtime and references to . or .. in urls are both uncommon cases, more so the combination of the two. Not to mention that the effect will almost always be nothing but a bit less convenience for the developer ...

In any case, here's a patch to handle this anyway.

Review of attachment 226688 [details] [review]:

::: src/st/st-theme.c
@@ +1019,3 @@
   char *dirname;
   char *filename;
+  char canonicalized_path[PATH_MAX];

See the BUGS section in realpath(3) for why this isn't acceptable.

Created attachment 226690 [details] [review]
st: Canonicalize URLs in stylesheets

Ugh, the memory handling is getting so ugly that I begin to prefer not handling the case ...

Comment on attachment 226679 [details] [review]
st: Fix handling of file:// URIs in _st_theme_resolve_uri()

Attachment 226679 [details] pushed as 0ea8217 - st: Fix handling of file:// URIs in _st_theme_resolve_uri()

Review of attachment 226690 [details] [review]:

I was thinking we could just rely on the standard allocator being there in glib, since I don't know any program out there that changes the malloc tables of glib anymore.

Review of attachment 226678 [details] [review]:

This makes sense to me.

Created attachment 226695 [details] [review]
st: Canonicalize URLs in stylesheets

(In reply to comment #49)
> Review of attachment 226690 [details] [review]:
> 
> I was thinking we could just rely on the standard allocator being there in
> glib

*and* no one changing the standard allocator from malloc, so no, let's not do this unconditionally.

(In reply to comment #51)
> Created an attachment (id=226695) [details] [review]
> st: Canonicalize URLs in stylesheets

I was thinking of this in terms of a code cleanup, but that's not what this is. Both patches are ACN from me, choose whichever one you prefer.

> *and* no one changing the standard allocator from malloc, so no, let's not do
> this unconditionally.

Do people do this, and how does it happen. Now that g_type_init is automatic, and glibc's allocator has gotten better, I wonder if it makes sense to deprecate the gmem (and possibly gslice) infrastructure entirely.

Attachment 225098 [details] pushed as f18fd8d - userMenu: Rely on automatic texture changes
Attachment 225099 [details] pushed as d3ba002 - st: Remove unused methods
Attachment 226676 [details] pushed as 9c8b752 - st-texture-cache: Add texture-file-changed signal
Attachment 226678 [details] pushed as d54f7b1 - st-widget: Keep background-image and border-image updated
Attachment 226695 [details] pushed as 15273c7 - st: Canonicalize URLs in stylesheets



(In reply to comment #52)
> I was thinking of this in terms of a code cleanup, but that's not what this is.

That's why I chose to rely on certain libc implementation details originally :-)


> > *and* no one changing the standard allocator from malloc, so no, let's not do
> > this unconditionally.
> 
> Do people do this, and how does it happen.

I was referring to the hypothetical case where someone changes the allocator in glib itself, let's say 2 years from now - I can guarantee you that I won't remember that this breaks an old assumption we made and requires updating old code.