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. -- Tom. ------------------------------------------------------------------------------ _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
