From a5eb66edd4245a5198ca0881e3583e8cc415afa3 Mon Sep 17 00:00:00 2001
From: Tom Hacohen <tom@stosb.com>
Date: Tue, 12 Jul 2016 10:26:23 +0100
Subject: [PATCH] Eo refcount: Split the refcount to private and public (user).

This commit changes the way refcount is dealt with internally. Before
this commit, there was one refcount shared between Eo internals and
users. Now there is a refcount for eo operations (like for example,
function calls) and one for user refcount (eo_ref).

An example bug that this protects against (which is seemingly rather
common) is:
some_eo_func(obj);

// Inside the implementation of that func:
pd->a = 1; // The object's private data
eo_unref(obj); // To delete the object
eo_unref(obj); // A big one extra unref
pd->a = 2; // Segfault, this data has already been freed

This is a feature, but really just a fix for a class of bugs.

@feature
---
 src/lib/eo/eo.c                               | 48 ++++++++++++-------
 src/lib/eo/eo_base_class.c                    |  2 +-
 src/lib/eo/eo_private.h                       |  6 ++-
 .../eo/suite/eo_test_class_behaviour_errors.c |  4 +-
 4 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/src/lib/eo/eo.c b/src/lib/eo/eo.c
index f147f0d2c7..50b8e6673f 100644
--- a/src/lib/eo/eo.c
+++ b/src/lib/eo/eo.c
@@ -697,19 +697,11 @@ _eo_add_internal_start(const char *file, int line, const Eo_Class *klass_id, Eo
 
    _eo_condtor_reset(obj);
 
-   _eo_ref(obj);
+   eo_ref(eo_id);
 
+   /* Reference for the parent if is_ref is done in _eo_add_end */
    eo_parent_set(eo_id, parent_id);
 
-   /* If there's a parent. Ref. Eo_add should return an object with either a
-    * parent ref, or with the lack of, just a ref. */
-     {
-        if (ref && eo_parent_get(eo_id))
-          {
-             _eo_ref(obj);
-          }
-     }
-
    /* eo_id can change here. Freeing is done on the resolved object. */
    eo_id = eo_constructor(eo_id);
    if (!eo_id)
@@ -724,10 +716,16 @@ _eo_add_internal_start(const char *file, int line, const Eo_Class *klass_id, Eo
      }
    else if (eo_id != _eo_obj_id_get(obj))
      {
+        EO_OBJ_POINTER_RETURN_VAL(eo_id, new_obj, NULL);
         /* We have two refs at this point. */
         _eo_unref(obj);
-        _eo_unref(obj);
+        eo_del((Eo *) obj->header.id);
 
+        _eo_ref(new_obj);
+     }
+
+   if (ref && eo_parent_get(eo_id))
+     {
         eo_ref(eo_id);
      }
 
@@ -1374,11 +1372,11 @@ eo_isa(const Eo *eo_id, const Eo_Class *klass_id)
 EAPI Eo *
 eo_xref_internal(const char *file, int line, Eo *obj_id, const Eo *ref_obj_id)
 {
-   EO_OBJ_POINTER_RETURN_VAL(obj_id, obj, obj_id);
-
-   _eo_ref(obj);
+   eo_ref(obj_id);
 
 #ifdef EO_DEBUG
+   EO_OBJ_POINTER_RETURN_VAL(obj_id, obj, obj_id);
+
    Eo_Xref_Node *xref = calloc(1, sizeof(*xref));
    xref->ref_obj = ref_obj_id;
    xref->file = file;
@@ -1419,7 +1417,7 @@ eo_xunref(Eo *obj_id, const Eo *ref_obj_id)
 #else
    (void) ref_obj_id;
 #endif
-   _eo_unref(obj);
+   eo_unref(obj_id);
 }
 
 EAPI Eo *
@@ -1427,7 +1425,11 @@ eo_ref(const Eo *obj_id)
 {
    EO_OBJ_POINTER_RETURN_VAL(obj_id, obj, (Eo *)obj_id);
 
-   _eo_ref(obj);
+   ++(obj->user_refcount);
+   if (EINA_UNLIKELY(obj->user_refcount == 1))
+     {
+        _eo_ref(obj);
+     }
    return (Eo *)obj_id;
 }
 
@@ -1436,7 +1438,17 @@ eo_unref(const Eo *obj_id)
 {
    EO_OBJ_POINTER_RETURN(obj_id, obj);
 
-   _eo_unref(obj);
+   --(obj->user_refcount);
+   if (EINA_UNLIKELY(obj->user_refcount <= 0))
+     {
+        if (obj->user_refcount < 0)
+          {
+             ERR("Obj:%p. User refcount (%d) < 0. Too many unrefs.", obj, obj->user_refcount);
+             return;
+          }
+
+        _eo_unref(obj);
+     }
 }
 
 EAPI int
@@ -1444,7 +1456,7 @@ eo_ref_get(const Eo *obj_id)
 {
    EO_OBJ_POINTER_RETURN_VAL(obj_id, obj, 0);
 
-   return obj->refcount;
+   return obj->user_refcount;
 }
 
 EAPI void
diff --git a/src/lib/eo/eo_base_class.c b/src/lib/eo/eo_base_class.c
index 2e2fe3fb2e..92e10af655 100644
--- a/src/lib/eo/eo_base_class.c
+++ b/src/lib/eo/eo_base_class.c
@@ -552,7 +552,7 @@ _eo_base_parent_set(Eo *obj, Eo_Base_Data *pd, Eo *parent_id)
          * the process of deleting the object.*/
         if (!parent_id && !eo_obj->del_triggered)
           {
-             _eo_unref(eo_obj);
+             eo_unref(obj);
           }
      }
 
