[Bug 63402] Avatar image caching is not asynchronous

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=63402

--- Comment #10 from Simon McVittie <simon.mcvittie at collabora.co.uk> ---
Comment on attachment 85964
  --> https://bugs.freedesktop.org/attachment.cgi?id=85964
Updated patch

Review of attachment 85964:
 --> (https://bugs.freedesktop.org/page.cgi?id=splinter.html&bug=63402&attachment=85964)
-----------------------------------------------------------------

::: telepathy-glib/contact.c
@@ +2814,5 @@
> +  GFile *file = G_FILE (source_object);
> +  gchar *mime_filename = NULL;
> +  GFile *mime_file = NULL;
> +
> +  g_array_unref (avatar_data->data);

I'd leave this in the struct, and do all the freeing in one go, at the end.

If you do want to free it here, use tp_clear_pointer or g_clear_pointer (or
explicitly set avatar_data->data to NULL) so you aren't pointing to
already-freed memory.

@@ +2830,5 @@
> +
> +  if (avatar_data->self != NULL)
> +    {
> +      tp_clear_object (&avatar_data->self->priv->avatar_file);
> +      avatar_data->self->priv->avatar_file = file;

You need to ref this when you store it in priv->avatar_file. That probably
explains the critical you saw.

@@ +2836,5 @@
> +
> +  /* Now write the mime-file. */
> +  if (!build_avatar_filename (avatar_data->connection, avatar_data->token,
> +      TRUE, NULL, &mime_filename))
> +    return;

In this early-return path, you don't free avatar_data or signal the change.
(But why would this fail anyway?)

I would suggest going back to doing a single build_avatar_filename() call in
contact_avatar_retrieved() for both the avatar filename and the MIME-type
filename, and putting either the MIME-type's path or its GFile in the struct
(I'd use the GFile, actually). That way, there's no way you can get an
unexpected failure here, so you don't need the ability to do an early-return.

@@ +2861,5 @@
>  {
>    TpContact *self = _tp_connection_lookup_contact (connection, handle);
> +  gchar *filename = NULL;
> +  GFile *avatar_file = NULL;
> +  WriteAvatarData *avatar_data = g_slice_new (WriteAvatarData);

I generally use g_slice_new0 so that if I forget to initialize a new field,
it's 0 or NULL, and I get a more deterministic crash (or even correct
behaviour, if NULL initialization was correct).

Declare this up here, but initialize it further down: otherwise...

@@ +2868,1 @@
>      return;

... early-returning here (which happens if g_mkdir_with_parents() fails) leaks
the WriteAvatarData.

@@ +2873,3 @@
>  
> +  avatar_data->data = g_array_new (FALSE, FALSE, sizeof (gchar));
> +  g_array_append_vals (avatar_data->data, avatar->data, avatar->len);

The reason you have to copy this (which, you're quite right, you do) is that
g_file_replace_contents_async() doesn't copy it.

I think that's a GIO bug, which we should open - how is Python/JS code expected
to ensure that the content "stays alive" like this? - but this is an
appropriate way to work around it.

@@ +2873,4 @@
>  
> +  avatar_data->data = g_array_new (FALSE, FALSE, sizeof (gchar));
> +  g_array_append_vals (avatar_data->data, avatar->data, avatar->len);
> +  avatar_data->self = self;

This should either ref it, or take a weak reference, if non-null. Otherwise,
things will go horribly wrong when you try to use it if it's already been
freed.

I'd suggest a GWeakRef, because the code being called already "does the right
thing" if there is no TpContact.

@@ +2873,5 @@
>  
> +  avatar_data->data = g_array_new (FALSE, FALSE, sizeof (gchar));
> +  g_array_append_vals (avatar_data->data, avatar->data, avatar->len);
> +  avatar_data->self = self;
> +  avatar_data->connection = connection;

This should ref it (because the code being called can't cope with there being
no connection).

@@ +2880,5 @@
>  
> +  g_file_replace_contents_async (avatar_file, avatar_data->data->data,
> +      avatar_data->data->len, NULL, FALSE,
> +      G_FILE_CREATE_PRIVATE|G_FILE_CREATE_REPLACE_DESTINATION, NULL,
> +      &avatar_file_written, avatar_data);

Style: you don't need the "&" to point to a function, and GLib code normally
doesn't. Just quoting the name of a function is enough to create a function
pointer.

::: tests/dbus/contacts.c
@@ +585,5 @@
>            g_main_loop_run (result.loop);
>          }
>  
> +      while (g_main_context_pending (NULL))
> +          g_main_context_iteration (NULL, TRUE);

I'd prefer this to wait for whichever signal you emit last (which I think is
notify::avatar-file here).

-- 
You are receiving this mail because:
You are the QA Contact for the bug.