Crash closing directory from the openfiles sidebar

[b4n]

Closing some directory structure from the sidebar tree (when showing the tree at least) sometimes crashes on master (and always makes Valgrind angry).

Steps to reproduce:

Given this file tree:

$ find /tmp/root/ -type f
/tmp/root/2
/tmp/root/1/1.1/1.1.2
/tmp/root/1/1.1/1.1.1
/tmp/root/1/1.2/1.2.2
/tmp/root/1/1.2/1.2.1

1. run Geany on those files (geany -c /tmp/_new_config $(find /tmp/root/ -type f)
2. right-click on e.g. directory /tmp/root/1 in the document sidebar, and select "close"
3. you'll either see a crash, or that not everything under that directory got closed properly

Here's Valgrind being unhappy:

$ valgrind geany -c /tmp/_new_config $(find /tmp/root/ -type f)
[…]
==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)
==917061== 

I believe this is due to collapsing directories together when there's only one child. This leads to tree iters changing, and overall structure shifting while navigating it -- even if it's down from the bottom-up depth-first as it is.

It's probably not trivial to fix, and I failed to do so quickly. One reason why it's not that easy is that it has to handle all this shifting and allow the tree to stay in the end if e.g. a file was not saved and the user cancelled the close request (my attempt was just to repeatedly close the first node until there was no more, but obviously that's not working).

[b4n]

@kugel- I assigned this to you as you're likely most knowledgeable about this. I might try to tackle this one if you can't, but if "recent" history is to be believed, I won't find time… 😕

[b4n]

One possibly easy fix or at least workaround could be collecting documents to perform actions on, and only then perform the actions. So walking the tree is disconnected from anything that might alter it.
This is kind of a workaround, but it seems a lot easier than coupling the tree relayout and tree walking in this case.