GNOME Bugzilla – Bug 774120
Fix crash on termination of edit-pane
Last modified: 2016-11-09 13:44:07 UTC
o/
Created attachment 339353 [details] [review] gtd-task: Avoid crash by using weak reference on GtdTaskList The crash happens as GtdTaskList was freed but GtdTask keeps a dangling pointer to it. In gtd_manager_update_task(), gtd_task_get_list() is called returning a dangling pointer which is provided to gtd_task_list_get_provider(), causing the crash on GTD_IS_TASK_LIST(). As documentation in gtd_task_get_list() says that it holds a weak reference to GtdTaskList but it did not do the weak_ref, this patch tries to fix that. If GtdTaskList is freed now, task_list_weak_notified() will set the priv->list pointer to NULL. Thread 1 "gnome-todo" received signal SIGSEGV, Segmentation fault. 0x0000000000426aa0 in GTD_IS_TASK_LIST (ptr=0x1251b70) at gtd-task-list.h:32 G_DECLARE_DERIVABLE_TYPE (GtdTaskList, gtd_task_list, GTD, TASK_LIST, GtdObject) (gdb) bt #0 in GTD_IS_TASK_LIST (ptr=0x1251b70) at gtd-task-list.h:32 #1 in gtd_task_list_get_provider (list=0x1251b70) at gtd-task-list.c:525 #2 in gtd_manager_update_task (manager=0x86cd00, task=0x12fc4a0) at engine/gtd-manager.c:607 #3 in gtd_task_list_view__edit_task_finished (pane=0xe39f80, task=0x12fc4a0, user_data=0x95ef90) at gtd-task-list-view.c:686 #4 in g_cclosure_marshal_VOID__OBJECTv (closure=0xe7feb0, return_value=0x0, instance=0xe39f80, args=0x7fffffff9a38, marshal_data=0x0, n_params=1, param_types=0x942930) at /jhbuild/glib/gobject/gmarshal.c:2102 #5 in _g_closure_invoke_va (closure=0xe7feb0, return_value=0x0, instance=0xe39f80, args=0x7fffffff9a38, n_params=1, param_types=0x942930) at /jhbuild/glib/gobject/gclosure.c:867 #6 in g_signal_emit_valist (instance=0xe39f80, signal_id=299, detail=0, var_args=0x7fffffff9a38) at /jhbuild/glib/gobject/gsignal.c:3300 #7 in g_signal_emit (instance=0xe39f80, signal_id=299, detail=0) at /jhbuild/glib/gobject/gsignal.c:3447 #8 in gtd_edit_pane_set_task (pane=0xe39f80, task=0x0) at gtd-edit-pane.c:446 #9 in gtd_edit_pane_dispose (object=0xe39f80) at gtd-edit-pane.c:258
Created attachment 339354 [details] [review] manager: do not ask update task without parent list This situation is fairly possible when gnome-todo is being terminated.
Note that I'm not sure how to reproduce the crash. As I'm using git master, I'm always running gnome-todo under gdb. I use it normally and when it crashes I try to fix base on backtrace or I simply report the crash... Cheers
Review of attachment 339353 [details] [review]: Looks good overall, just some minor style nitpicking. ::: src/gtd-task.c @@ +293,2 @@ static void +task_list_weak_notified(gpointer data, Style issue. @@ +306,3 @@ GtdTaskPrivate *priv = gtd_task_get_instance_private (self); + if (priv->list != NULL) "if (priv->list)" please @@ +1090,2 @@ priv = gtd_task_get_instance_private (task); + if (priv->list == list) Jump line before the 'if'. @@ +1094,1 @@ + if (priv->list != NULL) Ditto.
Created attachment 339384 [details] [review] --- v2: diff https://paste.fedoraproject.org/476231/93343147/ --- gtd-task: Avoid crash by using weak reference on GtdTaskList The crash happens as GtdTaskList was freed but GtdTask keeps a dangling pointer to it. In gtd_manager_update_task(), gtd_task_get_list() is called returning a dangling pointer which is provided to gtd_task_list_get_provider(), causing the crash on GTD_IS_TASK_LIST(). As documentation in gtd_task_get_list() says that it holds a weak reference to GtdTaskList but it did not do the weak_ref, this patch tries to fix that. If GtdTaskList is freed now, task_list_weak_notified() will set the priv->list pointer to NULL. Thread 1 "gnome-todo" received signal SIGSEGV, Segmentation fault. 0x0000000000426aa0 in GTD_IS_TASK_LIST (ptr=0x1251b70) at gtd-task-list.h:32 G_DECLARE_DERIVABLE_TYPE (GtdTaskList, gtd_task_list, GTD, TASK_LIST, GtdObject) (gdb) bt #0 in GTD_IS_TASK_LIST (ptr=0x1251b70) at gtd-task-list.h:32 #1 in gtd_task_list_get_provider (list=0x1251b70) at gtd-task-list.c:525 #2 in gtd_manager_update_task (manager=0x86cd00, task=0x12fc4a0) at engine/gtd-manager.c:607 #3 in gtd_task_list_view__edit_task_finished (pane=0xe39f80, task=0x12fc4a0, user_data=0x95ef90) at gtd-task-list-view.c:686 #4 in g_cclosure_marshal_VOID__OBJECTv (closure=0xe7feb0, return_value=0x0, instance=0xe39f80, args=0x7fffffff9a38, marshal_data=0x0, n_params=1, param_types=0x942930) at /jhbuild/glib/gobject/gmarshal.c:2102 #5 in _g_closure_invoke_va (closure=0xe7feb0, return_value=0x0, instance=0xe39f80, args=0x7fffffff9a38, n_params=1, param_types=0x942930) at /jhbuild/glib/gobject/gclosure.c:867 #6 in g_signal_emit_valist (instance=0xe39f80, signal_id=299, detail=0, var_args=0x7fffffff9a38) at /jhbuild/glib/gobject/gsignal.c:3300 #7 in g_signal_emit (instance=0xe39f80, signal_id=299, detail=0) at /jhbuild/glib/gobject/gsignal.c:3447 #8 in gtd_edit_pane_set_task (pane=0xe39f80, task=0x0) at gtd-edit-pane.c:446 #9 in gtd_edit_pane_dispose (object=0xe39f80) at gtd-edit-pane.c:258
Created attachment 339385 [details] [review] --- v2: just rebase --- manager: do not ask update task without parent list This situation is fairly possible when gnome-todo is being terminated.
Review of attachment 339384 [details] [review]: Looks good now. Thanks for working on that.
Review of attachment 339385 [details] [review]: Minor nitpicking, as usual. ::: src/engine/gtd-manager.c @@ +606,2 @@ list = gtd_task_get_list (task); + if (list == NULL) Jump line before 'if', move the comment below to before this 'if', and simplify to 'if (list)'
Created attachment 339389 [details] [review] manager: do not ask update task without parent list This situation is fairly possible when gnome-todo is being terminated.
Review of attachment 339389 [details] [review]: Apply those other style fixed and it's ok to push. ::: src/engine/gtd-manager.c @@ +608,3 @@ + if (!list) + { + /* Task does not have parent list, nothing we can do */ Move this comment to before the 'if' and remove the brackets. @@ +610,3 @@ + /* Task does not have parent list, nothing we can do */ + return; + } Jump another line between the end brackets and the next line.
Attachment 339389 [details] pushed as 8afb0d9 - manager: do not ask update task without parent list
Attachment 339384 [details] pushed as f1a8e5c964a1b4939d4517aebaf0eaca05fea519 (Not sure why git bz failed this one)