Fix set implementation to be truly threadsafe even with destroy() callbacks

The test/object test is passing again, instead of deadlocking.

src/hb-common.cc

@@ -162,7 +162,7 @@ struct hb_language_item_t {
   void finish (void) { free (lang); }
 };
 
-static hb_threadsafe_set_t<hb_language_item_t> langs;
+static hb_static_threadsafe_set_t<hb_language_item_t> langs;
 
 hb_language_t
 hb_language_from_string (const char *str)
@@ -293,8 +293,7 @@ hb_script_get_horizontal_direction (hb_script_t script)
  * should switch to using that insted for these too.
  */
 
-/* XXX  this can result in deadlocks because we call user callbacks */
+static hb_static_mutex_t user_data_lock;
-static hb_static_mutex_t user_data_mutex;
 
 bool
 hb_user_data_array_t::set (hb_user_data_key_t *key,
@@ -304,16 +303,12 @@ hb_user_data_array_t::set (hb_user_data_key_t *key,
   if (!key)
     return false;
 
-  hb_mutex_lock (&user_data_mutex);
-
   if (!data && !destroy) {
-    items.remove (key);
+    items.remove (key, user_data_lock);
     return true;
   }
   hb_user_data_item_t item = {key, data, destroy};
-  bool ret = !!items.insert (item);
+  bool ret = !!items.replace_or_insert (item, user_data_lock);
-
-  hb_mutex_unlock (&user_data_mutex);
 
   return ret;
 }
@@ -321,14 +316,15 @@ hb_user_data_array_t::set (hb_user_data_key_t *key,
 void *
 hb_user_data_array_t::get (hb_user_data_key_t *key)
 {
-  hb_mutex_lock (&user_data_mutex);
+  hb_user_data_item_t item = {NULL };
 
-  hb_user_data_item_t *item = items.find (key);
+  return items.find (key, &item, user_data_lock) ? item.data : NULL;
-  void *ret = item ? item->data : NULL;
+}
 
-  hb_mutex_unlock (&user_data_mutex);
+void
-
+hb_user_data_array_t::finish (void)
-  return ret;
+{
+  items.finish (user_data_lock);
 }
 
 

src/hb-mutex-private.hh

@@ -105,62 +105,49 @@ struct hb_static_mutex_t : hb_mutex_t
   hb_static_mutex_t (void) {
     hb_mutex_init (this);
   }
+
+  inline void lock (void) { hb_mutex_lock (this); }
+  inline void unlock (void) { hb_mutex_unlock (this); }
 };
 
 
 HB_END_DECLS
 
 
-/* XXX If the finish() callbacks of items in the set recursively try to
- * modify the set, deadlock occurs.  This needs fixing in set proper in
- * fact. */
-
 template <typename item_t>
-struct hb_threadsafe_set_t
+struct hb_static_threadsafe_set_t
 {
-  hb_set_t <item_t> set;
+  hb_lockable_set_t <item_t, hb_static_mutex_t> set;
-  hb_static_mutex_t mutex;
+  hb_static_mutex_t lock;
 
   template <typename T>
-  inline item_t *insert (T v)
+  inline item_t *replace_or_insert (T v)
   {
-    hb_mutex_lock (&mutex);
+    return set.replace_or_insert (v, lock);
-    item_t *item = set.insert (v);
-    hb_mutex_unlock (&mutex);
-    return item;
   }
 
   template <typename T>
   inline void remove (T v)
   {
-    hb_mutex_lock (&mutex);
+    set.remove (v, lock);
-    set.remove (v);
-    hb_mutex_unlock (&mutex);
   }
 
   template <typename T>
-  inline item_t *find (T v)
+  inline bool find (T v, item_t *i)
   {
-    hb_mutex_lock (&mutex);
+    return set.find (v, i, lock);
-    item_t *item = set.find (v);
-    hb_mutex_unlock (&mutex);
-    return item;
   }
 
   template <typename T>
-  inline item_t *find_or_insert (T v) {
+  inline item_t *find_or_insert (T v)
-    hb_mutex_lock (&mutex);
+  {
-    item_t *item = set.find_or_insert (v);
+    return set.find_or_insert (v, lock);
-    hb_mutex_unlock (&mutex);
-    return item;
   }
 
-  void finish (void) {
+  void finish (void)
-    hb_mutex_lock (&mutex);
+  {
-    set.finish ();
+    set.finish (lock);
-    hb_mutex_unlock (&mutex);
   }
-
 };
 
 

src/hb-object-private.hh

@@ -34,6 +34,8 @@
 
 #include "hb-private.hh"
 
+#include "hb-mutex-private.hh"
+
 HB_BEGIN_DECLS
 
 
@@ -117,7 +119,7 @@ struct hb_user_data_array_t {
     void finish (void) { if (destroy) destroy (data); }
   };
 
-  hb_set_t<hb_user_data_item_t> items;
+  hb_lockable_set_t<hb_user_data_item_t, hb_static_mutex_t> items;
 
   HB_INTERNAL bool set (hb_user_data_key_t *key,
 			void *              data,
@@ -125,7 +127,7 @@ struct hb_user_data_array_t {
 
   HB_INTERNAL void *get (hb_user_data_key_t *key);
 
-  void finish (void) { items.finish (); }
+  HB_INTERNAL void finish (void);
 };
 
 

src/hb-private.hh

@@ -319,56 +319,131 @@ template <typename Type>
 struct hb_array_t : hb_prealloced_array_t<Type, 2> {};
 
 
-template <typename item_t>
+template <typename item_t, typename lock_t>
-struct hb_set_t
+struct hb_lockable_set_t
 {
   hb_array_t <item_t> items;
 
   template <typename T>
-  inline item_t *insert (T v)
+  inline item_t *replace_or_insert (T v, lock_t &l)
   {
+    l.lock ();
     item_t *item = items.find (v);
-    if (item)
+    if (item) {
-      item->finish ();
+      item_t old = *item;
-    else
-      item = items.push ();
-    if (unlikely (!item)) return NULL;
       *item = v;
+      l.unlock ();
+      old.finish ();
+    } else {
+      item = items.push ();
+      if (likely (item))
+	*item = v;
+      l.unlock ();
+    }
     return item;
   }
 
   template <typename T>
-  inline void remove (T v)
+  inline void remove (T v, lock_t &l)
   {
+    l.lock ();
     item_t *item = items.find (v);
-    if (!item) return;
+    if (item) {
-
+      item_t old = *item;
-    item->finish ();
       *item = items[items.len - 1];
       items.pop ();
+      l.unlock ();
+      old.finish ();
+    } else {
+      l.unlock ();
+    }
   }
 
   template <typename T>
-  inline item_t *find (T v)
+  inline bool find (T v, item_t *i, lock_t &l)
   {
-    return items.find (v);
+    l.lock ();
+    item_t *item = items.find (v);
+    if (item)
+      *i = *item;
+    l.unlock ();
+    return !!item;
   }
 
   template <typename T>
-  inline item_t *find_or_insert (T v) {
+  inline item_t *find_or_insert (T v, lock_t &l)
-    item_t *item = find (v);
+  {
+    l.lock ();
+    item_t *item = items.find (v);
     if (!item) {
       item = items.push ();
       if (likely (item))
         *item = v;
     }
+    l.unlock ();
     return item;
   }
 
-  void finish (void) {
+  inline void finish (lock_t &l)
-    for (unsigned i = 0; i < items.len; i++)
+  {
-      items[i].finish ();
+    l.lock ();
+    while (items.len) {
+      item_t old = items[items.len - 1];
+	items.pop ();
+	l.unlock ();
+	old.finish ();
+	l.lock ();
+    }
     items.shrink (0);
+    l.unlock ();
+  }
+
+};
+
+template <typename item_t>
+struct hb_set_t
+{
+  struct lock_t {
+    int unused;
+
+    inline void lock (void) {}
+    inline void unlock (void) {}
+  };
+
+  hb_lockable_set_t <item_t, lock_t> set;
+
+  template <typename T>
+  inline item_t *replace_or_insert (T v)
+  {
+    lock_t lock;
+    return set.replace_or_insert (v, lock);
+  }
+
+  template <typename T>
+  inline void remove (T v)
+  {
+    lock_t lock;
+    set.remove (v, lock);
+  }
+
+  template <typename T>
+  inline bool find (T v, item_t *i)
+  {
+    lock_t lock;
+    return set.find (v, i, lock);
+  }
+
+  template <typename T>
+  inline item_t *find_or_insert (T v)
+  {
+    lock_t lock;
+    return set.find_or_insert (v, lock);
+  }
+
+  void finish (void)
+  {
+    lock_t lock;
+    set.finish (lock);
   }
 
 };