GNOME Bugzilla – Bug 778588
gnome-todo segfaults when selecting ToDo.txt file
Last modified: 2017-04-24 10:45:29 UTC
How to reproduce: 1. Enable todo.txt plugin. 2. Open settings to todo.txt -> Select some empty Todo.txt file Result: gnome-todo segfaults. I believe that an empty Todo.txt corresponds to not having any events added. So empty files should be accepted without segfault (the segfault may be happening for some different reason, but this can be one cause). Thanks
Here is the backtrace: (gdb) bt full
+ Trace 237146
Thanks
(In reply to Mohammed Sadiq from comment #0) > gnome-todo segfaults. I believe that an empty Todo.txt corresponds to not > having any events added. So empty files should be accepted without segfault > (the segfault may be happening for some different reason, but this can be > one cause). > > Thanks The segfaults it's produced because it's use a g_free() in a object derived from GObject, instead of use g_clear_object() or g_object_unref(). > having any events added. So empty files should be accepted without segfault > (the segfault may be happening for some different reason, but this can be > one cause). I'm not the official developer but as I can see, trying to solve the segfault, an Todo.txt file needs to have a context (eg "@foo"), this context is used to set the title of the todo list. So It is not possible to load an empty file. Anyway I think that will be good, add a dialog to allow user enter a context to an empty file.
Created attachment 347280 [details] [review] Patch Selecting empty file,causes a notification dialog that informs to the user that he selected an empty file.
Created attachment 347281 [details] [review] Patch to segfault I forgot free the strings.
Review of attachment 347281 [details] [review]: Thanks for the patch! Besides the specific changes below, I'd like to ask you to split the GtdPluginDialog work in a separate patch. ::: src/gtd-plugin-dialog.c @@ +49,3 @@ +static gboolean +delete_widget (GtkWidget *dialog, Rename to 'dialog_deleted'. Also, instead of "GtkWidget *dialog", you can declare "GtdPluginDialog *self" and simplify the code below. @@ +55,3 @@ + gtk_stack_set_transition_type (GTK_STACK (GTD_PLUGIN_DIALOG(dialog)->stack), GTK_STACK_TRANSITION_TYPE_NONE); + gtk_stack_set_visible_child_name (GTK_STACK (GTD_PLUGIN_DIALOG(dialog)->stack), "list"); + gtk_widget_hide (GTD_PLUGIN_DIALOG(dialog)->back_button); After declaring "GtdPluginDialog *self", you don't need all those GTD_PLUGIN_DIALOG() casts. @@ +58,3 @@ + gtk_widget_hide (dialog); + +{ Use GDK_EVENT_STOP instead of TRUE.
Created attachment 347283 [details] [review] Patch to the segfault
Created attachment 347284 [details] [review] Patch to gtd-plugin-dialog Hope this time the patch is correct. Regards!
Review of attachment 347283 [details] [review]: One more question ::: plugins/todo-txt/gtd-provider-todo-txt.c @@ +360,3 @@ + g_free (msg); + g_free (name); + } Now that I'm looking at it, I believe we must support empty files. What's exactly wrong with it being empty?
Review of attachment 347284 [details] [review]: Looks good.
Comment on attachment 347284 [details] [review] Patch to gtd-plugin-dialog Commited patch for GtdPluginDialog. Waiting for the segfault to be reviewed.
Created attachment 347898 [details] [review] Patch to the segfault (In reply to Georges Basile Stavracas Neto from comment #10) > Commited patch for GtdPluginDialog. Waiting for the segfault to be reviewed. Hope it is ok. Regards!
Created attachment 348343 [details] [review] Patch to segfault
Review of attachment 348343 [details] [review]: Some comments ::: plugins/todo-txt/gtd-plugin-todo-txt.c @@ +67,3 @@ + GFile *source_file; + GFileOutputStream *output; + gchar *buffer; Sort this by the size of the type, structures coming before native types. Should be like: GFileOutputStream GFile gchar @@ +76,3 @@ + NULL); + + buffer = g_strconcat ("@",context,NULL); Space after commas @@ +78,3 @@ + buffer = g_strconcat ("@",context,NULL); + + g_output_stream_write (G_OUTPUT_STREAM (output), buffer, strlen(buffer)*sizeof(gchar), NULL, NULL); One line per parameter, and spaces around the '*' @@ +109,3 @@ + GTK_RESPONSE_OK, + TRUE); + } You can write it like: gtk_dialog_set_response_sensitive (dialog, GTK_RESPONSE_OK, text_length > 0); @@ +114,3 @@ + +static void +show_context_dialog (GtdPluginTodoTxt *self, GtdProviderTodoTxt *provider) One parameter per line. @@ +121,3 @@ + const gchar *context; + + /**Hardcode to obtain the main window **/ Wrong comment style (should be '/* ... */') @@ +137,3 @@ + gtk_widget_set_size_request (dialog, 400, 300); + + label = gtk_label_new ("You selected an empty file without a context.\nPlease add a context to the Todo.txt file."); This label should be translatable. @@ +405,3 @@ + show_context_dialog (self, provider); + } + Don't need the brackets (one-line if()s don't need them) @@ +429,2 @@ g_settings_set_string (self->settings, + "file", What was the change here? ::: plugins/todo-txt/gtd-provider-todo-txt.c @@ +1139,3 @@ + G_MAXINT, + 0, + G_PARAM_READABLE)); This should be an unsigned integer (we don't support negative number of lines, right?)
Created attachment 348518 [details] [review] Pacth Thanks for the review. Regards!
Review of attachment 348518 [details] [review]: Now that I'm testing the patch, I don't really understand the necessity to show a dialog. Can't the user simply add a new task list?
(In reply to Georges Basile Stavracas Neto from comment #15) > Review of attachment 348518 [details] [review] [review]: > > Now that I'm testing the patch, I don't really understand the necessity to > show a dialog. Can't the user simply add a new task list? In my first approach, Gnome Todo only shows a dialog that warns to the user that add a Todo Txt empty file is not allowed. And you said in the comment 8 that we have to support add empty files. The todo txt plugin sets as name of the list, the task's context, so If we have to support empty files, a context is necessary be writen in the source file, which is the purpose of the dialog. Personally I prefer the first approach. Regards !
Hi, as far as i can test on my pc we can select empty files, add lists and tasks as with any other todo.txt files. The only problem is when the a line inside the todo.txt describing the task doesn't contain @list-name tag, we ignore such tasks and send user a notification.
(In reply to Rohit Kaushik from comment #17) > Hi, as far as i can test on my pc we can select empty files, add lists and > tasks as with any other todo.txt files. The only problem is when the a line > inside the todo.txt describing the task doesn't contain @list-name tag, we > ignore such tasks and send user a notification. Hi! Please se the video in the link below. The video is so big so I can't uploaded as attachment. http://kevlopez.com/files/Segfault.webm Regards!
Hey, thanks for uploading the video. Can you obtain the backtrace after you get segfault and upload it .
(In reply to Rohit Kaushik from comment #19) > Hey, thanks for uploading the video. Can you obtain the backtrace after you > get segfault and upload it . The backtrace is in the comment 1 and the explanation of why happens is in the comment 3. Regards !
(In reply to Kevin Lopez from comment #20) > (In reply to Rohit Kaushik from comment #19) > > Hey, thanks for uploading the video. Can you obtain the backtrace after you > > get segfault and upload it . > > The backtrace is in the comment 1 and the explanation of why happens is in > the comment 3. > > Regards ! Correction: the explanation is in the comment 2.
Review of attachment 348518 [details] [review]: I'll mark this patch as 'rejected', for it is fixed in master already. Thanks for the efforts.
I can't reproduce this issue anymore. Besides that, since Todo.txt is not enabled by default anymore, this is not a blocker bug. Closing as RESOLVED FIXED, and lowering the priority.