telepathy: Correctly maintain strong refs on TpContacts in property accessors

While performing operations on a TpContact’s properties, folks should hold
a strong ref on the TpContact (even if it doesn’t normally hold one).
This expands on commit b251bcb92a343b051e62d458b66de4a9d3011f82 to fix folks
to correctly hold a strong reference in such situations.

This also fixes a potential bug where changing a Tpf.Persona’s groups while
the persona was being served out of the cache would cause a crash (null
pointer dereference).

This adds the following new API:
 • PropertyError.UNAVAILABLE

See: https://bugzilla.gnome.org/show_bug.cgi?id=680335

diff --git a/NEWS b/NEWS
index 7cccf6b..3b7c94b 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -16,6 +16,9 @@ Bugs fixed:
 • Bug 680335 — empathy crashed with SIGSEGV in
   _tpf_persona_contact_weak_notify_cb()
 
+API changes:
+• Add PropertyError.UNAVAILABLE
+
 Overview of changes from libfolks 0.7.2 to libfolks 0.7.3
 =========================================================
 

diff --git a/backends/telepathy/lib/tpf-persona.vala b/backends/telepathy/lib/tpf-persona.vala
index 58118bb..2e3249a 100644 (file)
--- a/backends/telepathy/lib/tpf-persona.vala
+++ b/backends/telepathy/lib/tpf-persona.vala
@@ -486,15 +486,29 @@ public class Tpf.Persona : Folks.Persona,
   public async void change_group (string group, bool is_member)
       throws GLib.Error
     {
+      /* Ensure we have a strong ref to the contact for the duration of the
+       * operation. */
+      var contact = (Contact?) this._contact.get ();
+
+      if (contact == null)
+        {
+          /* The Tpf.Persona is being served out of the cache. */
+          throw new PropertyError.UNAVAILABLE (
+              _("Failed to change group membership: %s"),
+              /* Translators: "account" refers to an instant messaging
+               * account. */
+              _("Account is offline."));
+        }
+
       try
         {
           if (is_member && !this._groups.contains (group))
             {
-              yield this.contact.add_to_group_async (group);
+              yield contact.add_to_group_async (group);
             }
           else if (!is_member && this._groups.contains (group))
             {
-              yield this.contact.remove_from_group_async (group);
+              yield contact.remove_from_group_async (group);
             }
         }
       catch (GLib.Error e)
@@ -547,9 +561,21 @@ public class Tpf.Persona : Folks.Persona,
    */
   public async void change_groups (Set<string> groups) throws PropertyError
     {
+      var contact = (Contact?) this._contact.get ();
+
+      if (contact == null)
+        {
+          /* The Tpf.Persona is being served out of the cache. */
+          throw new PropertyError.UNAVAILABLE (
+              _("Failed to change group membership: %s"),
+              /* Translators: "account" refers to an instant messaging
+               * account. */
+              _("Account is offline."));
+        }
+
       try
         {
-          yield this.contact.set_contact_groups_async (groups.to_array ());
+          yield contact.set_contact_groups_async (groups.to_array ());
         }
       catch (GLib.Error e)
         {
@@ -758,22 +784,27 @@ public class Tpf.Persona : Folks.Persona,
 
       /* Contact can be null if we've been created from the cache. All the code
        * below this point is for non-cached personas. */
-      if (this.contact == null)
+      var contact = (Contact?) this._contact.get ();
+
+      if (contact == null)
         {
           return;
         }
 
       /* Set our alias. */
-      this._alias = this.contact.get_alias ();
+      this._alias = contact.get_alias ();
 
-      this.contact.notify["alias"].connect ((s, p) =>
+      contact.notify["alias"].connect ((s, p) =>
           {
+            var c = (Contact?) this._contact.get ();
+            assert (c != null); /* should never be called while cached */
+
             /* Tp guarantees that aliases are always non-null. */
-            assert (this.contact.alias != null);
+            assert (c.alias != null);
 
-            if (this._alias != this.contact.alias)
+            if (this._alias != c.alias)
               {
-                this._alias = this.contact.alias;
+                this._alias = c.alias;
                 this.notify_property ("alias");
 
                 /* Mark the cache as needing to be updated. */
@@ -782,7 +813,7 @@ public class Tpf.Persona : Folks.Persona,
           });
 
       /* Set our single IM address */
-      var connection = this.contact.connection;
+      var connection = contact.connection;
       var account = connection.get_account ();
 
       try
@@ -798,21 +829,21 @@ public class Tpf.Persona : Folks.Persona,
           warning (e.message);
         }
 
-      this.contact.notify["avatar-file"].connect ((s, p) =>
+      contact.notify["avatar-file"].connect ((s, p) =>
         {
           this._contact_notify_avatar ();
         });
       this._contact_notify_avatar ();
 
-      this.contact.notify["presence-message"].connect ((s, p) =>
+      contact.notify["presence-message"].connect ((s, p) =>
         {
           this._contact_notify_presence_message ();
         });
-      this.contact.notify["presence-type"].connect ((s, p) =>
+      contact.notify["presence-type"].connect ((s, p) =>
         {
           this._contact_notify_presence_type ();
         });
-      this.contact.notify["presence-status"].connect ((s, p) =>
+      contact.notify["presence-status"].connect ((s, p) =>
         {
           this._contact_notify_presence_status ();
         });
@@ -820,17 +851,17 @@ public class Tpf.Persona : Folks.Persona,
       this._contact_notify_presence_type ();
       this._contact_notify_presence_status ();
 
-      this.contact.notify["contact-info"].connect ((s, p) =>
+      contact.notify["contact-info"].connect ((s, p) =>
         {
           this._contact_notify_contact_info (false);
         });
       this._contact_notify_contact_info (false);
 
-      this.contact.contact_groups_changed.connect ((added, removed) =>
+      contact.contact_groups_changed.connect ((added, removed) =>
         {
           this._contact_groups_changed (added, removed);
         });
-      this._contact_groups_changed (this.contact.get_contact_groups (), {});
+      this._contact_groups_changed (contact.get_contact_groups (), {});
 
       var tpf_store = this.store as Tpf.PersonaStore;
 
@@ -918,6 +949,13 @@ public class Tpf.Persona : Folks.Persona,
           this._phone_numbers_ro = this._phone_numbers.read_only_view;
         }
 
+      var contact = (Contact?) this._contact.get ();
+      if (contact == null)
+        {
+          /* If operating from the cache, bail out early. */
+          return;
+        }
+
       var changed = false;
       var new_birthday_str = "";
       var new_full_name = "";
@@ -931,7 +969,7 @@ public class Tpf.Persona : Folks.Persona,
           (GLib.HashFunc) UrlFieldDetails.hash,
           (GLib.EqualFunc) UrlFieldDetails.equal);
 
-      var contact_info = this.contact.get_contact_info ();
+      var contact_info = contact.get_contact_info ();
       foreach (var info in contact_info)
         {
           if (info.field_name == "") {}
@@ -1180,7 +1218,7 @@ public class Tpf.Persona : Folks.Persona,
     {
       debug ("Destroying Tpf.Persona '%s': %p", this.uid, this);
 
-      var contact = this._contact.get ();
+      var contact = (Contact?) this._contact.get ();
       if (contact != null)
         {
           contact.weak_unref (this._contact_weak_notify_cb);
@@ -1189,18 +1227,24 @@ public class Tpf.Persona : Folks.Persona,
 
   private void _contact_notify_presence_message ()
     {
-      this.presence_message = this.contact.get_presence_message ();
+      var contact = (Contact?) this._contact.get ();
+      assert (contact != null); /* should never be called while cached */
+      this.presence_message = contact.get_presence_message ();
     }
 
   private void _contact_notify_presence_type ()
     {
+      var contact = (Contact?) this._contact.get ();
+      assert (contact != null); /* should never be called while cached */
       this.presence_type = Tpf.Persona._folks_presence_type_from_tp (
-          this.contact.get_presence_type ());
+          contact.get_presence_type ());
     }
 
   private void _contact_notify_presence_status ()
     {
-      this.presence_status = this.contact.get_presence_status ();
+      var contact = (Contact?) this._contact.get ();
+      assert (contact != null); /* should never be called while cached */
+      this.presence_status = contact.get_presence_status ();
     }
 
   private static PresenceType _folks_presence_type_from_tp (
@@ -1233,8 +1277,11 @@ public class Tpf.Persona : Folks.Persona,
 
   private void _contact_notify_avatar ()
     {
-      var file = this.contact.avatar_file;
-      var token = this.contact.avatar_token;
+      var contact = (Contact?) this._contact.get ();
+      assert (contact != null); /* should never be called while cached */
+
+      var file = contact.avatar_file;
+      var token = contact.avatar_token;
       Icon? icon = null;
       var from_cache = false;
 

diff --git a/folks/persona.vala b/folks/persona.vala
index 9cb17da..ac8c2e5 100644 (file)
--- a/folks/persona.vala
+++ b/folks/persona.vala
@@ -48,7 +48,17 @@ public errordomain Folks.PropertyError
    *
    * @since 0.6.2
    */
-  UNKNOWN_ERROR
+  UNKNOWN_ERROR,
+
+  /**
+   * The backing store is offline or otherwise unavailable.
+   *
+   * This is a temporary error which should be retifiable by going online or
+   * ensuring the backing store is logged in.
+   *
+   * @since UNRELEASED
+   */
+  UNAVAILABLE
 }
 
 /**