[PATCH weston] RDP compositor: make the seat dynamic and don't destroy it on removal

[Derek Foreman]

On 25/09/15 08:19 AM, Hardening wrote:
> Le 25/09/2015 11:31, Pekka Paalanen a écrit :
>> On Thu, 24 Sep 2015 23:40:26 +0200
>> David FORT <rdp.effort at gmail.com> wrote:
>>
>>> This patch makes the seat dynamic and leak it on purpose during seat removal to
>>> prevent the ghost object case.
>>> ---
>>>  src/compositor-rdp.c | 39 +++++++++++++++++++++++++++------------
>>>  1 file changed, 27 insertions(+), 12 deletions(-)
>>
>> Hi David,
>>
>> this patch is still missing the whole explanation of what is going on in
>> here. My questions from
>> http://lists.freedesktop.org/archives/wayland-devel/2015-May/022055.html
>> are still unanswered, and today I understand even less than then. :-)
>>
>> Or is there already a comment in the code explaining why rdp-backend
>> does funny stuff with seats? I couldn't find it on a quick look.
>>
>> Why leak it?
>> What is the ghost object problem?
>> Why you must use only part of weston_seat_release()?
>>
>> I have some very vague memories of wl_seat missing destructor protocol
>> or something, is this related?
> 
> 
> Hello Pekka,
> you're right I've not elaborated much.
> 
> So the general problem was that wl_seat doesn't have a release request,
> so we can't track the usage of a wl_seat by wayland clients. And as we
> can't track the usage, we can't release it safely or we take the risk
> that clients could address a released object. In the current situation
> we can't safely release a seat object.

Should we move forward with your patch to add seat release protocol first?

Does that change this implementation at all?

> So in my patch I'm mallocating a seat (not having it static with the RDP
> context), an I took the parts of weston_seat_release() that only do
> things internally to weston (so that I release as much as I can).
> 
> 
> Perhaps I'm not cleaning up correctly because with my patch applied, if I:
> * open a terminal;
> * connect with 2 RDP clients (so 2 seats);
> * take the focus in the terminal;
> * stop weston with a Ctrl-C
> 
> I get the following valgrind traceback during weston's shutdown, related
> to the input method:
> ^C[14:35:47.687] caught signal 2
> ==21909== Invalid read of size 8
> ==21909==    at 0x4173B9: unbind_input_method (text-backend.c:834)
> ==21909==    by 0x4E3D37B: destroy_resource (wayland-server.c:537)
> ==21909==    by 0x4E42255: for_each_helper.isra.0 (wayland-util.c:359)
> ==21909==    by 0x4E4279E: wl_map_for_each (wayland-util.c:365)
> ==21909==    by 0x4E3DF57: wl_client_destroy (wayland-server.c:675)
> ==21909==    by 0x418401: text_backend_destroy (text-backend.c:1046)
> ==21909==    by 0x93542F2: shell_destroy (shell.c:6473)
> ==21909==    by 0x4109C6: weston_compositor_destroy
> (wayland-server-core.h:264)
> ==21909==    by 0x40897D: main (main.c:829)
> ==21909==  Address 0x9b9e6f0 is 112 bytes inside a block of size 120 free'd
> ==21909==    at 0x4C2BDEC: free (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==21909==    by 0x6A468AE: rdp_peer_context_free (wayland-server-core.h:264)
> ==21909==    by 0x6D520DA: freerdp_peer_context_free (peer.c:738)
> ==21909==    by 0x6A469B7: rdp_client_activity (compositor-rdp.c:674)
> ==21909==    by 0x4E3FE51: wl_event_loop_dispatch (event-loop.c:422)
> ==21909==    by 0x4E3E6B4: wl_display_run (wayland-server.c:1004)
> ==21909==    by 0x408D35: main (main.c:818)
> ==21909==
> 
> 
> Anyway without the patch, valgrind complains during seat releasing (for
> example when a RDP peer disconnects).

Sigh.  There's a lot of multi-seat related problems in the
text-backend/input-method bits.  I've got a bunch of patches I need to
sort through and post. :/

I don't think you've added this problem, I think it's already there (and
I may have fixed it already...)