lightdash: Adding bookmarks buttons

[Matthew Brush]

On 2015-11-20 11:12 AM, adlo wrote:
>> On 20 Nov 2015, at 02:49, Matthew Brush <mbrush at codebrainz.ca> wrote:
>>
>> You should provide a link to the malfunctioning code so people can see what you're doing (wrong). With the information you've given, it would be really hard to help you, at least not without sifting through thousands of lines of C code, guessing what lines you might be asking about, assuming you even have the code pushed to your repo.
>
> You may have a point there; I assumed that the content of my last commit to that branch would provide enough information.
>
> Here is the code where the bookmarks-changed signal is emitted:
> https://github.com/adlocode/xfce4-lightdash-plugin/blob/bookmarks-buttons/src/appfinder-model.c#L2461
>
> Here is the code that handles bookmarks-changed events:
> https://github.com/adlocode/xfce4-lightdash-plugin/blob/bookmarks-buttons/src/appfinder-window.c#L288
>
> Does anyone have any ideas?
>

At a quick glance, it might be because you're mutating the linked list 
during traversal. Usually doing that requires a little finesse to ensure 
you're not accessing dangling pointers.

One way for this[0] loop (untested):


     GList *li = window->bookmarks_buttons;
     while (li != NULL)
     {
         GList *next = li->next; // save next pointer
         GtkWidget *button = li->data;
         if (button != NULL) // conditionally mutate list
         {
             window->bookmarks_buttons =
                 g_list_delete_link(window->bookmarks_buttons, li);
             gtk_widget_destroy(button);
         }
         li = next; // move to next element
     }

Not sure that's 100% correct but it gives the idea. There's a number of 
ways to code such a loop as well. For this specific case you could 
probably also just do:

     g_list_free_full(window->bookmarks_buttons, gtk_widget_destroy);
     window->bookmarks_buttons = NULL;

Assuming you just want to delete everything in the list.

For the second loop[1], assuming `lightdash_model_get_items()` transfers 
ownership of the list to the caller, you probably want to stash a 
pointer to the head of that list, since you need to pass it to 
`g_slist_free()` at line 323, as opposed to passing it `NULL` (what 
`sli` will contain once that loop is finished).

Maybe like this:

     GSList *items = lightdash_get_items(model);
     for (GSList sli = items; sli != NULL; sli = sli->next)
     {
         // careful about mutating list here too !
     }
     g_slist_free(items);

One good tip for such types of bugs is to run the code in Valgrind 
(though it's hard since GLib/GTK+ causes many "false positives" noise in 
the output). It will tell you when you're accessing memory out of 
bounds, double-freeing, accessing already freed memory, etc.

Cheers,
Matthew Brush

[0]: 
https://github.com/adlocode/xfce4-lightdash-plugin/blob/bookmarks-buttons/src/appfinder-window.c#L297
[1]: 
https://github.com/adlocode/xfce4-lightdash-plugin/blob/bookmarks-buttons/src/appfinder-window.c#L308