Jamie, any ideas about this?  It's not immediately obvious why it'd be crashing, and I can't see why we'd be calling gdk_pixbuf_new_from_file_at_scale with a bad filename either.

Not got a clue. I can't pretend that that's a particularly well written function (or entire file), but I don't understand why this crash has happened.

I'm going to be crazy busy this next week--my uni project is due on friday--but after that I can try to give it a more in depth look.

On the off-chance that the reporter hasn't used last.fm in rhythmbox since the crash happened, perhaps the contents of ~/.cache/rhythmbox could be useful.

I used rhythmbox after that crash.

I just disabled booklet plugin and not seen crash any more. Now I re-enabled it and it does not crash as well, however some images are not found and waiting animation is displayed.

So the crash is not reproducible.

Note that I don't have any last.fm profile photo uploaded.

Log shows lots of these errors:
(rhythmbox:31971): Rhythmbox-WARNING **: eel_strdup_strftime does not support non-standard escape code %e

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

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

Spent some time with my brother thinking this through and I think we've found the problem:

We initially make an array of RBAudioscrobblerUserData, eg my top tracks, from either the cache or a request to last.fm. If a track is missing an image a download is started.

Suppose whilst the image is downloading the profile gets refreshed. A new top tracks request is sent.

In top_tracks_response_cb (or equivalent): the old array of data is freed so the new array can be stored. The image being downloaded is for an RBAudioscrobblerUserData which no longer exists.

image_download_cb: Image download finishes, looks up data in file_to_data_map, but this is now garbage. Meaning data->image could be non-null but also not a valid gobject => g_object_unref segfaults.

calculate_cached_image_path: data is garbage meaning that data->type could be anything. The lack of a default block means image path remains NULL. This explains the warnings from GdkPixbuf.

Fixes I can think of are: a) Cancel the download before the RBAudioscrobblerUserData is freed. b) Again, before the RBAudioscrobblerUserData is freed, remove the download file from the file_to_data_map. Then in the callback handle such a case properly (by not crashing). The second has the advantage that the download is allowed to finish.

If this sounds sensible then patch forthcoming.

Yes, that sounds sensible.  I think allowing the download to finish would be better.  Looking at this code again, I suspect we're leaking a reference to the source file for the downloads too.

In download_image (), where I've written

	src_file = g_file_new_for_uri (image_url);

	/* only start a download if the file is not already being downloaded */
	if (g_hash_table_lookup (user->priv->file_to_data_map, src_file) == NULL) {
		<snip>
	}

does that actually make any sense? Even if that image url is being downloaded already, it'll be a different GFile object surely?

But suppose we were to actually do what this line is intended to, by having a url to gfile map and checking if a url lookup succeeds. Then if we were to do something along the lines of:

	/* only start a download if the file is not already being downloaded */
	src_file = g_hash_table_lookup (user->priv->url_to_file_map, image_url)
	if (src_file == NULL) {
		/* download file */
	} else {
		/* file's old data points to garbage. replace with new one */
		g_hash_table_insert (user->priv->file_to_data_map, src_file, data);
	}

I think this would also fix the bug, but I'm not 100% confident, and it's making my head hurt.
The callback wouldn't need to be edited (except to unref src_file, as you mentioned), nor would the array deletion code.

What do you think?

(In reply to comment #8)
> In download_image (), where I've written
> 
>     src_file = g_file_new_for_uri (image_url);
> 
>     /* only start a download if the file is not already being downloaded */
>     if (g_hash_table_lookup (user->priv->file_to_data_map, src_file) == NULL) {
>         <snip>
>     }
> 
> does that actually make any sense? Even if that image url is being downloaded
> already, it'll be a different GFile object surely?

Yes.. we could use g_file_hash and g_file_equal as the hash and equal functions for this hash table.  Probably use g_object_unref as the key destroy function too.

> But suppose we were to actually do what this line is intended to, by having a
> url to gfile map and checking if a url lookup succeeds. Then if we were to do
> something along the lines of:
> 
>     /* only start a download if the file is not already being downloaded */
>     src_file = g_hash_table_lookup (user->priv->url_to_file_map, image_url)
>     if (src_file == NULL) {
>         /* download file */
>     } else {
>         /* file's old data points to garbage. replace with new one */
>         g_hash_table_insert (user->priv->file_to_data_map, src_file, data);
>     }
> 
> I think this would also fix the bug, but I'm not 100% confident, and it's
> making my head hurt.
> The callback wouldn't need to be edited (except to unref src_file, as you
> mentioned), nor would the array deletion code.
> 
> What do you think?

I think we aren't guaranteed that an image that was needed before the top tracks response (or whatever) would be needed afterwards, so this wouldn't be enough by itself.

It seems like making RBAudioscrobblerUserData reference counted would fix this - the pointer arrays would have one reference, the download map would have another.  The download callback could check if the other reference was still alive before emitting the updated signals etc.

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

Created attachment 190294 [details] [review]
audioscrobbler: ensure RBAudioscrobblerUserData stays alive for image download

Make RBAudioscrobblerUserData refcounted and give the download map a reference.
Also make download maps responsible for unreffing their keys and values, and
fix a couple leaks.

Oops. Just to explain the patch a bit more in depth. I've basically done what you suggested in comment 9, and a couple other memory management fixes.

I have not done what I suggested in comment 8 as there can be more than one valid Data for each src_file. To fix this properly I think file_to_data_map would need its values to be lists of RBAudioscrobblerUserDatas rather than single ones. When the hash table lookup is succesful in download_image, append the data to the list. And in image_download_cb update the pixbuf and emit signals for each element (with refcount > 1).

For the time being though this should stop the segfault, but images will be missing for items if they were not the first item to require a download of the same image.

Created attachment 190321 [details] [review]
audioscrobbler: keep RBAudioscrobblerUserData alive for image download

Alternatively this patch keeps the hash tables using g_direct_hash instead of g_file_hash. The hash table lookup in download image is removed as it doesn't work with direct hash. Refcounting should fix the bug, and every item with an available image will download and display it. There will be multiple downloads of the same image, however.

I'm OK with either of these, but I think I prefer the first one.

Review of attachment 190321 [details] [review]:

ok

Review of attachment 190294 [details] [review]:

also ok

I'm backpacking at the moment and my only internet access is the occasional internet cafe. So I wont be able to commit anything for the next 3 or 4 weeks.

I've just pushed the first patch to master. I don't have permissions to mark the patches appropriately or close the bug, however.

Jamie, you should now be able to mark the patches as committed and to close the bug

Thanks! 1st patch commited, 2nd rejected. Bug fixed.