Hi,
Thanks for taking the time to look into this issue. There's been an amount
of discussion and back-and-forth patching related to it for some time now,
and it's great to see someone taking some initiative here.
Looking over this patch, I'm unsure whether the method used for resolving
the issue is indeed the optimal one. Given the already-existing discussion
going on related to this topic, I think this is a patch which would have
benefited from review prior to being landed. In particular, I'd have
suggested that unit tests be added for this case--we have somewhat
extensive tests for promise usage already, so having a good test case like
this would improve the reliability of future changes to corresponding
codepaths--which should not be too difficult given the time that you've
already invested in debugging the issue to fully understand it.
I'm constructing this mail from scratch since it looks like you pushed this
from a branch without resetting the commit date and prevented a mail from
being sent to the list, so please excuse any strange formatting in the
patch diff below.
Regards,
Mike
> commit de2ec0559b01dba7919503955cc47c1c5fcd0f97
> Author: Carsten Haitzler (Rasterman) <ras...@rasterman.com>
> Date: Tue Dec 25 13:00:38 2018 +0000
>
> fix crashes created by "make efl_loop_promise_new a function"
>
> commit 9b5155c9f135f9ef450a817979f5884352b2d4c0 brought about crashes
> - specifically that i saw in terminology because it actually uses
> eina_promise_data_set() and the new efl_loop_promise_new basically
> took over ownership of that data, but if anyone used
> eina_promise_data_set() the data ptr used by this new code would bwe
> overwritten, causing segfauls when terminology loses selection
> ownership. for days i had mysterious crashes of terminology until i
> narrowed it down to the above, so if you have too, then this will fix
> it.
>
> what this does is create a data set intercept function callback that
> for now is only for use inside efl to everride data sets so they set
> data inside the new struct that tracks data. i also had to add and
> intercept for eina_promise_data_free_cb_set() as this in theory could
> also ber a similar problem.
>
> so perhaps the idea/design of efl_loop_promise_new() is not right and
> this kind of thgn has to be internal to eina promise... this means
> eina promise and loops are much more tied together.
>
> diff --git a/src/lib/ecore/efl_loop_consumer.c
b/src/lib/ecore/efl_loop_consumer.c
> index 29e94ed52d..1e49bc32aa 100644
> --- a/src/lib/ecore/efl_loop_consumer.c
> +++ b/src/lib/ecore/efl_loop_consumer.c
> @@ -50,4 +50,68 @@ _efl_loop_consumer_future_rejected(Eo *obj,
Efl_Loop_Consumer_Data *pd EINA_UNUS
> return eina_future_rejected(efl_loop_future_scheduler_get(obj),
error);
> }
>
> +typedef struct _Efl_Loop_Consumer_Promise Efl_Loop_Consumer_Promise;
> +struct _Efl_Loop_Consumer_Promise
> +{
> + EflLoopConsumerPromiseCancel func;
> + Eina_Free_Cb free;
> + void *data;
> + Eo *obj;
> +};
> +
> +static void
> +_cancel_free(void *data)
> +{
> + Efl_Loop_Consumer_Promise *lcp = data;
> +
> + if (lcp->free) lcp->free(lcp->data);
> + efl_unref(lcp->obj);
> + free(lcp);
> +}
> +
> +static void
> +_cancel_triggered(void *data, const Eina_Promise *p)
> +{
> + Efl_Loop_Consumer_Promise *lcp = data;
> +
> + if (lcp->func) lcp->func(lcp->data, lcp->obj, p);
> +}
> +
> +static void
> +_data_set(Eina_Promise *p, void *data)
> +{
> + Efl_Loop_Consumer_Promise *lcp = eina_promise_data_get(p);
> + lcp->data = data;
> +}
> +
> +static void
> +_cancel_free_cb_set(Eina_Promise *p, Eina_Free_Cb free_cb)
> +{
> + Efl_Loop_Consumer_Promise *lcp = eina_promise_data_get(p);
> + lcp->free = free_cb;
> +}
> +
> +static Eina_Promise *
> +_efl_loop_consumer_promise_new(Eo *obj, Efl_Loop_Consumer_Data *pd,
> + void *cancel_data,
EflLoopConsumerPromiseCancel cancel, Eina_Free_Cb cancel_free_cb)
> +{
> + Efl_Loop_Consumer_Promise *lcp;
> + Eina_Promise *p;
> +
> +
> + lcp = calloc(1, sizeof (Efl_Loop_Consumer_Promise));
> + if (!lcp) return NULL;
> +
> + lcp->func = cancel;
> + lcp->data = cancel_data;
> + lcp->free = cancel_free_cb;
> + lcp->obj = efl_ref(obj);
> +
> + p = eina_promise_new(efl_loop_future_scheduler_get(obj),
_cancel_triggered, lcp);
> + eina_promise_data_set_cb_set(p, _data_set);
> + eina_promise_data_free_cb_set_cb_set(p, _cancel_free_cb_set);
> + eina_promise_data_free_cb_set(p, _cancel_free);
> + return p;
> +}
> +
> #include "efl_loop_consumer.eo.c"
> diff --git a/src/lib/eina/eina_promise.c b/src/lib/eina/eina_promise.c
> index c239ba6261..aa5ced8ab2 100644
> --- a/src/lib/eina/eina_promise.c
> +++ b/src/lib/eina/eina_promise.c
> @@ -103,6 +103,8 @@ struct _Eina_Promise {
> Eina_Future *future;
> Eina_Future_Scheduler *scheduler;
> Eina_Promise_Cancel_Cb cancel;
> + Eina_Promise_Data_Set_Cb data_set_cb;
> + Eina_Promise_Data_Free_Cb_Set_Cb data_free_cb_set_cb;
> Eina_Free_Cb free_cb;
> const void *data;
> };
> @@ -1116,7 +1118,14 @@ eina_promise_data_set(Eina_Promise *p,
> void *data)
> {
> EINA_SAFETY_ON_NULL_RETURN(p);
> - p->data = data;
> + if (p->data_set_cb)
> + {
> + Eina_Promise_Data_Set_Cb cb = p->data_set_cb;
> + p->data_set_cb = NULL;
> + cb(p, data);
> + p->data_set_cb = cb;
> + }
> + else p->data = data;
> }
>
> EAPI void
> @@ -1124,7 +1133,30 @@ eina_promise_data_free_cb_set(Eina_Promise *p,
> Eina_Free_Cb free_cb)
> {
> EINA_SAFETY_ON_NULL_RETURN(p);
> - p->free_cb = free_cb;
> + if (p->data_free_cb_set_cb)
> + {
> + Eina_Promise_Data_Free_Cb_Set_Cb cb = p->data_free_cb_set_cb;
> + p->data_free_cb_set_cb = NULL;
> + cb(p, free_cb);
> + p->data_free_cb_set_cb = cb;
> + }
> + else p->free_cb = free_cb;
> +}
> +
> +EAPI void
> +eina_promise_data_set_cb_set(Eina_Promise *p,
> + Eina_Promise_Data_Set_Cb data_set_cb)
> +{
> + EINA_SAFETY_ON_NULL_RETURN(p);
> + p->data_set_cb = data_set_cb;
> +}
> +
> +EAPI void
> +eina_promise_data_free_cb_set_cb_set(Eina_Promise *p,
> + Eina_Promise_Data_Free_Cb_Set_Cb
data_free_cb_set_cb)
> +{
> + EINA_SAFETY_ON_NULL_RETURN(p);
> + p->data_free_cb_set_cb = data_free_cb_set_cb;
> }
>
> static Eina_Value
> diff --git a/src/lib/eina/eina_promise.h b/src/lib/eina/eina_promise.h
> index 1896260dba..060d8dccf8 100644
> --- a/src/lib/eina/eina_promise.h
> +++ b/src/lib/eina/eina_promise.h
> @@ -187,6 +187,26 @@ struct _Eina_Future_Scheduler {
> */
> typedef void (*Eina_Promise_Cancel_Cb) (void *data, const Eina_Promise
*dead_promise);
>
> +/*
> + * @typedef Eina_Promise_Data_Set_Cb Eina_Promise_Data_Set_Cb.
> + * @ingroup eina_promise
> + *
> + * A callback used to intercept eina_promise_data_set().
> + *
> + * Used internally by EFL - please do not use.
> + */
> +typedef void (*Eina_Promise_Data_Set_Cb) (Eina_Promise *p, void *data);
> +
> +/*
> + * @typedef Eina_Promise_Data_Free_Cb_Set_Cb
Eina_Promise_Data_Free_Cb_Set_Cb.
> + * @ingroup eina_promise
> + *
> + * A callback used to intercept eina_promise_data_set_cb_set().
> + *
> + * Used internally by EFL - please do not use.
> + */
> +typedef void (*Eina_Promise_Data_Free_Cb_Set_Cb) (Eina_Promise *p,
Eina_Free_Cb free_cb);
> +
> /**
> * @typedef Eina_Future_Success_Cb Eina_Future_Success_Cb.
> * @ingroup eina_future
> @@ -636,6 +656,28 @@ EAPI void eina_promise_data_set(Eina_Promise *p,
void *data) EINA_ARG_NONNULL(1)
> */
> EAPI void eina_promise_data_free_cb_set(Eina_Promise *p, Eina_Free_Cb
free_cb);
>
> +/**
> + * Sets a data set intercept function that can alter the behavior of
> + * eina_promise_data_set(). Please do not use this as it is only used
> + * internally inside EFL and may be used to slightly alter a promise
> + * behavior and if used on these promises may remove EFL's override
> + *
> + * @param[in] p The promise to set the data set callback on
> + * @param[in] data_set_cb The callabck to intercept the data set
> + */
> +EAPI void eina_promise_data_set_cb_set(Eina_Promise *p,
Eina_Promise_Data_Set_Cb data_set_cb);
> +
> +/**
> + * Sets a data free cb set intercept function that can alter the
behavior of
> + * eina_promise_data_free_cb_set(). Please do not use this as it is only
used
> + * internally inside EFL and may be used to slightly alter a promise
> + * behavior and if used on these promises may remove EFL's override
> + *
> + * @param[in] p The promise to set the data set callback on
> + * @param[in] data_free_cb_set_cb The callabck to intercept the data
free cb set
> + */
> +EAPI void eina_promise_data_free_cb_set_cb_set(Eina_Promise *p,
Eina_Promise_Data_Free_Cb_Set_Cb data_free_cb_set_cb);
> +
> /**
> * Resolves a promise.
> *
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
