From 300de6615c425001d901bc98ba6db2dc5f9afa1f Mon Sep 17 00:00:00 2001 From: Seunghun Lee Date: Mon, 7 Nov 2022 17:18:13 +0900 Subject: [PATCH] viewport: Do not use evas_object_event_callback_del() For short, the `evas_object_event_callback_del_full()` should be used instead of `evas_object_event_callback_del()`. The crash happened as the backtrace shown below, and it seems that the crash was due to the misuse of `evas_object_event_callback_del()`. This patch also log more about the viewport just in case the patch may not fix the crash. backtrace: (gdb)bt #0 0x000d6e98 in e_object_is_del (obj=0x6) at /usr/src/debug/enlightenment-0.20.0/src/bin/e_object.c:106 #1 0x00086088 in _e_comp_wl_viewport_cb_parent_show(data=0x1d50c70, e=, obj=, event_info=) at /usr/src/debug/enlightenment-0.20.0/src/bin/e_comp_wl_viewport.c:572 #2 0xb5d91df6 in _eo_evas_object_cb (data=0x207e4c8, event=) at /usr/src/debug/efl-1.25.1/builddir/../src/lib/evas/canvas/evas_callbacks.c:181 ... The evas_object_event_callback_del() is to remove the most recently added callback from the object. Having used it in `e_comp_wl_viewport.c` could have caused unintentional removal of another callback depending on the timing of creation and destruction of several viewports, and this eventually could result in a crash. I couldn't find the clear evidence that this really led the crash, but let me give you an example what situation can make this crash. 1. There are 2 viewports created with the same parent. So, 2 viewports would add a callback for its own. I will call both viewport as `A` and `B`, and `A` is created before `B`. 2. The viewport `A` try to delete its callback from the parent evas object because of the change of subsurface parent by calling `evas_object_event_callback_del()`. And this will unintentionally delete the callback of `B`, not `A` because the callback of `B` is the most recently added one. 3. Let's say the viewport `A` gets deleted by `tizen_viewport.destroy`, and it does not call the `evas_object_event_callback_del_full()` because `viewport->epc` must already be null. And now, the data for the callback `A` becomes freed memory, but the callback `A` is still in callback list of parent object. 4. Event if the viewport `B` gets deleted by `tizen_viewport.destroy`, it won't be able to remove any callbacks with `evas_object_event_callback_del_full()` because the callback of `B` has already been removed by the process 2. As I mentioned, I'm not sure this really led the crash but I believe and hope that this will fix the crash. Change-Id: Ie92900df80d2351bbf9054fd3c4e9e6184b67d57 --- src/bin/e_comp_wl_viewport.c | 208 ++++++++++++++++++++++--------------------- 1 file changed, 107 insertions(+), 101 deletions(-) diff --git a/src/bin/e_comp_wl_viewport.c b/src/bin/e_comp_wl_viewport.c index f2279d0..41b31da 100644 --- a/src/bin/e_comp_wl_viewport.c +++ b/src/bin/e_comp_wl_viewport.c @@ -8,17 +8,14 @@ #include #include -#define PER(fmt,arg...) ELOGF("VIEWPORT", " window(0x%08"PRIxPTR") epc(%p): " \ - fmt, viewport->ec, \ - viewport->window, viewport->epc, ##arg) -#define PWR(fmt,arg...) ELOGF("VIEWPORT", " window(0x%08"PRIxPTR") epc(%p): " \ - fmt, viewport->ec, \ - viewport->window, viewport->epc, ##arg) -#define PIN(fmt,arg...) ELOGF("VIEWPORT", " window(0x%08"PRIxPTR") epc(%p): " \ - fmt, viewport->ec, \ - viewport->window, viewport->epc, ##arg) -#define PDB(fmt,arg...) DBG("window(0x%08"PRIxPTR") ec(%p) epc(%p): "fmt, \ - viewport->window, viewport->ec, viewport->epc, ##arg) +#define PER(fmt,arg...) ELOGF("VIEWPORT", "viewport(%p) epc(%p): " \ + fmt, viewport->ec, viewport, viewport->epc, ##arg) +#define PWR(fmt,arg...) ELOGF("VIEWPORT", "viewport(%p) epc(%p): " \ + fmt, viewport->ec, viewport, viewport->epc, ##arg) +#define PIN(fmt,arg...) ELOGF("VIEWPORT", "viewport(%p) epc(%p): " \ + fmt, viewport->ec, viewport, viewport->epc, ##arg) +#define PDB(fmt,arg...) DBG("viewport(%p) window(0x%08"PRIxPTR") ec(%p) epc(%p): "fmt, \ + viewport, viewport->window, viewport->ec, viewport->epc, ##arg) #undef SWAP #define SWAP(a, b) ({double t; t = a; a = b; b = t;}) @@ -101,9 +98,9 @@ typedef struct _E_Viewport } E_Viewport; static E_Viewport* _e_comp_wl_viewport_get_viewport(struct wl_resource *resource); -static void _e_comp_wl_viewport_cb_parent_show(void *data, Evas *e, Evas_Object *obj, void *event_info); -static void _e_comp_wl_viewport_cb_parent_resize(void *data, Evas *e, Evas_Object *obj, void *event_info); static void _e_comp_wl_viewport_parent_check(E_Viewport *viewport); +static void _e_comp_wl_viewport_parent_set(E_Viewport *viewport, E_Client *epc); +static void _e_comp_wl_viewport_parent_unset(E_Viewport *viewport); static void _destroy_viewport(E_Viewport *viewport) @@ -112,18 +109,13 @@ _destroy_viewport(E_Viewport *viewport) if (!viewport) return; + PIN("destroy E_Viewport"); + ec = viewport->ec; ecore_event_handler_del(viewport->topmost_rotate_hdl); - if (viewport->epc) - { - evas_object_event_callback_del_full(viewport->epc->frame, EVAS_CALLBACK_SHOW, - _e_comp_wl_viewport_cb_parent_show, viewport); - evas_object_event_callback_del_full(viewport->epc->frame, EVAS_CALLBACK_RESIZE, - _e_comp_wl_viewport_cb_parent_resize, viewport); - viewport->epc = NULL; - } + _e_comp_wl_viewport_parent_unset(viewport); e_client_hook_del(viewport->client_hook_del); e_client_hook_del(viewport->client_hook_move); @@ -151,8 +143,6 @@ _destroy_viewport(E_Viewport *viewport) ec->comp_data->pending.buffer_viewport.changed = 1; } - PIN("tizen_viewport@%d destroy. viewport(%p)", wl_resource_get_id(viewport->resource), viewport); - free(viewport); } @@ -197,12 +187,9 @@ _client_cb_del(void *data, E_Client *ec) viewport = data; if (viewport->epc != ec) return; - evas_object_event_callback_del(viewport->epc->frame, EVAS_CALLBACK_SHOW, - _e_comp_wl_viewport_cb_parent_show); - evas_object_event_callback_del(viewport->epc->frame, EVAS_CALLBACK_RESIZE, - _e_comp_wl_viewport_cb_parent_resize); PIN("epc del"); - viewport->epc = NULL; + + _e_comp_wl_viewport_parent_unset(viewport); } static void @@ -247,33 +234,9 @@ _e_comp_wl_viewport_parent_check(E_Viewport *viewport) if (e_comp_wl_subsurface_check(ec)) new_parent = e_comp_wl_subsurface_parent_get(ec); - if (viewport->epc == new_parent) return; - - if (viewport->epc) - { - evas_object_event_callback_del(viewport->epc->frame, - EVAS_CALLBACK_SHOW, - _e_comp_wl_viewport_cb_parent_show); - evas_object_event_callback_del(viewport->epc->frame, - EVAS_CALLBACK_RESIZE, - _e_comp_wl_viewport_cb_parent_resize); - } - - viewport->epc = new_parent; + PIN("parent_check: parent(%p)", new_parent); - PIN("epc(%p)", viewport->epc); - - if (viewport->epc) - { - evas_object_event_callback_add(viewport->epc->frame, - EVAS_CALLBACK_SHOW, - _e_comp_wl_viewport_cb_parent_show, - viewport); - evas_object_event_callback_add(viewport->epc->frame, - EVAS_CALLBACK_RESIZE, - _e_comp_wl_viewport_cb_parent_resize, - viewport); - } + _e_comp_wl_viewport_parent_set(viewport, new_parent); } static void @@ -564,52 +527,6 @@ static const struct tizen_destination_mode_interface _e_comp_wl_destination_mode }; static void -_e_comp_wl_viewport_cb_parent_show(void *data, Evas *e EINA_UNUSED, Evas_Object *obj EINA_UNUSED, void *event_info EINA_UNUSED) -{ - E_Viewport *viewport; - - viewport = data; - if (e_object_is_del(E_OBJECT(viewport->ec))) - return; - - if (e_comp_wl_viewport_is_changed(viewport->ec)) - { - PIN("Parent gets visible and viewport has changed, " - "try to apply viewport to ec(%p)", viewport->ec); - e_comp_wl_viewport_apply(viewport->ec); - } -} - -static void -_e_comp_wl_viewport_cb_parent_resize(void *data, Evas *e EINA_UNUSED, Evas_Object *obj EINA_UNUSED, void *event_info EINA_UNUSED) -{ - E_Viewport *viewport = data; - Evas_Coord old_w, old_h; - - if (e_object_is_del(E_OBJECT(viewport->epc))) return; - - if (viewport->query_parent_size) - { - old_w = viewport->parent_size.w; - old_h = viewport->parent_size.h; - - evas_object_geometry_get(viewport->epc->frame, - &viewport->parent_size.x, - &viewport->parent_size.y, - &viewport->parent_size.w, - &viewport->parent_size.h); - - if ((old_w != viewport->parent_size.w) || - (old_h != viewport->parent_size.h)) - tizen_viewport_send_parent_size(viewport->resource, - viewport->parent_size.w, - viewport->parent_size.h); - } - - e_comp_wl_viewport_apply(viewport->ec); -} - -static void _e_comp_wl_viewport_destroy(struct wl_resource *resource) { E_Viewport *viewport; @@ -1848,7 +1765,7 @@ e_comp_wl_viewport_create(struct wl_resource *resource, viewport->surface_destroy_listener.notify = _e_comp_wl_viewport_cb_surface_destroy; wl_resource_add_destroy_listener(ec->comp_data->surface, &viewport->surface_destroy_listener); - PIN("tizen_viewport@%d viewport(%p) created", id, viewport); + PIN("create viewport"); return EINA_TRUE; } @@ -2233,3 +2150,92 @@ e_comp_wl_viewport_shutdown(void) { e_info_server_hook_set("viewport", NULL, NULL); } + +static void +_e_comp_wl_viewport_cb_parent_show(void *data, Evas *e EINA_UNUSED, Evas_Object *obj EINA_UNUSED, void *event_info EINA_UNUSED) +{ + E_Viewport *viewport = data; + + if (e_object_is_del(E_OBJECT(viewport->ec))) return; + + if (e_comp_wl_viewport_is_changed(viewport->ec)) + { + PIN("Parent gets visible and viewport has changed, " + "try to apply viewport to ec(%p)", viewport->ec); + e_comp_wl_viewport_apply(viewport->ec); + } +} + +static void +_e_comp_wl_viewport_cb_parent_resize(void *data, Evas *e EINA_UNUSED, Evas_Object *obj EINA_UNUSED, void *event_info EINA_UNUSED) +{ + E_Viewport *viewport = data; + Evas_Coord old_w, old_h; + + if (e_object_is_del(E_OBJECT(viewport->epc))) return; + + if (viewport->query_parent_size) + { + old_w = viewport->parent_size.w; + old_h = viewport->parent_size.h; + + evas_object_geometry_get(viewport->epc->frame, + &viewport->parent_size.x, + &viewport->parent_size.y, + &viewport->parent_size.w, + &viewport->parent_size.h); + + if ((old_w != viewport->parent_size.w) || + (old_h != viewport->parent_size.h)) + tizen_viewport_send_parent_size(viewport->resource, + viewport->parent_size.w, + viewport->parent_size.h); + } + + e_comp_wl_viewport_apply(viewport->ec); +} + +static void +_e_comp_wl_viewport_parent_set(E_Viewport *viewport, E_Client *epc) +{ + if (viewport->epc == epc) return; + + PIN("parent_set: new_parent(%p)", epc); + + _e_comp_wl_viewport_parent_unset(viewport); + + if (!epc) + return; + + viewport->epc = epc; + + EINA_SAFETY_ON_NULL_RETURN(epc->frame); + + evas_object_event_callback_add(epc->frame, EVAS_CALLBACK_SHOW, + _e_comp_wl_viewport_cb_parent_show, + viewport); + evas_object_event_callback_add(epc->frame, EVAS_CALLBACK_RESIZE, + _e_comp_wl_viewport_cb_parent_resize, + viewport); +} + +static void +_e_comp_wl_viewport_parent_unset(E_Viewport *viewport) +{ + if (!viewport->epc) return; + + PIN("parent_unset"); + + EINA_SAFETY_ON_NULL_GOTO(viewport->epc->frame, end); + + evas_object_event_callback_del_full(viewport->epc->frame, + EVAS_CALLBACK_SHOW, + _e_comp_wl_viewport_cb_parent_show, + viewport); + evas_object_event_callback_del_full(viewport->epc->frame, + EVAS_CALLBACK_RESIZE, + _e_comp_wl_viewport_cb_parent_resize, + viewport); +end: + viewport->epc = NULL; +} -- 2.7.4