WayfireWM / wayfire

A modular and extensible wayland compositor
https://wayfire.org/
MIT License
2.41k stars 179 forks source link

Multi-finger swipe on touchscreen crashes wayfire #2176

Closed mark-herbert42 closed 8 months ago

mark-herbert42 commented 8 months ago

Happens with git version - 0.8 is OK. Disabling viewport-swipe plugin does not help, still crashes.

gdb_log.txt

It happens allways when multi-finger swipe happens on desktop. If the gesture is captured by application like firefox f/e - everything perfect. But If fingers out of application window - crash/

mark-herbert42 commented 8 months ago

I've found the cause of issue

Just to avoid crash it is enough to comment assert(handle || is_shutting_down()); in src/core/output-layout.cpp line 1670. But the multifinger swipes will not work.

The real thing is here

https://gitlab.freedesktop.org/wlroots/wlroots/-/commit/843b874f22b87140a04bfc279852e025967c055e

Reverting this commit from wlroots makes finger swipes on desktop working again. I do not think that this is a solution - the real solution is to adopt wlroots changes to be handled from wayfire code correctly.

ammen99 commented 8 months ago

Yeah, definitely. But I wonder how come the output layout is empty - it shouldn't be.

mark-herbert42 commented 8 months ago

It happens only when I swipe on the desktop itself. When I swipe on any window over this desktop - no issues at al.

ammen99 commented 8 months ago

@mark-herbert42 I looked through the relevant places indicated in the backtrace but I cannot find the error just by looking, and unfortunately I also could not reproduce it easily on my machine - neither with an automatic test nor by testing with an actual touchscreen on my own 2-in-1 laptop.

Soo I have the following request: could you compile wayfire in debug mode (meson --buildtype=debug) and then when you start Wayfire with GDB (judging by the log, you already know how to do that), take a look at what values we are passing to the wlroots function. I have the suspicion that we might be feeding NaN or infinite values, which would cause the bug (but then again the question is, where do these values come from? Are they part of the wlroots event?).

Also please attach the whole Wayfire log.

It could also be a memory corruption problem, in which case you should try running with address sanitizer as well.

mark-herbert42 commented 8 months ago

I could not run it in gdb - instead of crash it made a dead hang when I could not return to gdb.

And as for the Wayfire log - where to find it? I've reverted all patches preventing crash - so i start wayfire, swipe, it crashes. How to find the logs?

soreau commented 8 months ago

I could not run it in gdb - instead of crash it made a dead hang when I could not return to gdb.

And as for the Wayfire log - where to find it? I've reverted all patches preventing crash - so i start wayfire, swipe, it crashes. How to find the logs?

Wayfire logs messages to stdout. You should redirect the output to file, something like wayfire -d -d wlroots -c /tmp/wayfire.ini &> /tmp/wayfire.log.

ammen99 commented 8 months ago

I could not run it in gdb - instead of crash it made a dead hang when I could not return to gdb.

So how did you obtain the file gdb_log.txt from your report?

And as for the Wayfire log - where to find it? I've reverted all patches preventing crash - so i start wayfire, swipe, it crashes. How to find the logs?

The log is directly printed on stdout if you run wayfire (you can redirect it to a file with wayfire &> log.txt, if you use startwayfire from wf-install it redirects to a file, ~/.local/share/wayfire/wayfire.log

mark-herbert42 commented 8 months ago

gdb_log was backtrace from coredump.

mark-herbert42 commented 8 months ago

gdb_log was backtrace from coredump.
wayfire.log.gz I've put additional LOGI to get the parameters of the failig function.

mark-herbert42 commented 8 months ago

wayfire.log.gz

More variables

mark-herbert42 commented 8 months ago

And some more. Buggy call happens from hotspot-manager. So seems that it is not wlroots bug, but wayfire bug. Wlroots just had some tolerance to the buggy call until 0.17 and could ignore the call for empty coordinate parameters. But now the wlroots function is "optimized"(according to wlroots commit description) so now the caller have to take care about data sanity.

wayfire.log.gz

soreau commented 8 months ago

It seems to me, that the problem is coming from this function. Apparently the wlr_box mapping struct in that function contains nan in multiple if not all members. Are you able to verify that the function in question leaves you with lx = nan and ly = nan?

mark-herbert42 commented 8 months ago

Got some more understanding of it. Actually the tochscreen swipe functionality is broken. It got translated into hotspot top_left (coords 0 0).
So when I use my patched wayfire with swipe workspace on for 2 fingers then it works strange. I swipe - instead of getting to the next workspace in the direction of swipe it opens me "expo" screen. I was thinking that this is a new way of swipe workspace working - but suddenly I realized that I have Expo plugin activated by top-left. So I switched it off - and now my 2 fingers swipe is absolutely ignored. But - extended gestures for 3 finger move works - if I put 3 fingers on window and wait (not moving fingers) window goes into mover mode and then I can move with touchscreen.

Swipe is just moving cursor to top-left, no ide why and how.

mark-herbert42 commented 8 months ago

It seems to me, that the problem is coming from this function. Apparently the wlr_box mapping struct in that function contains nan in multiple if not all members. Are you able to verify that the function in question leaves you with lx = nan and ly = nan?

No - here I receive nans

https://github.com/WayfireWM/wayfire/blob/80569f47cf71c6dc72fbcb840c1bbf806a37c998/src/core/seat/hotspot-manager.cpp#L14

In my latest log with extra logging points it becomes clear. Touch swipes seems to be completely broken, at least on my hardware.

soreau commented 8 months ago

Right, but something has to call that function with nan's. It seems that it's here which ends up here. invalid_coordinate is defined as nan. So.. perhaps increasing this to however many fingers you're using might help workaround the bug.

soreau commented 8 months ago

Aside from that hunch, does this patch help?

mark-herbert42 commented 8 months ago

The patch works fine - no more crashes. And now multifinger swipe does not generate top-left hot corner.

Vieport swipe is not working at all , but it is better than crash. And I guess that's a different story.

ammen99 commented 8 months ago

@mark-herbert42 @soreau Thanks for debugging this, I figured out the problem. Hotspots unconditionally poll the position of touch point 0, but the touch point might not exist, in which case, we get a NaN from core.

The following test is enough to crash Wayfire (with a single configured hotspot):

    def _run(self):
        self.socket.set_touch(1, 10, 10)
        self.socket.set_touch(1, 10, 20)
        return wt.Status.OK, None

I'll push a fix shortly.

mark-herbert42 commented 8 months ago

Yes - the patch fix it. So the issue is done. But then it will be new issue - viewport swipe does not work.

Viewport swipe was not causing the crash as the system was crashing with viewport swipe disabled. Now there is no crash, and no dirty hacks. So when I set default 4 fingers (and all other default values) - nothing happens. When I use extra touchscreen actions and put my 3 fingers on window - I get the move move activated and can move window using touchscreen. So in hardware it works, feels the movement and correctly counts fingers number.

But vieport swipe does not activate.

ammen99 commented 8 months ago

The vswipe plugin only supports touchpad gestures, not touchscreen gestures. I suppose the code could be extended but nobody has written the code yet.

In the meantime, you can set swipe left/right 3 as an additional activator in the vswitch plugin (the one usually used with keybindings) - that's what I use.