On Thu, Aug 31, 2017 at 06:44:52PM +0900, Carsten Haitzler wrote:
> On Thu, 31 Aug 2017 09:31:19 +0200 [email protected] said:
>
> > Good morning,
> >
> > On Thu, Aug 31, 2017 at 12:17:16AM -0700, Carsten Haitzler wrote:
> > > raster pushed a commit to branch master.
> > >
> > > http://git.enlightenment.org/core/efl.git/commit/?id=65d2dfc8925a183bf7f2709677d87edcd759e9c3
> > >
> > > commit 65d2dfc8925a183bf7f2709677d87edcd759e9c3
> > > Author: Carsten Haitzler (Rasterman) <[email protected]>
> > > Date: Thu Aug 31 16:12:57 2017 +0900
> > >
> > > elm focus manager - dont leak child lists when updating focus order
> > >
> > > this should fix T5800
> > >
> > > @fix
> > > ---
> > > src/lib/elementary/elm_box.c | 1 +
> > > src/lib/elementary/elm_grid.c | 1 +
> > > src/lib/elementary/elm_table.c | 1 +
> > > 3 files changed, 3 insertions(+)
> > >
> > > diff --git a/src/lib/elementary/elm_box.c b/src/lib/elementary/elm_box.c
> > > index b6a617dd2b..1c77ccd882 100644
> > > --- a/src/lib/elementary/elm_box.c
> > > +++ b/src/lib/elementary/elm_box.c
> > > @@ -29,6 +29,7 @@ _focus_order_flush(Eo *obj, Elm_Box_Data *pd
> > > EINA_UNUSED)
> > > Eina_List *order = evas_object_box_children_get(wpd->resize_obj);
> > >
> > > efl_ui_focus_manager_calc_update_order(wpd->focus.manager, obj,
> > > order);
> > > + eina_list_free(order);
> >
> > Noep, you are passing the ownership of the list to the focus manager,
> > you should NOT use the list after that anymore, esp. not freeing it,
> > same for the other usages ... :)
>
> yeah... i figured that out right after. it fixed the leak... then led to more
> badness randomly in other cases. the issue is that failed method calls dont
> free allocated params (also it was missing an own() in the eo file too and
> other things).
>
> basically this will leak until eo can clean/free up the list passed in on a
> failed call (and eo file is fixed to say it owns and the code matches this too
> and frees the list inside the call all the time).
>
> so for now it duplicates until eo can/does do this.
I know your manner of writing emails, so i thought twice about replying
to this.
First of all lets break down what issues we had:
- A missing own() in the .eo file, yep my misstake simple commit, simple fix
- A missing free for the case a node was not known to the manager, which
is basically the insertion of { eina_list_free(order); return; } which
is not alot of things
- eo is not freeing parameters that where defined with own(...)
Number 1 & 2 are my misstakes and can be fixed very simple.
Number 3 was caused by the sample code from T5800, which just creates
an evas (NOT and efl.ui.win) and then creates a elm.widget. On the one
hand if you just spend that second looking into your terminal you would
have seen that there is a error messages indicating that this box there
should REALLY not be there... or there should be a efl.ui.win. So we have
the first fuck up here that does not really need fixing in
efl.ui.focus.manager, but rather in your calling code.
Now to your commit that "fixed" it. You have dropped out a optimization,
which was done to keep list duplication and clonging number low, without
even asking or trying to understand why. This was a lot of perf and
massif work to get those things down and small while working correctly.
Then you now went for the safe way and added if (manager) in every
single place where i called the order_update api. So #3 doesnt even
strike anymore, in the end those ifs where enough, without dropping out
the performance optimization.
All in all this feels just like another "I need to fix this now without
asking why things where done that way". Could we somehow establish to at
least try to ask the person that wrote that code, and somehow coordinate
a fix that doesnt just hammer things in with a "deal with it" manner?
Also on a slightly other note, just because code leaks memory or crashes
doesnt always indicate that we have a error on the called code. Thse
usecase where this elm.box was used is completly wrong and even tells
you that in a error messages.
slightly salty greetings,
Marcel Hollerbach
>
> > Greetings
> > Marcel Hollerbach
> >
> > > }
> > >
> > > static void *
> > > diff --git a/src/lib/elementary/elm_grid.c b/src/lib/elementary/elm_grid.c
> > > index 7c85648882..913ffe3b2f 100644
> > > --- a/src/lib/elementary/elm_grid.c
> > > +++ b/src/lib/elementary/elm_grid.c
> > > @@ -20,6 +20,7 @@ _focus_order_flush(Eo *obj)
> > > Eina_List *order = evas_object_grid_children_get(wpd->resize_obj);
> > >
> > > efl_ui_focus_manager_calc_update_order(wpd->focus.manager, obj,
> > > order);
> > > + eina_list_free(order);
> > > }
> > >
> > >
> > > diff --git a/src/lib/elementary/elm_table.c
> > > b/src/lib/elementary/elm_table.c
> > > index 48e6dfa62a..48bc137b23 100644
> > > --- a/src/lib/elementary/elm_table.c
> > > +++ b/src/lib/elementary/elm_table.c
> > > @@ -21,6 +21,7 @@ _focus_order_flush(Eo *obj)
> > > Eina_List *order = evas_object_table_children_get(wpd->resize_obj);
> > >
> > > efl_ui_focus_manager_calc_update_order(wpd->focus.manager, obj,
> > > order);
> > > + eina_list_free(order);
> > > }
> > >
> > > EOLIAN static Eina_Bool
> > >
> > > --
> > >
> > >
> >
> > ------------------------------------------------------------------------------
> > Check out the vibrant tech community on one of the world's most
> > engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> > _______________________________________________
> > enlightenment-devel mailing list
> > [email protected]
> > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
> >
>
>
> --
> ------------- Codito, ergo sum - "I code, therefore I am" --------------
> The Rasterman (Carsten Haitzler) [email protected]
>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
enlightenment-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
