GNOME Bugzilla – Bug 687196
filesystemmodel: invalidate nodes on file remove
Last modified: 2013-05-22 07:20:28 UTC
The file system model isn't properly invalidated when a file is removed from the model. This causes an assertion (r == n_visible_rows) to fail at some later point in the future when the model is re-sorted. The bug can be reproduced by opening a GtkFileChooser to a directory with non-hidden files, and renaming (using the terminal) a file (not the last) to be last in the sort order. This bug is reported with several duplicates happening on Ubuntu as https://bugs.launchpad.net/ubuntu/+source/gtk+3.0/+bug/851843. It may be related to https://bugzilla.gnome.org/show_bug.cgi?id=666158.
Created attachment 227637 [details] [review] filesystemmodel: invalidate nodes on file remove The file system model isn't properly invalidated when a file is removed from the model. This causes an assertion (r == n_visible_rows) to fail at some later point in the future when the model is re-sorted.
You are completely correct in saying that *right there* the model needs to be invalidated. However, the invalidation should already happen via the existing call to node_set_visible_and_filtered_out(). But now that I look at this again, there are some problems. That function can return prematurely if the node is already not visible (e.g. remove_file() on a file that was invisible due to being a hidden or backup file), and in that case, the model wouldn't be invalidated. Also, treemodel signals should only be emitted when the model itself is in a consistent state. Right now we emit them in the middle of things. Finally, I think remove_file() is a bit overzealous when it comes to clearing the files hash table. It should be enough to update the files that will get their indexes changed. I'm starting to do these fixes; this will hopefully fix the bug. I cannot reproduce it reliably yet :(
Thanks very much, Federico. After a bit of further testing, I see why the re-invalidation is required, or some suitable alternative fix. emit_row_deleted_for_row () emits a row-deleted signal, which indirectly triggers gtk_file_system_model_get_iter (), causing the model to re-validate *before* the file system node is finally removed from the model->files array. There's may be a better solution than re-invalidating at the end of remove_file (), but it would not be as trivial to fix. Also, it was suggested by others that we should use GSequence as a data structure instead. Here's a stack trace showing what's happening. Please ignore the line numbers.
+ Trace 231119
Perfect, thanks for figuring this out! That trace is exactly the evidence we needed. I'll make sure that signal emissions happen only when the model is in a consistent state. They have to be emitted outside of node_set_visible...() and at the very end of the add/remove functions.
These are the changes I'm making (in addition to some drive-by cleanups): * The filtering/visibility of the deleted node is not updated. The node will simply be gone; we don't need to update those values at all. * We invalidate just the node that is being deleted. * The model->file_lookup hash table is not completely nuked; instead, we carefully adjust its indices. * The row-deleted signal is only emitted at the very end, when deletion is complete and the model is consistent.
*** Bug 666158 has been marked as a duplicate of this bug. ***
*** Bug 653038 has been marked as a duplicate of this bug. ***
*** Bug 680586 has been marked as a duplicate of this bug. ***
I've pushed this to the master and gtk-3-6 branches. In subsequent commits I'll clean up add_file() and the other code that emits signals in not quite the right place. Thanks, William, for figuring this out! :)
I just got a similar crash report from 3.4.4 of evolution [1]. Do you think it is caused by the same reason as this bug report is filled against? I think it is, but it'll be better if you could confirm it. [1] https://bugzilla.redhat.com/show_bug.cgi?id=889612
*** Bug 680628 has been marked as a duplicate of this bug. ***
*** Bug 679257 has been marked as a duplicate of this bug. ***
*** Bug 689515 has been marked as a duplicate of this bug. ***