Re: fixed bug in wl_list
In my opinion the problem is this: The code to remove the element from the list is called every time, and the code to insert it is called only when 'surface'. So in case surface is NULL the item won't be added, but will be removed next time the function is entered. Isn't that the case? I don't have a detailed understanding of this exact code, but isn't the explanation as simple as that? Should I add some debugging info to see if this is indeed the case, or you are sure that the problem lies somewhere else? About the 'tagging' -- are you proposing to add a boolean flag to the listener to explicitly mark it as 'in a list' or 'out of a list'? Regards, Iskren On Sat, Mar 12, 2011 at 9:48 PM, Marty Jack marty...@comcast.net wrote: On 03/12/2011 02:20 PM, Bill Spitzak wrote: On 03/12/2011 04:28 AM, Marty Jack wrote: I have never encountered a system where it was believed to be desirable to allow something to be removed twice. It is important to keep data structures clean. If anything you would be more likely to see a debugging mode where the lists were fully checked after every insert or remove to make sure they are internally consistent, especially if they are important to keeping the system running. It's not that much different from memory allocation. A block is allocated, or it is free, and a double free is a bug. Making it crash at the moment the second remove is attempted is better than it leaving the data corrupted and crashing later. It makes it a lot easier to find out why it went wrong. I think that is what the newest version of the patch is doing, right? ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel Oh and no, the second version of the patch makes the second remove a no-op. ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: fixed bug in wl_list
On Fri, Mar 11, 2011 at 7:32 PM, Iskren Chernev
iskren.cher...@gmail.com wrote:
Hello,
I found a bug and fixed it with the patch :)
to reproduce:
Hi,
I wasn't able to reproduce it immediately, but I know that there's a
crasher in there somewhere. I think the real fix is the patch I've
attached, could you give it a try and see if it fixes it for you?
thanks,
Kristian
run compositor on top of x11
repeat
run flower
drag drop it a little
move the pointer in and out of the compositor/flower
Ctrl+C the flower client
it would break eventually
problem:
I found that the linked list surface-destroy_listener_list got corrupted at
some point (it was not circular any more, strange next/prev etc), which
causes the crash.
solution:
The problem was in wl_list_remove -- when you erase an element, you don't
mark it as 'erased', by setting prev/next to NULL for example. Then if you
erase it again the list becomes corrupt. I nullified the prev/next and check
in the begining of wl_list_remove for not-in-list elements and just ignore
them. That seems to fix it.
Regards,
Iskren
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
From 249c42a153e94e12e6734465e94d310dcf2421da Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kristian=20H=C3=B8gsberg?= k...@bitplanet.net
Date: Sat, 12 Mar 2011 21:26:21 -0500
Subject: [PATCH] Fix double remove from surface destroy_listener_list
We remove the listener when a device loses its pointer focus, but doesn't
insert it in another destroy_listener list if surface is NULL. Tracked
down by Iskren Chernev.
---
wayland/wayland-server.c | 24 +---
1 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/wayland/wayland-server.c b/wayland/wayland-server.c
index dece0d1..c505e32 100644
--- a/wayland/wayland-server.c
+++ b/wayland/wayland-server.c
@@ -356,19 +356,21 @@ wl_input_device_set_pointer_focus(struct wl_input_device *device,
device-object,
WL_INPUT_DEVICE_POINTER_FOCUS,
time, NULL, 0, 0, 0, 0);
- if (surface)
+ if (device-pointer_focus)
+ wl_list_remove(device-pointer_focus_listener.link);
+
+ if (surface) {
wl_client_post_event(surface-client,
device-object,
WL_INPUT_DEVICE_POINTER_FOCUS,
time, surface, x, y, sx, sy);
+ wl_list_insert(surface-destroy_listener_list.prev,
+ device-pointer_focus_listener.link);
+ }
device-pointer_focus = surface;
device-pointer_focus_time = time;
- wl_list_remove(device-pointer_focus_listener.link);
- if (surface)
- wl_list_insert(surface-destroy_listener_list.prev,
- device-pointer_focus_listener.link);
}
WL_EXPORT void
@@ -385,20 +387,20 @@ wl_input_device_set_keyboard_focus(struct wl_input_device *device,
device-object,
WL_INPUT_DEVICE_KEYBOARD_FOCUS,
time, NULL, device-keys);
+ if (device-keyboard_focus)
+ wl_list_remove(device-keyboard_focus_listener.link);
- if (surface)
+ if (surface) {
wl_client_post_event(surface-client,
device-object,
WL_INPUT_DEVICE_KEYBOARD_FOCUS,
time, surface, device-keys);
+ wl_list_insert(surface-destroy_listener_list.prev,
+ device-keyboard_focus_listener.link);
+ }
device-keyboard_focus = surface;
device-keyboard_focus_time = time;
-
- wl_list_remove(device-keyboard_focus_listener.link);
- if (surface)
- wl_list_insert(surface-destroy_listener_list.prev,
- device-keyboard_focus_listener.link);
}
WL_EXPORT void
--
1.7.4.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: fixed bug in wl_list
Well, with your patch it doesn't seem to crash. There is also no list corruption, as far as I could test it :) Regards, Iskren 2011/3/13 Kristian Høgsberg k...@bitplanet.net On Fri, Mar 11, 2011 at 7:32 PM, Iskren Chernev iskren.cher...@gmail.com wrote: Hello, I found a bug and fixed it with the patch :) to reproduce: Hi, I wasn't able to reproduce it immediately, but I know that there's a crasher in there somewhere. I think the real fix is the patch I've attached, could you give it a try and see if it fixes it for you? thanks, Kristian run compositor on top of x11 repeat run flower drag drop it a little move the pointer in and out of the compositor/flower Ctrl+C the flower client it would break eventually problem: I found that the linked list surface-destroy_listener_list got corrupted at some point (it was not circular any more, strange next/prev etc), which causes the crash. solution: The problem was in wl_list_remove -- when you erase an element, you don't mark it as 'erased', by setting prev/next to NULL for example. Then if you erase it again the list becomes corrupt. I nullified the prev/next and check in the begining of wl_list_remove for not-in-list elements and just ignore them. That seems to fix it. Regards, Iskren ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: fixed bug in wl_list
2011/3/12 Iskren Chernev iskren.cher...@gmail.com: Well, with your patch it doesn't seem to crash. There is also no list corruption, as far as I could test it :) Excellent, thanks for tracking this down. Kristian Regards, Iskren 2011/3/13 Kristian Høgsberg k...@bitplanet.net On Fri, Mar 11, 2011 at 7:32 PM, Iskren Chernev iskren.cher...@gmail.com wrote: Hello, I found a bug and fixed it with the patch :) to reproduce: Hi, I wasn't able to reproduce it immediately, but I know that there's a crasher in there somewhere. I think the real fix is the patch I've attached, could you give it a try and see if it fixes it for you? thanks, Kristian run compositor on top of x11 repeat run flower drag drop it a little move the pointer in and out of the compositor/flower Ctrl+C the flower client it would break eventually problem: I found that the linked list surface-destroy_listener_list got corrupted at some point (it was not circular any more, strange next/prev etc), which causes the crash. solution: The problem was in wl_list_remove -- when you erase an element, you don't mark it as 'erased', by setting prev/next to NULL for example. Then if you erase it again the list becomes corrupt. I nullified the prev/next and check in the begining of wl_list_remove for not-in-list elements and just ignore them. That seems to fix it. Regards, Iskren ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel
