-
Notifications
You must be signed in to change notification settings - Fork 590
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix crash closing directory from the openfiles sidebar #3535
Conversation
When the openfiles sidebar shows documents as a tree, closing a document can lead to sever re-layout of the view (e.g. collapsing directory nodes together). This makes walking the tree and closing documents at the same time highly tricky, as nodes might be shifting as we go. This lead to invalid memory access, and unexpected results, when closing some tree structures. One example of Valgrind showing how bad things are: ==917061== Invalid read of size 8 ==917061== at 0x5B31345: g_node_nth_child (gnode.c:1052) ==917061== by 0x50A7361: gtk_tree_store_iter_nth_child (gtktreestore.c:793) ==917061== by 0x491DB76: on_openfiles_document_action_apply (sidebar.c:1330) ==917061== by 0x491DC0C: on_openfiles_document_action (sidebar.c:1347) ==917061== by 0x5A833AF: g_closure_invoke (gclosure.c:832) ==917061== by 0x5A96075: signal_emit_unlocked_R.isra.0 (gsignal.c:3796) ==917061== by 0x5A9CBF4: g_signal_emit_valist (gsignal.c:3549) ==917061== by 0x5A9CDBE: g_signal_emit (gsignal.c:3606) ==917061== by 0x50D7833: gtk_widget_activate (gtkwidget.c:7845) ==917061== by 0x4F8A935: gtk_menu_shell_activate_item (gtkmenushell.c:1375) ==917061== by 0x4F8AC70: gtk_menu_shell_button_release (gtkmenushell.c:791) ==917061== by 0x4DFBCB3: _gtk_marshal_BOOLEAN__BOXEDv (gtkmarshalers.c:130) ==917061== Address 0x9bcdc20 is 32 bytes inside a block of size 40 free'd ==917061== at 0x484317B: free (vg_replace_malloc.c:872) ==917061== by 0x5B3068B: g_nodes_free (gnode.c:123) ==917061== by 0x5B3068B: g_node_destroy (gnode.c:143) ==917061== by 0x50AA8F2: gtk_tree_store_remove (gtktreestore.c:1229) ==917061== by 0x491F851: sidebar_openfiles_remove_iter (sidebar.c:959) ==917061== by 0x491F8AE: openfiles_remove (sidebar.c:972) ==917061== by 0x491FA6C: sidebar_remove_document (sidebar.c:1027) ==917061== by 0x48D0B5B: remove_page (document.c:733) ==917061== by 0x48D29C0: document_remove_page (document.c:787) ==917061== by 0x48D29FC: document_close (document.c:695) ==917061== by 0x491DAED: document_action (sidebar.c:1299) ==917061== by 0x491DB47: on_openfiles_document_action_apply (sidebar.c:1322) ==917061== by 0x491DB9E: on_openfiles_document_action_apply (sidebar.c:1332) ==917061== Block was alloc'd at ==917061== at 0x48407B4: malloc (vg_replace_malloc.c:381) ==917061== by 0x5B2C678: g_malloc (gmem.c:130) ==917061== by 0x5B45011: g_slice_alloc (gslice.c:1074) ==917061== by 0x5B305BD: g_node_new (gnode.c:110) ==917061== by 0x50AAF0B: gtk_tree_store_insert_before (gtktreestore.c:1375) ==917061== by 0x491E725: tree_add_new_dir (sidebar.c:654) ==917061== by 0x491E89A: get_parent_for_file (sidebar.c:840) ==917061== by 0x491E995: sidebar_openfiles_add_iter (sidebar.c:869) ==917061== by 0x491F5C7: sidebar_openfiles_add (sidebar.c:901) ==917061== by 0x48D0CE4: document_create (document.c:662) ==917061== by 0x48D3840: document_open_file_full (document.c:1349) ==917061== by 0x48D3C81: document_open_file (document.c:914) Fix this by decoupling tree walking from closing documents. We now do two passes: first we collect documents to work on walking the tree as before, and only then we perform the action on each node of the list. Fixes geany#3527.
Not tested but seems ok in principle. |
Anybody up for giving this a test spin? I'm running that daily, but a second opinion is always welcome :) |
So, first, cool, one can close all files from one directory, I didn't know about that! I just had a look at the patch which seems to do the right thing and also tested it a bit. I wasn't able to reproduce the crash, just the incorrect closing of directories with the original version and the patch fixes it for me. So from my point of view the patch can be merged. |
When the openfiles sidebar shows documents as a tree, closing a document can lead to sever re-layout of the view (e.g. collapsing directory nodes together). This makes walking the tree and closing documents at the same time highly tricky, as nodes might be shifting as we go.
This lead to invalid memory access, and unexpected results, when closing some tree structures.
One example of Valgrind showing how bad things are:
Fix this by decoupling tree walking from closing documents. We now do two passes: first we collect documents to work on walking the tree as before, and only then we perform the action on each node of the list.
Fixes #3527.