GNOME Bugzilla – Bug 686174
Crash in GtkUIManager's update_node
Last modified: 2014-08-30 05:07:41 UTC
Created attachment 226496 [details] Test program GtkUIManager's update_node seems to not handle the case that the node structure might change out from under it as it runs. Consider the case of a destroy signal handler further modifying the UIManager. See attached test program. It triggers two similar bugs: (1) deleting the next sibling node in the destruction handler and (2) deleting the parent node in the destruction handler. Run the program like so: valac --pkg gtk+-3.0 test.vala && G_SLICE=debug-blocks ./test The first bug you hit will be the sibling one. Unless you comment out that function call in the test, you won't ever get to the parent bug because you'll keep crashing.
Created attachment 226515 [details] [review] Proposed patch This whole thing is complicated because we can't just queue up updates if we are asked to do one while in the middle of an update. Because gtk_ui_manager_ensure_update must finish the whole update by the time it's done. So we have to handle nested updates immediately and cleanly. Here's one possible fix. It does two things: Before freeing a node's info, it removes the node from the tree. This helps if we nested-update the tree, because that node won't be visited twice. Secondly, it adds a guint stamp to mark each update. This lets us detect nested updates. If after updating a node, we notice we had a nested update, we just stop updating.
Review of attachment 226515 [details] [review]: Even though the use is a bit of a corner case (calling gtk_ui_manager_ensure_update() all the times from an application should not be required), it looks like a real bug, and I think we definitely should be robust against this. I added some comments inline. ::: gtk/gtkuimanager.c @@ +3062,3 @@ update_node (manager, current, in_popup, popup_accels); + if (manager->private_data->last_update_id != update_id) + return; // stop now if we have started a new update No C++ comments please @@ +3078,3 @@ if (node->children == NULL && info->uifiles == NULL) { + g_node_destroy (node); It seems safer to me to first unlink the node with g_node_unlink() here, and then destroy it normally as the previous code did.
I don't think connecting to ::destroy and recursing into uimanager like that is even remotely legal.
Note that while my sample code does call ensure_update explicitly, which makes it look like a corner case, both get_widget or get_action call ensure_update internally. Those presumably are commonly used. As for whether listening to the destroy signal of a UIManager's item is remotely legal... ::shrug:: it doesn't seem so unlikely that someone would listen to that and call get_widget or get_action. But this isn't just theoretical, it appears to cause downstream bug https://launchpad.net/bugs/1053862 which has quite a lot of duplicates.
Created attachment 226647 [details] [review] Proposed patch #2 Here's the patch with your suggested fixes (no c++ comment, use g_node_unlink).
Hello, Is there any progress on a fix for this bug?
GtkAction has been deprecated