I think the whole problem comes from the lack of concepts.
When we use referencing counting, it is meant for sharing
ownership. So, we shouldn't have to unparent before
eo_unref. However, parents own a reference to the object,
or else the object might disappear unexpectedly.
However, eo_add is not consistent as raster has
pointed out.
eo_add(CLASS, NULL); returns reference 1
eo_add(CLASS, parent); also returns reference 1
And that is a problem.
In the first case, the code that calls eo_add owns the
reference, and in the second case it doesn't. It makes
no sense.
Also, we have a huge problem with elm_win which
reparents itself but hacks its referencing counting to
still be 1 so eo_del works.
On elm_win.c:3636 we have:
/* XXX: This hack is needed because we parent ourselves to an inside object.
* That should be fixed, and then this can be fixed.
* Only needed if there wasn't a parent, because if there was, we are just
* replacing it. */
if (!parent)
{
eo_unref(obj);
}
Why is elm_win reparenting itself in the first place? Code
that instantiates elm_win asssumes to be the only one
owning a reference to it and it can't eo_unref it because
there's still a parent that conceptually owns the only
reference to it. That's a huge problem which makes
eo_unref'ing it incorrect, though it should be correct for
all classes.
What is the rationale for this?
So, taking this problem away for elm_win and making
eo_add and eo_add_ref consistent, I agree with Tom
and Daniel. Since the parent owns the reference to
the child, the child should not be getting eo_unref'ed to
0. Some code is unref'ing without ref'ing it first. If
he wants to free that child, he should be unpareting
(only) or unref'ing the parent. However, objects should
not be getting parents implicitly or else that puts off
code that deals with reference ownership.
Regards,
On Wed, Aug 26, 2015 at 6:17 AM, Tom Hacohen <t...@osg.samsung.com> wrote:
> On 21/08/15 11:21, Tom Hacohen wrote:
>> On 21/08/15 07:07, Jean-Philippe André wrote:
>>> On Thu, Aug 20, 2015 at 6:48 PM, Carsten Haitzler <ras...@rasterman.com>
>>> wrote:
>>>
>>>> On Thu, 20 Aug 2015 10:12:30 +0200 Cedric BAIL <cedric.b...@free.fr> said:
>>>>
>>>>> On Thu, Aug 20, 2015 at 9:22 AM, Daniel Zaoui <daniel.za...@samsung.com>
>>>>> wrote:
>>>>>> On Thu, 20 Aug 2015 15:48:23 +0900
>>>>>> Carsten Haitzler <ras...@rasterman.com> (The Rasterman) wrote:
>>>>>>
>>>>>>> On Thu, 20 Aug 2015 09:09:44 +0300 Daniel Zaoui
>>>>>>> <daniel.za...@samsung.com> said:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Wed, 19 Aug 2015 20:53:53 -0700
>>>>>>>> Carsten Haitzler <ras...@rasterman.com> wrote:
>>>>>>>>
>>>>>>>>> raster pushed a commit to branch master.
>>>>>>>>>
>>>>>>>>>
>>>> http://git.enlightenment.org/core/efl.git/commit/?id=8689d54471aafdd7a5b5a27ce116bf2ab68c1042
>>>>>>>>>
>>>>>>>>> commit 8689d54471aafdd7a5b5a27ce116bf2ab68c1042
>>>>>>>>> Author: Carsten Haitzler (Rasterman) <ras...@rasterman.com>
>>>>>>>>> Date: Thu Aug 20 12:50:52 2015 +0900
>>>>>>>>>
>>>>>>>>> eo - destruction - ensure child is removed from parent child
>>>>>>>>> list
>>>>>>>>> this follows on from cbc1a217bfc8b5c6dd94f0448f19245c43eb05e0
>>>>>>>>> as this code was correct, but was then causing bugs due to
>>>>>>>>> children staying in their parent lists. this should never have
>>>>>>>>> happened and this is really bad. this fixes this and ensures
>>>>>>>>> children on destruction are gone from their parent lists.
>>>>>>>>>
>>>>>>>>> @fix
>>>>>>>>> ---
>>>>>>>>> src/lib/eo/eo_base_class.c | 14 ++++++++++----
>>>>>>>>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/src/lib/eo/eo_base_class.c
>>>>>>>>> b/src/lib/eo/eo_base_class.c index fe52203..9f8252b 100644
>>>>>>>>> --- a/src/lib/eo/eo_base_class.c
>>>>>>>>> +++ b/src/lib/eo/eo_base_class.c
>>>>>>>>> @@ -977,7 +977,6 @@ EOLIAN static void
>>>>>>>>> _eo_base_destructor(Eo *obj, Eo_Base_Data *pd)
>>>>>>>>> {
>>>>>>>>> Eo *child;
>>>>>>>>> - Eo_Base_Data *child_pd;
>>>>>>>>>
>>>>>>>>> DBG("%p - %s.", obj, eo_class_name_get(MY_CLASS));
>>>>>>>>>
>>>>>>>>> @@ -987,11 +986,18 @@ _eo_base_destructor(Eo *obj, Eo_Base_Data
>>>>>>>>> *pd) while (pd->children)
>>>>>>>>> {
>>>>>>>>> child = eina_list_data_get(pd->children);
>>>>>>>>> - child_pd = eo_data_scope_get(child, EO_BASE_CLASS);
>>>>>>>>> - pd->children = eina_list_remove_list(pd->children,
>>>>>>>>> pd->children);
>>>>>>>>> - child_pd->parent_list = NULL;
>>>>>>>>> eo_do(child, eo_parent_set(NULL));
>>>>>>>>> }
>>>>>>>>> + // remove child from its parent on destruction - ha to be done
>>>>>>>>> + if (pd->parent)
>>>>>>>>> + {
>>>>>>>>> + Eo_Base_Data *parent_pd;
>>>>>>>>> +
>>>>>>>>> + parent_pd = eo_data_scope_get(pd->parent, EO_BASE_CLASS);
>>>>>>>>> + parent_pd->children =
>>>>>>>>> eina_list_remove_list(parent_pd->children,
>>>>>>>>> +
>>>>>>>>> pd->parent_list);
>>>>>>>>> + pd->parent_list = NULL;
>>>>>>>>> + }
>>>>>>>>>
>>>>>>>>> _eo_generic_data_del_all(pd);
>>>>>>>>> _wref_destruct(pd);
>>>>>>>>>
>>>>>>>>
>>>>>>>> The parent should never be !NULL when reaching the destructor. Imo,
>>>>>>>> this code has not to be here. Instead, an error message should be
>>>>>>>> displayed in the case the parent is still connected to the object.
>>>>>>>> There is a bug but definitely the solution doesn't have to be here.
>>>>>>>> I think this issue may happen if eo_del is never called and
>>>>>>>> eo_unref is called instead. We need to check inside _eo_unref that
>>>>>>>> the parent is NULL and display an error message.
>>>>>>>>
>>>>>>>> Tom, any thoughts?
>>>>>>>
>>>>>>> eo_suite simple examples show this case. in fact i think the test
>>>>>>> case is broken too... but as such i followed the code, and read it.
>>>>>>> if you destruct a child, the parent still has a list entry pointing
>>>>>>> to it.
>>>>>>>
>>>>>>> _eo_unref() gets to 0, and _eo_del_internal() calls the destructor...
>>>>>>
>>>>>> eo_del unparents it and that's the issue of the test, as it is not
>>>> called
>>>>>> before destruction. I think the test has not been adapted when
>>>> references
>>>>>> mechanism has changed. Anyway, eo_unref should display an error message
>>>>>> indicating that only one ref exists and belongs to the parent. I think
>>>> it
>>>>>> should force unparenting too.
>>>>>
>>>>> Maybe their is an issue in the test, but I have seen other issue where
>>>>> when you do del with ref > 1 and then unref it doesn't remove the
>>>>> parent. My believe is that the issue is somewhere in unref.
>>>>
>>>> thats was the problem here. it just happened to be just an unref by itself
>>>> instead of a del.
>>>>
>>>>>>> this does nothing in the way of removing from a parent (read it). the
>>>>>>> base class destructor is the only place to do this... and so i added
>>>>>>> it.
>>>>>>>
>>>>>>> look at
>>>>>>>
>>>>>>> START_TEST(eo_refs)
>>>>>>>
>>>>>>> here:
>>>>>>>
>>>>>>> obj = eo_add(SIMPLE_CLASS, NULL);
>>>>>>> obj2 = eo_add(SIMPLE_CLASS, obj);
>>>>>>> eo_unref(obj2);
>>>>>>> eo_ref(obj2);
>>>>>>> eo_del(obj2);
>>>>>>> eo_unref(obj);
>>>>>>
>>>>>> I don't see how this test is supposed to work. eo_ref and eo_del
>>>> should not
>>>>>> work well as the object is already deleted. As I said, it doesn't seem
>>>>>> updated with last Eo changes.
>>>>>>
>>>>>>>
>>>>>>> we create obj, then obj2 as a child of obj, then unref obj2. after
>>>>>>> this unref of obj2, obj2 was still in the child list of obj. this
>>>>>>> isn't a complex case. its an insanely simple one. our own test cases
>>>>>>> never caught this issue until i "fixed" the code above that pointed
>>>>>>> this out. the base class destructor never removes an object from its
>>>>>>> parent list. no code in the destructor to do that at all. i fixed
>>>>>>> that :)
>>>>>>
>>>>>> The code is not supposed to be in the destructor as everything should
>>>> have
>>>>>> been done before to not have a parent at this time. That's why I say
>>>> the
>>>>>> fix should not be there imo but more in _eo_unref.
>>>>>
>>>>> The code here create a problem for me. It doesn't call eo_parent_set
>>>>> -> doesn't trigger the virtual function -> more impossible to fix leak
>>>>> somewhere else. If we can't make parent_set work in a reliable way as
>>>>> an eo function, then it is meaningless for it to be an eo function.
>>>>
>>>> current code shouldnt leak. it'll be deleted/unreffed somehow. either on
>>>> destruction of parent or when child is unreffed/deleted in the base class
>>>> destructor.
>>>>
>>>> we do have an issue still - up for discussion here.
>>>>
>>>> obj = eo_add(SIMPLE_CLASS, NULL);
>>>> obj2 = eo_add(SIMPLE_CLASS, obj);
>>>>
>>>> has obj2 at refcount of 1. as i would expect. BUT
>>>>
>>>> obj = eo_add(SIMPLE_CLASS, NULL);
>>>> obj2 = eo_add(SIMPLE_CLASS, NULL);
>>>> eo_do(obj2, eo_parent_set(obj);
>>>>
>>>> has obj2 having a refcount of 2. imho this is totally inconsistent. it's
>>>> surprising to the developer. unexpected. and that is bad.
>>>>
>>>
>>> I expect both snippets to behave the same way.
>>>
>>>
>>>>
>>>> if you ask me the first case is better. if requires 1 less unref (and if
>>>> we are
>>>> consistent then every time we create a child we have to remember to unref
>>>> every
>>>> child all the time to ensure the parent can delete it). so from programming
>>>> convenience it makes code longer and far more likely to get leaks when you
>>>> forget to unref an obj you give to a parent.
>>>>
>>>> the other reason we should do case 1 in both situations in terms of
>>>> refcount is
>>>> that conceptually, when you set parent, you are handing YOUR ref TO the
>>>> parent.
>>>> logically you are transferring your ref. if its creation (constructors
>>>> etc.) or
>>>> if its inside some callbacks that is moving an obj from obj a to b the ref
>>>> is
>>>> being transferred.
>>>>
>>>> so i vote this get fixed. (dis)agreements?
>>>>
>>>>
>>> obj = eo_add(SIMPLE_CLASS, NULL);
>>> obj2 = eo_add(SIMPLE_CLASS, obj);
>>>
>>> OR
>>>
>>> obj = eo_add(SIMPLE_CLASS, NULL);
>>> obj2 = eo_add(SIMPLE_CLASS, NULL);
>>> eo_do(obj2, eo_parent_set(obj);
>>>
>>> then, eo_unref/eo_del(obj2) removes from parent and deletes the object (ie
>>> refcnt was 1, down to 0)
>>>
>>> So, yeah, I agree with raster here.
>>>
>>>
>>
>> I already said it on IRC. I too agree.
>>
>
> This is now in. We still need to the discuss the other part of this
> thread about unreffing when there's still a parent.
>
> --
> Tom.
>
>
> ------------------------------------------------------------------------------
> _______________________________________________
> enlightenment-devel mailing list
> enlightenment-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
--
Felipe Magno de Almeida
------------------------------------------------------------------------------
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
