Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi-finger swipe on touchscreen crashes wayfire #2176

Closed
mark-herbert42 opened this issue Mar 5, 2024 · 20 comments · Fixed by #2187
Closed

Multi-finger swipe on touchscreen crashes wayfire #2176

mark-herbert42 opened this issue Mar 5, 2024 · 20 comments · Fixed by #2187
Labels
Milestone

Comments

@mark-herbert42
Copy link

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/

@ammen99 ammen99 added this to the 0.8.1 milestone Mar 5, 2024
@mark-herbert42
Copy link
Author

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
Copy link
Member

ammen99 commented Mar 7, 2024

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

@mark-herbert42
Copy link
Author

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

@ammen99
Copy link
Member

ammen99 commented Mar 7, 2024

@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
Copy link
Author

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
Copy link
Member

soreau commented Mar 7, 2024

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
Copy link
Member

ammen99 commented Mar 7, 2024

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
Copy link
Author

gdb_log was backtrace from coredump.

@mark-herbert42
Copy link
Author

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

@mark-herbert42
Copy link
Author

wayfire.log.gz

More variables

@mark-herbert42
Copy link
Author

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
Copy link
Member

soreau commented Mar 8, 2024

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
Copy link
Author

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
Copy link
Author

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

auto target = wf::get_core().output_layout->get_output_coords_at(gc, gc);

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

@soreau
Copy link
Member

soreau commented Mar 8, 2024

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
Copy link
Member

soreau commented Mar 8, 2024

Aside from that hunch, does this patch help?

@mark-herbert42
Copy link
Author

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
Copy link
Member

ammen99 commented Mar 8, 2024

@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.

ammen99 added a commit that referenced this issue Mar 8, 2024
Otherwise, we may get NaN as its position from core, which causes
subsequent problems.

Fixes #2176.
@mark-herbert42
Copy link
Author

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
Copy link
Member

ammen99 commented Mar 8, 2024

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.

ammen99 added a commit that referenced this issue Mar 8, 2024
Otherwise, we may get NaN as its position from core, which causes
subsequent problems.

Fixes #2176.
ammen99 added a commit that referenced this issue Mar 13, 2024
Otherwise, we may get NaN as its position from core, which causes
subsequent problems.

Fixes #2176.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants