Send tablet tool frame on proximity_out

[valpackett]

Tablet events, including proximity_out, are supposed to be grouped into frames. GTK will only "forget" a tool on the frame event after proximity_out. Not sending a frame event can cause a crash:

• have GTK >= 3.24.9
• launch a GTK app that likes to change cursors, e.g. gnome-terminal or tilix, on a HiDPI monitor
• interact with the app using a tablet
• start moving the mouse over the app
• GTK tries to reference the destroyed tablet tool, boom, crash in wl_proxy_marshal under gdk_window_set_cursor

(btw, I'm not sure about the send_tool_frame above, maybe having only this one is better?)

[ammen99]

According to the tablet-v2 protocol about frame events:

Marks the end of a series of axis and/or button updates from the
tablet. The Wayland protocol requires axis updates to be sent
sequentially, however all events within a frame should be considered
one hardware event.

So it seems that a frame event is not necessary for proximity events?

[valpackett]

The proximity_out description mentions the "same frame as proximity_out":

When the tablet tool leaves proximity of the tablet, button release events are sent for each button that was held down at the time of leaving proximity. These events are sent before the proximity_out event but within the same wp_tablet.frame.

I don't see a frame being required for proximity in the spec, but it doesn't say anything like "clients must handle proximity without a frame" either. GTK made the assumption that proximity is in a frame. That seems reasonable? Why would there be special unframed events?..

[Ongy]

Can you cross check this with a couple of non-gtk applications?
Looking at the code, I think I remember doing it this weird way because some things do crash when there's a frame after a proximity_out event
Also, don't send 2 events, if this does work on other applications, merge the two frames that might be sent here

[Ongy]

Looking at the spec (button events in same frame as proximity) we should send the frame event.
So the second part of my earlier message is relevant, don't send 2events

[emersion]

Using queue_tool_frame is probably enough to make sure we don't send two.

[valpackett]

nah, queue breaks stuff, e.g. alt-tabbing out of mypaint did this:

* thread #1, name = 'wayfire', stop reason = signal SIGSEGV
  * frame #0: 0x000000004070e7b0 libwayland-server.so.0`wl_list_remove(elm=0x00000000439b52d0) at wayland-util.c:55:18
    frame #1: 0x0000000040708540 libwayland-server.so.0`wl_event_source_remove(source=0x00000000439b52c0) at event-loop.c:486:2
    frame #2: 0x00000000407b3f84 libwlroots.so.3`destroy_tablet_tool_v2(resource=0x0000000049aecb00) at wlr_tablet_v2_tool.c:90:3
    frame #3: 0x00000000407033ec libwayland-server.so.0`destroy_resource(element=0x0000000049aecb00, data=0x0000000000000000, flags=0) at wayland-server.c:728:3
    frame #4: 0x00000000407032dc libwayland-server.so.0`wl_resource_destroy(resource=0x0000000049aecb00) at wayland-server.c:745:2
    frame #5: 0x00000000407b612c libwlroots.so.3`handle_tablet_tool_v2_set_cursor(client=0x0000000049abfbc0, resource=0x0000000049aecb00, serial=0, surface_resource=0x0000ffffffffd830, hotspot_x=0, hotspot_y=1236192000) at wlr_tablet_v2_tool.c:35:4
    frame #6: 0x00000000411363b4 libffi.so.6`ffi_call_SYSV + 100
    frame #7: 0x0000000041136624 libffi.so.6`ffi_call + 148
    frame #8: 0x000000004070c190 libwayland-server.so.0`wl_closure_invoke(closure=0x000000004808bc20, flags=2, target=0x0000000049aecb00, opcode=1, data=0x0000000049abfbc0) at connection.c:1006:2
    frame #9: 0x0000000040702ed0 libwayland-server.so.0`wl_client_connection_data(fd=69, mask=1, data=0x0000000049abfbc0) at wayland-server.c:439:4
    frame #10: 0x0000000040707a98 libwayland-server.so.0`wl_event_source_fd_dispatch(source=0x000000004284db80, ep=0x0000ffffffffdfe8) at event-loop.c:95:9
    frame #11: 0x00000000407089d0 libwayland-server.so.0`wl_event_loop_dispatch(loop=0x0000000042833b90, timeout=-1) at event-loop.c:641:4
    frame #12: 0x0000000040704448 libwayland-server.so.0`wl_display_run(display=0x0000000042843000) at wayland-server.c:1300:3
    frame #13: 0x00000000004418bc wayfire`main(argc=1, argv=0x0000ffffffffe608) at main.cpp:229:5
    frame #14: 0x00000000004400ac wayfire`__start(argc=1, argv=0x0000ffffffffe608, env=0x0000ffffffffe618, cleanup=<unavailable>) at crt1.c:84:7

[Ongy]

Queuing the frame event is intended to group events that come "at the same time" form libinput, since I'm not aware of something that groups properly from there.

It uses information about the currently surface that currently has proximity, which will be NULL after proximity_out.

Just cancel the event and sent the frame after proximity_out, that should be correct looking at the spec.
It was probably my own testing tool that segfaulted in that case and lead me astray.

[valpackett]

I'm not sure what "cancel the event" would entail here…

[ddevault]

bump

[valpackett]

@ddevault I'm not sure what exactly @Ongy meant here..

[valpackett]

Even with this patch, there's another wl_proxy_marshal crash: under gtk_drag_begin when trying to drag something in Firefox or Epiphany. (But not in e.g. Nautilus o_0) I guess there might be another place with a similar problem.

[jhalmen]

xournalpp does crash on me after a while as well without sending the frame event on proximity_out.
with this patch https://github.com/jhalmen/wlroots/commit/80149ad7db0087a39afa27fb6989245c55a0d4b8 i can't get it to crash anymore.
sometimes when using the buttons on the tool it seems to get stuck (it hangs), but it does catch up again when pressing a button on the pad.
i expect we might be missing another frame event somewhere

[chron-isch]

Is there an update on if/when this will be merged? Because it fixes my issues with GTK3/xournalpp while using a stylus/tablet tool on my Surface Go.

@jhalmen https://github.com/jhalmen/wlroots/commit/80149ad7db0087a39afa27fb6989245c55a0d4b8 leads to a segfault in wl_proxy_get_user_data.

[ddevault]

Thanks!