On Fri, 6 May 2016 11:03:23 -0700 Cedric BAIL <cedric.b...@free.fr> said:
> On Thu, May 5, 2016 at 11:58 PM, Carsten Haitzler <ras...@rasterman.com> > wrote: > > On Thu, 05 May 2016 21:55:24 -0700 Cedric BAIL <cedric.b...@free.fr> said: > > > >> cedric pushed a commit to branch master. > > > > ecore_timer is full of double frees. i literally get a segv every few > > seconds if i open/close windows. > > With my patch from just after the revert ? I have not been able to > reproduce any at all. oh i didn't see that. i just saw a revert... i spent a while staring at a lot of segv's... :) > > _ecore_call_task_cb() sometimes has ecore_timer_del() called inside > > (sometimes not), so SOMETIMES ecore_timer_del has freed the legacy data, > > sometimes it has not. thus free(legacy) is going to break. OFTEN. > > Yes, legacy lifecycle is a tricky mess. You can legally ask for a > double free and the legacy code was handling that fine. I missed that > with my patch and corrected in the patch following this revert. Your > code was not fixing the main issue as it was still calling eo_del one > to many. As long as we only had one callback registered on an object, > it will have been fine, but if one more, crash will be back and inside > eo callback call logic (As at that point, it is expected to have the > object refed and protected from death and do not rely on eo to get the > private data during each iteration of the loop). > > > all you did was bring back segv's. > > Gni ? Did you test with the fix I pushed along with this commit which > is a proper way to avoid this double eo_del and double free (I don't > even know how you could update git and not get both patch as I pushed > them at the same time). I have not seen any issue with that patch nor > is valgrind reporting anything in the case you are discribing. > > Cedric > > >> http://git.enlightenment.org/core/efl.git/commit/?id=bfc19893d73d5ca8baf2e9df35183b6852d9e9e0 > >> > >> commit bfc19893d73d5ca8baf2e9df35183b6852d9e9e0 > >> Author: Cedric Bail <ced...@osg.samsung.com> > >> Date: Thu May 5 21:32:37 2016 -0700 > >> > >> Revert "ecore timer - fuix up segv storm that has crept in with frees" > >> > >> This reverts commit a13570c17c97bb9407c24bcf78ab28eaa4541ad9. > >> > >> This doesn't really fix the problem which is hidden by eo capability to > >> not crash on bad unref. With legacy API you are allowed to do a > >> ecore_timer_del and also return EINA_FALSE. In that case you have a double > >> eo_del (which is luckily protected) and a double free (that is not). It > >> does crash on the double free, but the issue is a lifecycle issue. Will > >> bring a better patch for this. > >> --- > >> src/lib/ecore/ecore_timer.c | 14 +++----------- > >> 1 file changed, 3 insertions(+), 11 deletions(-) > >> > >> diff --git a/src/lib/ecore/ecore_timer.c b/src/lib/ecore/ecore_timer.c > >> index 1b69676..54cf773 100644 > >> --- a/src/lib/ecore/ecore_timer.c > >> +++ b/src/lib/ecore/ecore_timer.c > >> @@ -157,14 +157,8 @@ _ecore_timer_legacy_tick(void *data, const Eo_Event > >> *event) > >> if (!_ecore_call_task_cb(legacy->func, (void*)legacy->data)) > >> { > >> - if (eo_key_data_get(event->obj, "_legacy")) > >> - { > >> - eo_key_del(event->obj, "_legacy"); > >> - eo_event_callback_del(event->obj, EFL_TIMER_EVENT_TICK, > >> - _ecore_timer_legacy_tick, legacy), > >> - free(legacy); > >> - eo_del(event->obj); > >> - } > >> + eo_del(event->obj); > >> + free(legacy); > >> } > >> > >> return EO_CALLBACK_CONTINUE; > >> @@ -225,10 +219,8 @@ ecore_timer_del(Ecore_Timer *timer) > >> legacy = eo_key_data_get(timer, "_legacy"); > >> data = (void*) legacy->data; > >> > >> - eo_key_del(timer, "_legacy"); > >> - eo_event_callback_del(timer, EFL_TIMER_EVENT_TICK, > >> - _ecore_timer_legacy_tick, legacy), > >> free(legacy); > >> + eo_key_del(timer, "_legacy"); > >> eo_del(timer); > >> > >> return data; > >> > >> -- > >> > >> > > > > > > -- > > ------------- Codito, ergo sum - "I code, therefore I am" -------------- > > The Rasterman (Carsten Haitzler) ras...@rasterman.com > > > > > > > > -- > Cedric BAIL > > ------------------------------------------------------------------------------ > Find and fix application performance issues faster with Applications Manager > Applications Manager provides deep performance insights into multiple tiers of > your business applications. It resolves application problems quickly and > reduces your MTTR. Get your free trial! > https://ad.doubleclick.net/ddm/clk/302982198;130105516;z > _______________________________________________ > enlightenment-devel mailing list > enlightenment-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/enlightenment-devel > -- ------------- Codito, ergo sum - "I code, therefore I am" -------------- The Rasterman (Carsten Haitzler) ras...@rasterman.com ------------------------------------------------------------------------------ Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