diff --git a/src/lib/eo/eo_private.h b/src/lib/eo/eo_private.h
index 9958c8210f..d47fcca93a 100644
--- a/src/lib/eo/eo_private.h
+++ b/src/lib/eo/eo_private.h
@@ -110,6 +110,7 @@ struct _Eo_Object
      Eo_Del_Intercept del_intercept;
 
      short refcount;
+     short user_refcount;
 #ifdef EO_DEBUG
      short datarefcount;
 #endif
@@ -322,8 +323,9 @@ _eo_unref(_Eo_Object *obj)
 
         if (obj->del_intercept)
           {
-             (obj->refcount)++;
-             obj->del_intercept(_eo_obj_id_get(obj));
+             Eo *obj_id = _eo_obj_id_get(obj);
+             eo_ref(obj_id);
+             obj->del_intercept(obj_id);
              return;
           }
 
diff --git a/src/tests/eo/suite/eo_test_class_behaviour_errors.c b/src/tests/eo/suite/eo_test_class_behaviour_errors.c
index 12c478f454..3b25b389c0 100644
--- a/src/tests/eo/suite/eo_test_class_behaviour_errors.c
+++ b/src/tests/eo/suite/eo_test_class_behaviour_errors.c
@@ -48,7 +48,7 @@ START_TEST(eo_destructor_unref)
    Eo *obj = eo_add(klass, NULL);
    fail_if(!obj);
 
-   TEST_EO_ERROR("_eo_unref", "Object %p deletion already triggered. You wrongly call eo_unref() within a destructor.");
+   TEST_EO_ERROR("eo_unref", "Obj:%p. User refcount (%d) < 0. Too many unrefs.");
    eo_unref(obj);
 
    eina_log_print_cb_set(eina_log_print_cb_stderr, NULL);
@@ -80,7 +80,7 @@ START_TEST(eo_destructor_double_del)
    eo_manual_free_set(obj, EINA_TRUE);
    fail_if(!obj);
 
-   TEST_EO_ERROR("_eo_unref", "Object %p already destructed.");
+   TEST_EO_ERROR("eo_unref", "Obj:%p. User refcount (%d) < 0. Too many unrefs.");
    eo_del(obj);
    eo_del(obj);