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