GNOME Bugzilla – Bug 770965
segfault on undo after failed batch rename attempt
Last modified: 2016-09-15 12:26:49 UTC
On certain conditions nautilus segfaults. How to reproduce: 1. create some files: touch 0.txt 1.txt 2. select all those files and select rename 3. select some pattern. Say, [Original file name][1, 2, 3] 4. Now create a file with conflicting filename in the same path touch 01.txt 5. Now click rename. Nautilus renames files except the conflicting one. 6. Now press Ctrl+z (undo). Nautilus segfaults here, which shouldn't happen This can happen when some user try to rename some files before a copy operation is complete, and so on.
This also happens when no action is given. Eg: 1. Select some files -> rename 2. Click "Rename" (Don't do anything else) 3. Press Ctrl+z (undo). Nautilus now segfaults.
Created attachment 335023 [details] [review] nautilus-file: Fix segfault on undo for batch rename In some cases, if some of the files are not renamed, for reasons like deletion or files having the same name, the undo would fail. The problem was that the files and names lists provided for undo also contained info about the files for which the rename failed. The fix was to add undo info only for files that are actually renamed and prepare the undo only if at least a file has successfully been renamed.
The patch above only covers the second case. For the first one, the conflict should be identified. The problem is that the conflicts are only checked when the entry changes, but there should be made a check also when the directory changes, so the directory should be monitored.
Comment on attachment 335023 [details] [review] nautilus-file: Fix segfault on undo for batch rename Attachment 335023 [details] pushed as f686cc9 - nautilus-file: Fix segfault on undo for batch rename
Review of attachment 335023 [details] [review]: Indeed, thanks!! Just small nitpick, feel free to commit after changing. The tittle of the commit message is too abstract "fix a crash". That's the actual problem, not the fix the patch does. The tittle should reflect the change. "don't add skipped files to the undo operation" Then in the body of the commit message you say something like (not literally): We were adding the skipped files to the undo operations (what we were doing). This is wrong because those files are not actually affected (why it is wrong). This was causing some crashes (what problem was causing). To fix it, don't add skipped files to the undo data (what we do to fix it, without mentioning the actual code).
Created attachment 335340 [details] [review] batch-rename-dialog: Check if directories get new files If a new file is added to the directory (or directories, in search), then the batch rename dialog should check if there will be any conflicts involving the new added file(s). To do this, all directories were connected to the files-added signal.
Review of attachment 335340 [details] [review]: Thanks for looking into this! I think it's missing the most complex part though. See comments. ::: src/nautilus-batch-rename-dialog.c @@ +3044,3 @@ + dialog = NAUTILUS_BATCH_RENAME_DIALOG (callback_data); + +{ is this enough? Doesn't the selection change if some file got deleted, and also the metadata has to be reseted again etc? @@ +3198,3 @@ + dir = nautilus_directory_get_by_uri (l->data); + + g_signal_connect_object (dir, "files-added", Why the files-added signal? We want to test when a file is deleted or changed too no? Also, when do you disconnect to this signal?
Created attachment 335387 [details] [review] batch-rename-dialog: Monitor directories for changes If a new file is added to the directory (or directories, in search), then the batch rename dialog should check if there will be any conflicts involving the new added file(s). If a file is deleted, the dialog should update the list of files by removing that file from it. To do this, all directories were connected to the files-added and files-changed signals.
Review of attachment 335387 [details] [review]: This patch looks much better, and surprisingly not so complex. Nice! Still some work to do: In the commit message the "how we fix it" part is using code specific words, like files-added and files-changed. It has to be understandable even if you don't know the specific code. How about "To do this, monitor the directories the files belong to and update the dialog when a file changes or gets deleted". Also, did you test it with search? Removing a file as one case, and changing it's name (to a one that conflicts) as a different case while in batch renaming. Ideally we should have unittests for this, but we are missing it for the entire batch renaming yet so..it's not a problem now as long as you test it manually. ::: src/nautilus-batch-rename-dialog.c @@ +2199,3 @@ + dialog = NAUTILUS_BATCH_RENAME_DIALOG (callback_data); + + for (l = dialog->selection; l != NULL; l = l->next) put the index in the for @@ +2204,3 @@ + + if (nautilus_file_is_gone (file)) + { can you extract this code that removes a file from batch renaming to another function? remove_file_from_selection or so. Also what happen if the number of files decreases to 0? I believe everything is going to break afterwards :) We have to gratefully handle this somehow. I'm fine if for now we destroy the dialog and print a warning (since we are in UI freeze and late in the cycle, just a command line warning so at least we can know if we receive a bug report if it's was because of that). Ideally for 3.24 (although we can work on that already) we would replace the rows with an "empty state"[0] and insensitivize the entries and the rename button, only allowing to cancel. [0]https://developer.gnome.org/hig/stable/empty-placeholder.html.en @@ +2208,3 @@ + + link = g_list_nth (dialog->original_name_listbox_rows, index); + gtk_widget_hide (link->data); why hide and not a destroy? do we want to reuse this somehow? is it not going to mess up the indexes in case we take the gtklistbox and iterate over it? (although of course we don't do it now, but still it's an inconsistent state) @@ +2362,3 @@ { NautilusBatchRenameDialog *dialog; + NautilusDirectory *dir; directory @@ +2381,3 @@ + for (l = directories_names; l != NULL; l = l->next) + { + dir = nautilus_directory_get_by_uri (l->data); I just realized, you forgot to add a monitor to track the files (you can look files-view for an example), if not it's not going to work (probably is going to work because currently we use the same files as present in the parent directories, which are probably already monitored, but it's coincidental).
*** Bug 771369 has been marked as a duplicate of this bug. ***
Marking as fixed, what is left is not a crash but rather an (nice) improvement
the case explained is bug 771369 is not yet fixed. Reopening
This one is fixed. The issue is that bug 771369 is marked as duplicated when it'snot.
My assertion about bug 771369 didn't convince you. I was saying like, Yes, it fails to rename. But it shouldn't segfault after the fail. Anyway, here is yet another case where nautilus segfaults. How to reproduce: 1. Create a folder mkdir test 2. Create some files inside the folder touch 1.txt 2.txt 3. Make the 'test' folder read only. chmod 455 test 4. Now in Nautilus select both 1.txt and 2.txt and batch rename to some other name. It won't, because the parent directory is read-only. 5. Now do Ctrl+Z. Nautilus now segfaults with the following error: ** ERROR:/home/sadiq/jhbuild/checkout/nautilus/src/nautilus-file.c:713:nautilus_file_get_internal: assertion failed: (location != NULL) Aborted (core dumped) Sorry for re-opening again. Thanks
(In reply to Mohammed Sadiq from comment #14) > My assertion about bug 771369 didn't convince you. I was saying like, Yes, > it fails to rename. But it shouldn't segfault after the fail. > > Anyway, here is yet another case where nautilus segfaults. > > How to reproduce: > > 1. Create a folder > mkdir test > 2. Create some files inside the folder > touch 1.txt 2.txt > 3. Make the 'test' folder read only. > chmod 455 test > 4. Now in Nautilus select both 1.txt and 2.txt and batch rename to some > other name. It won't, because the parent directory is read-only. > 5. Now do Ctrl+Z. > > Nautilus now segfaults with the following error: > ** > ERROR:/home/sadiq/jhbuild/checkout/nautilus/src/nautilus-file.c:713: > nautilus_file_get_internal: assertion failed: (location != NULL) > Aborted (core dumped) > > Sorry for re-opening again. > > Thanks That's a different bug (which actually I believe is broader than just batch rename). Please open a new bug report with that. This bug is about the case in comment 0, if that's fixed, this should remain closed and fixed.