Page MenuHomePhabricator

calling canvas event feed functions during a post event callback breaks the canvas
Closed, InvalidPublic

Description

Results like https://gist.github.com/zmike/e99b0c245874b76122e4, followed by a crash.

It seems that this level of recursion has not been accounted for.

zmike created this task.Feb 4 2016, 3:40 PM
zmike updated the task description. (Show Details)
zmike raised the priority of this task from to High.
zmike added a project: efl.
zmike added subscribers: zmike, cedric, jpeg, raster.

yregh. code is too different now to match that up do. but _evas_post_event_callback_call() has stuff to avoid recursion... do you have a simple test case?

cedric assigned this task to jpeg.Jul 19 2016, 11:09 AM
jpeg added a comment.Jul 21 2016, 4:05 AM

A test case would be good.

jpeg closed this task as Wontfix.Feb 15 2017, 10:15 PM

We might not want to do this at all. In fact we will document evas_post_event_callback_push() as "do not modify the canvas or otherwise trigger events to the canvas from this callback". You can use a job to feed events or modify the canvas safely.

jpeg reopened this task as Open.Feb 16 2017, 2:47 AM

Will fix in a more satisfying manner.

zmike reopened this task as Open.Feb 22 2017, 8:17 AM

Hm now I get double event callbacks. 100% repro case:

  1. add bryce
  2. click start menu gadget
  3. get 2x mouse down event callbacks (initial down event, feed up event, second down event)

Not entirely sure why this is happening...

jpeg closed this task as Invalid.Feb 27 2017, 2:11 AM

I get a crash as a result, in the post cb, as the event info is used without null check.
Anyway this happens because you feed events from the mouse,down callback. Please don't do that.
Here's the flow:

  • mouse,down on bryce, event id N
    • bryce mouse down cb
    • _button_cb_mouse_down: creates a menu:
      • menu trigger feeds mouse,up
      • last event on bryce is now a mouse,up with id M > N
  • back to the top level event, a child of a child of a child finally gets the event and it is propagated all the way back up to the bryce. normally this duplicated mouse,down event would be avoided, but since there was a fake mouse,up in between everything is messed up.

Please find a better solution than feeding events. It's really ugly, especially in this situation. You might also want to consider jobs before creating evas objects, otherwise mouse,in/out, show and resize events may also mess up the event flow. But definitely don't feed mouse events. Evas can't even count how many ups & downs it gets because this up is not preceded by any down.

zmike reopened this task as Open.Feb 28 2017, 4:16 AM

Enlightenment has been doing this for years in a number of places without any issues. I'm confused why this is suddenly invalid use of library and not a library regression.

zmike added a comment.Feb 28 2017, 4:24 AM

Now that I think about it, would feeding a better timestamp be useful here? It seems like it should be possible, during recursion, to determine if a mouse down event is being processed "after" its corresponding up event and then reject it.

jpeg added a comment.Mar 1 2017, 6:41 PM

Mouse,up needs to be preceeded by mouse,down, but also by the appropriate mouse,in (if needed) and mouse,move (if position changed). If any event is fed manually rather than letting the normal inputs do their work, this can break evas internal logic. If E has been doing it for years and it "works" this was out of pure luck, and because those post callbacks were specifically designed to not be recursive (a really shitty design & implementation btw). The timestamp change will not change anything as evas does not postpone an event until said timestamp has been reached. An invalid, future or past, timestamp would just be a sign of a misbehaving display manager, faulty hardware or driver or whatnot. Evas is not handling any of these cases. The best it could then do would be to simply ignore the event.

Specifically here, how is Evas meant to know if the mouse button is up or down when E feeds fake events during a mouse,down callback? Events should really be fed only for debugging purposes (eg. exactness).

jpeg closed this task as Invalid.Mar 2 2017, 12:39 AM
zmike added a comment.Mar 2 2017, 3:59 AM

The intent for the feed was that the second (real) mouse up would just be ignored since there was no preceding mouse down event. I'm pretty sure this is how it had been working for all this time...

jpeg added a comment.Mar 2 2017, 5:09 AM

Sounds correct but doing it with a post-event callback breaks this logic.
So maybe have a look at my latest commit trying to fixup the post-event callback logic, and think whether the behaviour is better now or before. Honestly I am not sure either.
But feeding events from inside the post-event cb is not a great idea (ask raster). Use a job from the post-event cb.

zmike added a comment.Mar 2 2017, 10:54 AM

Okay, I'm willing to compromise on this:

I'll move the current use to a job or something, and you add docs to the feed functions about them not being usable in post-event callbacks and then also add checks to CRI/abort/whatever if the user does this.

raster added a comment.Mar 2 2017, 3:46 PM

indeed it isn't a great idea... post event's were added (pre efl 1.0) to deal with nested scrollers... and feeding an up inside an event cb for down kind of works only if no repeat events are being processed etc... but only then... it's a bit "flaky" so yeah a job likely is far better as it moves the event sources back to their original place - directly from the event loop handling and not deep inside a nested set of event cb's between objects

jpeg added a comment.Mar 2 2017, 5:10 PM
In T3144#83011, @zmike wrote:

[...] you add docs to the feed functions about them not being usable in post-event callbacks and then also add checks to CRI/abort/whatever if the user does this.

I already added some doc in the post-event cb function:

* This function should only be called from inside an evas input event
* callback. The event_info data may be kept up until @p func is called, in
* order to check the state of the "on-hold" flag for instance. Do not modify
* the canvas or otherwise trigger or feed a events to the canvas from inside
* @p func. Use jobs to safely modify the canvas.
zmike added a comment.Mar 3 2017, 7:54 AM

I think it should throw errors too.

jpeg added a comment.Mar 6 2017, 6:46 PM
In T3144#83054, @zmike wrote:

I think it should throw errors too.

Yeah it probably should. That's what I did at first, so the code is still there. What do you think this should do? Change DBG to ERR? Or even abort the function call (return from the feed function)?
From evas_events.c:33:

static inline void
_evas_event_feed_check(Evas_Public_Data *e)
{
   if (EINA_LIKELY(!e->running_post_events)) return;
   DBG("Feeding new input events from a post-event callback is risky!");
}

#define EVAS_EVENT_FEED_SAFETY_CHECK(evas) _evas_event_feed_check(evas)

Ideally it should abort the function call. But I'm worried of breaking existing stuff...

zmike added a comment.Mar 7 2017, 4:05 AM

I think CRI would be fine