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. 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? -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
