Bug 100878

I don't have any reproduction steps, it was out-of-the blue as I was playing with weston-editor. However, weston itself came crashing down

The clue, however, is in the names of the parameters to the functions. For instance, you do:
    struct wl_list list;
    struct {
        int something;
        struct wl_list link;
    } foo;

    wl_list_init(&list);
    wl_list_insert(&list, &foo->link);
    wl_list_remove(&foo->link);

At this point, foo has been removed from list (making its link member dangling), but the overall list itself is still valid. Trying to remove a list from itself does not make any real sense; instead, you remove an element from that list.

(In reply to Daniel Stone from comment #2)
> The clue, however, is in the names of the parameters to the functions. For
> instance, you do:
>     struct wl_list list;
>     struct {
>         int something;
>         struct wl_list link;
>     } foo;
> 
>     wl_list_init(&list);
>     wl_list_insert(&list, &foo->link);
>     wl_list_remove(&foo->link);
> 
> At this point, foo has been removed from list (making its link member
> dangling), but the overall list itself is still valid. Trying to remove a
> list from itself does not make any real sense; instead, you remove an
> element from that list.

ah, I see! So, does that mean the list itself is represented as an always-present dummy-node that resides in the list?

However, it looks like the crash occurred as a result of es->destroy_signal.listener_list.prev being NULL. Somehow, this list ceased to be circular (at least while traversing backwards)

(In reply to worknesday from comment #0)
> The problem here is that the *identity* of the list is subject to change as
> you add/remove nodes -- especially wl_list_remove. If, say, you have a list
> 
>     struct wl_list *my_list; // initialized to some list with 3 elements
> 
> then you do
> 
>     wl_list_remove(my_list)

This is stricty in the land of "don't do that". You are removing the list's head from the circular list. It is a very handy trick but rarely useful: it's purpose is to be able to free() the list head element while leaving all the nodes still valid for a wl_list_remove() call later. It is no longer possible to iterate the list, so this is only useful in destruction paths.

> I think a better (robust) design is to wrap and hide the actual node
> pointers so that
>     1. we preserve the identity (pointer) of the list itself
>     2. we can call wl_list_remove multiple times

The current list API may not be very intuitive at first, but it probably cannot be changed in libwayland at least due to stable ABI.

> Then, removing a node would be something like
> 
>     // assert that node is an element of list
>     void
>     wl_list_remove(struct wl_list *list, struct wl_list_node *node);

That is inconvenient. We use a lot the feature of wl_list_remove() that you do not need to know in which list the element is in to just remove it. If we had to look up the identity of the list to remove the node from it, that would lead into a lot of new code for not much benefit. For instance, all object destroy functions can currently simply wl_list_remove() without regard to which list the node is in, as long as the node is initialized (does not even need to be in a list really).

(In reply to worknesday from comment #3)
> ah, I see! So, does that mean the list itself is represented as an
> always-present dummy-node that resides in the list?

Yes, exactly. The list head is always part of the circular list, but the head is not a list node - dereferencing the head as a node would produce a garbage pointer. This is a design choice. To iterate a list, one always needs the list head. A list cannot be iterated starting from an arbitrary node without knowing the head, as one must never dereference the head.

> However, it looks like the crash occurred as a result of
> es->destroy_signal.listener_list.prev being NULL. Somehow, this list ceased
> to be circular (at least while traversing backwards)

Yes, that is a side-effect of list corruption. Something else is corrupting the list, and you happen to see a crash in a list manipulation function. Another very common effect from list corruption is an endless loop.


Since this report is full of discussion about wl_list design rather than an actual bug, I would suggest to rename this bug, close it, and open a new one concentrating on the actual crash.

close as suggested by Pekka