Bug 389674 - kwin_wayland crashes on startup in KWin::TabletModeInputEventSpy::switchEvent
Summary: kwin_wayland crashes on startup in KWin::TabletModeInputEventSpy::switchEvent
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Plasma
Component: libinput (other bugs)
Version First Reported In: 5.11.95
Platform: openSUSE Linux
: NOR crash
Target Milestone: ---
Assignee: KWin default assignee
URL: https://phabricator.kde.org/D10234
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-31 07:58 UTC by Fabian Vogt
Modified: 2018-02-03 15:03 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In: 5.12.0
Sentry Crash Report:
mgraesslin: Wayland+
mgraesslin: X11-
fabian: ReviewRequest+


Attachments
libinput list-devices (8.82 KB, text/plain)
2018-02-01 14:30 UTC, Fabian Vogt
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Fabian Vogt 2018-01-31 07:58:28 UTC
After upgrading to the 5.12 beta, I can no longer start kwin_wayland on my laptop if it's docked (+ 2 external monitors, mouse and keyboard).

Thread 1 "kwin_wayland" received signal SIGSEGV, Segmentation fault.
0x000055555555eb44 in KWin::TabletModeInputEventSpy::switchEvent (this=0x5555557f8250, event=0x7fffffffd650)
    at /usr/src/debug/kwin5-5.11.95-1.1.x86_64/tabletmodemanager.cpp:54
54          if (!event->device()->isTabletModeSwitch()) {
#0  0x000055555555eb44 in KWin::TabletModeInputEventSpy::switchEvent(KWin::SwitchEvent*) (this=0x5555557f8250, event=0x7fffffffd650)
    at /usr/src/debug/kwin5-5.11.95-1.1.x86_64/tabletmodemanager.cpp:54
#1  0x00007ffff79be19b in std::__invoke_impl<void, void (KWin::InputEventSpy::*&)(KWin::SwitchEvent*), KWin::InputEventSpy* const&, KWin::SwitchEvent*&>(std::__invoke_memfun_deref, void (KWin::InputEventSpy::*&)(KWin::SwitchEvent*), KWin::InputEventSpy* const&, KWin::SwitchEvent*&) (__t=<optimized out>, __f=<optimized out>)
    at /usr/include/c++/7/bits/invoke.h:73
#2  0x00007ffff79be19b in std::__invoke<void (KWin::InputEventSpy::*&)(KWin::SwitchEvent*), KWin::InputEventSpy* const&, KWin::SwitchEvent*&>(void (KWin::InputEventSpy::*&)(KWin::SwitchEvent*), KWin::InputEventSpy* const&, KWin::SwitchEvent*&) (__fn=<optimized out>) at /usr/include/c++/7/bits/invoke.h:95
#3  0x00007ffff79be19b in std::_Bind<void (KWin::InputEventSpy::*(std::_Placeholder<1>, KWin::SwitchEvent*))(KWin::SwitchEvent*)>::__call<void, KWin::InputEventSpy* const&, 0ul, 1ul>(std::tuple<KWin::InputEventSpy* const&>&&, std::_Index_tuple<0ul, 1ul>) (__args=..., this=<optimized out>) at /usr/include/c++/7/functional:467
#4  0x00007ffff79be19b in std::_Bind<void (KWin::InputEventSpy::*(std::_Placeholder<1>, KWin::SwitchEvent*))(KWin::SwitchEvent*)>::operator()<KWin::InputEventSpy* const&, void>(KWin::InputEventSpy* const&) (this=<optimized out>) at /usr/include/c++/7/functional:551
#5  0x00007ffff79be19b in std::for_each<KWin::InputEventSpy* const*, std::_Bind<void (KWin::InputEventSpy::*(std::_Placeholder<1>, KWin::SwitchEvent*))(KWin::SwitchEvent*)> >(KWin::InputEventSpy* const*, KWin::InputEventSpy* const*, std::_Bind<void (KWin::InputEventSpy::*(std::_Placeholder<1>, KWin::SwitchEvent*))(KWin::SwitchEvent*)>) (__f=..., __last=0x55555582d570, __first=0x55555582d568) at /usr/include/c++/7/bits/stl_algo.h:3884
#6  0x00007ffff79be19b in KWin::InputRedirection::processSpies<std::_Bind<void (KWin::InputEventSpy::*(std::_Placeholder<1>, KWin::SwitchEvent*))(KWin::SwitchEvent*)> >(std::_Bind<void (KWin::InputEventSpy::*(std::_Placeholder<1>, KWin::SwitchEvent*))(KWin::SwitchEvent*)>) (function=..., this=<optimized out>)
    at /usr/src/debug/kwin5-5.11.95-1.1.x86_64/input.h:207
#7  0x00007ffff79be19b in KWin::InputRedirection::<lambda(KWin::SwitchEvent::State, quint32, quint64, KWin::LibInput::Device*)>::operator() (device=<optimized out>, timeMicroseconds=<optimized out>, time=<optimized out>, state=<optimized out>, __closure=0x5555558d7d30) at /usr/src/debug/kwin5-5.11.95-1.1.x86_64/input.cpp:1784
#8  0x00007ffff79be19b in std::__invoke_impl<void, KWin::InputRedirection::setupLibInput()::<lambda(KWin::SwitchEvent::State, quint32, quint64, KWin::LibInput::Device*)>&, KWin::SwitchEvent::State&, unsigned int&, long long unsigned int&, KWin::LibInput::Device*&> (__f=...) at /usr/include/c++/7/bits/invoke.h:60
#9  0x00007ffff79be19b in std::__invoke<KWin::InputRedirection::setupLibInput()::<lambda(KWin::SwitchEvent::State, quint32, quint64, KWin::LibInput::Device*)>&, KWin::SwitchEvent::State&, unsigned int&, long long unsigned int&, KWin::LibInput::Device*&> (__fn=...) at /usr/include/c++/7/bits/invoke.h:95
#10 0x00007ffff79be19b in std::_Bind<KWin::InputRedirection::setupLibInput()::<lambda(KWin::SwitchEvent::State, quint32, quint64, KWin::LibInput::Device*)>(KWin::SwitchEvent::State, std::_Placeholder<1>, std::_Placeholder<2>, std::_Placeholder<3>)>::__call<void, unsigned int&, unsigned long long&, KWin::LibInput::Device*&, 0, 1, 2, 3> (__args=..., this=0x5555558d7d30) at /usr/include/c++/7/functional:467
#11 0x00007ffff79be19b in std::_Bind<KWin::InputRedirection::setupLibInput()::<lambda(KWin::SwitchEvent::State, quint32, quint64, KWin::LibInput::Device*)>(KWin::SwitchEvent::State, std::_Placeholder<1>, std::_Placeholder<2>, std::_Placeholder<3>)>::operator()<unsigned int&, long long unsigned int&, KWin::LibInput::Device*&> (this=0x5555558d7d30) at /usr/include/c++/7/functional:551
#12 0x00007ffff79be19b in QtPrivate::FunctorCall<QtPrivate::IndexesList<0, 1, 2>, QtPrivate::List<unsigned int, long long unsigned int, KWin::LibInput::Device*>, void, std::_Bind<KWin::InputRedirection::setupLibInput()::<lambda(KWin::SwitchEvent::State, quint32, quint64, KWin::LibInput::Device*)>(KWin::SwitchEvent::State, std::_Placeholder<1>, std::_Placeholder<2>, std::_Placeholder<3>)> >::call (arg=<optimized out>, f=...) at /usr/include/qt5/QtCore/qobjectdefs_impl.h:130
#13 0x00007ffff79be19b in QtPrivate::Functor<std::_Bind<KWin::InputRedirection::setupLibInput()::<lambda(KWin::SwitchEvent::State, quint32, quint64, KWin::LibInput::Device*)>(KWin::SwitchEvent::State, std::_Placeholder<1>, std::_Placeholder<2>, std::_Placeholder<3>)>, 3>::call<QtPrivate::List<unsigned int, unsigned long long, KWin::LibInput::Device*>, void> (arg=<optimized out>, f=...) at /usr/include/qt5/QtCore/qobjectdefs_impl.h:240
#14 0x00007ffff79be19b in QtPrivate::QFunctorSlotObject<std::_Bind<KWin::InputRedirection::setupLibInput()::<lambda(KWin::SwitchEvent::State, quint32, quint64, KWin::LibInput::Device*)>(KWin::SwitchEvent::State, std::_Placeholder<1>, std::_Placeholder<2>, std::_Placeholder<3>)>, 3, QtPrivate::List<unsigned int, long long unsigned int, KWin::LibInput::Device*>, void>::impl(int, QtPrivate::QSlotObjectBase *, QObject *, void **, bool *) (which=<optimized out>, this_=0x5555558d7d20, r=<optimized out>, a=<optimized out>, ret=<optimized out>) at /usr/include/qt5/QtCore/qobjectdefs_impl.h:423
Comment 1 Martin Flöser 2018-01-31 17:24:05 UTC
Looks like the device is null
Comment 2 Martin Flöser 2018-01-31 18:03:52 UTC
Can you get me the full backtrace? I have an idea what's going on, but I cannot verify yet.

I assume that the Device gets destroyed in another thread before the events get processed. With a full backtrace I could see whether the event gets queued from the main event loop.
Comment 3 Fabian Vogt 2018-01-31 18:16:01 UTC
(In reply to Martin Flöser from comment #2)
> Can you get me the full backtrace? I have an idea what's going on, but I
> cannot verify yet.

Unfortunately it's not that easy: The setup where I could reproduce this (with dock etc.) is at my now old workplace.
I might be able to give it a quick try tomorrow (if I don't get thrown out instead), but I can't guarantee it.

> I assume that the Device gets destroyed in another thread before the events
> get processed. With a full backtrace I could see whether the event gets
> queued from the main event loop.

If you mean a race condition, it's unlikely as this fails 20/20 times with identical backtrace.
Comment 4 Martin Flöser 2018-01-31 18:50:28 UTC
(In reply to Fabian Vogt from comment #3)
> (In reply to Martin Flöser from comment #2)
> > Can you get me the full backtrace? I have an idea what's going on, but I
> > cannot verify yet.
> 
> Unfortunately it's not that easy: The setup where I could reproduce this
> (with dock etc.) is at my now old workplace.
> I might be able to give it a quick try tomorrow (if I don't get thrown out
> instead), but I can't guarantee it.

uh that sounds bad. I'm very sorry for you *hugs*

> 
> > I assume that the Device gets destroyed in another thread before the events
> > get processed. With a full backtrace I could see whether the event gets
> > queued from the main event loop.
> 
> If you mean a race condition, it's unlikely as this fails 20/20 times with
> identical backtrace.

No, I don't think it's a race.
Comment 5 Fabian Vogt 2018-01-31 19:15:32 UTC
(In reply to Martin Flöser from comment #4)
> (In reply to Fabian Vogt from comment #3)
> > (In reply to Martin Flöser from comment #2)
> > > Can you get me the full backtrace? I have an idea what's going on, but I
> > > cannot verify yet.
> > 
> > Unfortunately it's not that easy: The setup where I could reproduce this
> > (with dock etc.) is at my now old workplace.
> > I might be able to give it a quick try tomorrow (if I don't get thrown out
> > instead), but I can't guarantee it.
> 
> uh that sounds bad. I'm very sorry for you *hugs*

It probably sounded worse than it is: My temporary position is over tomorrow
and until I get a regular one I am technically not allowed in the office.
So no worries! *hugs back*

> > > I assume that the Device gets destroyed in another thread before the events
> > > get processed. With a full backtrace I could see whether the event gets
> > > queued from the main event loop.
> > 
> > If you mean a race condition, it's unlikely as this fails 20/20 times with
> > identical backtrace.
> 
> No, I don't think it's a race.

Ok. If you have an idea already and it sounds plausible you could make a patch
for verification which I might be able to test tomorrow as well.
Comment 6 Martin Flöser 2018-01-31 20:24:17 UTC
(In reply to Fabian Vogt from comment #5)
> Ok. If you have an idea already and it sounds plausible you could make a
> patch
> for verification which I might be able to test tomorrow as well.

If my theory is correct it'll be difficult to patch, unfortunately.

The Device* is passed as signal argument, but I fear the signal gets queued. If now in another thread the device gets destroyed, we call deleteLater on the Device and then we get into the crash situation.

The bad news is that this could happen in many areas. The Device* argument is passed along in quite a few signals. So if my theory is correct, this might be crashy in various places. The good news is it should be possible to verify in the DebugConsole.
Comment 7 Martin Flöser 2018-01-31 20:25:59 UTC
And in DebugConsole I couldn't get it to crash.
Comment 8 Fabian Vogt 2018-01-31 20:39:12 UTC
(In reply to Martin Flöser from comment #6)
> (In reply to Fabian Vogt from comment #5)
> > Ok. If you have an idea already and it sounds plausible you could make a
> > patch
> > for verification which I might be able to test tomorrow as well.
> 
> If my theory is correct it'll be difficult to patch, unfortunately.
> 
> The Device* is passed as signal argument, but I fear the signal gets queued.
> If now in another thread the device gets destroyed, we call deleteLater on
> the Device and then we get into the crash situation.

I guess I could verify this by building kwin without the device->deleteLater call then.

However, why is there a device remove event in the first place?
Comment 9 Martin Flöser 2018-02-01 05:15:57 UTC
I assume the switch device gets removed because the notebook is docked and thus cannot be turned into a tablet.
Comment 10 Fabian Vogt 2018-02-01 11:52:10 UTC
(In reply to Martin Flöser from comment #9)
> I assume the switch device gets removed because the notebook is docked and
> thus cannot be turned into a tablet.

Well, even when undocked it can't be. It has neither a touchscreen nor hinges able to survive 180° bends.

I'll attach the output of "libinput list-devices" when docked.
Comment 11 Fabian Vogt 2018-02-01 14:30:28 UTC
Created attachment 110282 [details]
libinput list-devices

Good news! (I think)

The other threads are completely unrelated and waiting for events:

Thread 4 (Thread 0x7fffdd6ab700 (LWP 26682)):
#0  0x00007fffecb3af2b in poll () at /lib64/libc.so.6
#1  0x00007fffe6c74149 in  () at /usr/lib64/libglib-2.0.so.0
#2  0x00007fffe6c7425c in g_main_context_iteration () at /usr/lib64/libglib-2.0.so.0
#3  0x00007fffedee855f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib64/libQt5Core.so.5
#4  0x00007fffede8f4aa in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib64/libQt5Core.so.5
#5  0x00007fffedcb68da in QThread::exec() () at /usr/lib64/libQt5Core.so.5
#6  0x00007fffedcbb8d0 in  () at /usr/lib64/libQt5Core.so.5
#7  0x00007ffff02b1558 in start_thread () at /lib64/libpthread.so.0
#8  0x00007fffecb456df in clone () at /lib64/libc.so.6

Thread 3 (Thread 0x7fffddeac700 (LWP 26681)):
#0  0x00007fffecb3af2b in poll () at /lib64/libc.so.6
#1  0x00007fffe6c74149 in  () at /usr/lib64/libglib-2.0.so.0
#2  0x00007fffe6c7425c in g_main_context_iteration () at /usr/lib64/libglib-2.0.so.0
#3  0x00007fffedee855f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib64/libQt5Core.so.5
#4  0x00007fffede8f4aa in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib64/libQt5Core.so.5
#5  0x00007fffedcb68da in QThread::exec() () at /usr/lib64/libQt5Core.so.5
#6  0x00007fffedcbb8d0 in  () at /usr/lib64/libQt5Core.so.5
#7  0x00007ffff02b1558 in start_thread () at /lib64/libpthread.so.0
#8  0x00007fffecb456df in clone () at /lib64/libc.so.6

Thread 2 (Thread 0x7fffde6ad700 (LWP 26680)):
#0  0x00007fffecb3af2b in poll () at /lib64/libc.so.6
#1  0x00007fffe6c74149 in  () at /usr/lib64/libglib-2.0.so.0
#2  0x00007fffe6c7425c in g_main_context_iteration () at /usr/lib64/libglib-2.0.so.0
#3  0x00007fffedee855f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib64/libQt5Core.so.5
#4  0x00007fffede8f4aa in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () at /usr/lib64/libQt5Core.so.5
#5  0x00007fffedcb68da in QThread::exec() () at /usr/lib64/libQt5Core.so.5
#6  0x00007ffff2e96bd5 in  () at /usr/lib64/libQt5DBus.so.5
#7  0x00007fffedcbb8d0 in  () at /usr/lib64/libQt5Core.so.5
#8  0x00007ffff02b1558 in start_thread () at /lib64/libpthread.so.0
#9  0x00007fffecb456df in clone () at /lib64/libc.so.6

I found out that I can easily work around this by opening the lid while starting kwin_wayland.
This seems to be caused by this:

> sudo libinput debug-events
-event3   DEVICE_ADDED     Power Button                      seat0 default group1  cap:k
-event4   DEVICE_ADDED     Video Bus                         seat0 default group2  cap:k
-event1   DEVICE_ADDED     Lid Switch                        seat0 default group3  cap:S
 event1   SWITCH_TOGGLE     +0.00s      switch lid state 1
-event2   DEVICE_ADDED     Sleep Button                      seat0 default group4  cap:k
[...] some more DEVICE_ADDED
Comment 12 Fabian Vogt 2018-02-01 15:34:47 UTC
Just had a debugging session and I found the issue - it's a design flaw.

Connection::handleEvent reads all available events from libinput and constructs Event objects.
Those event objects try to find the corresponding LibInput::Device* to reference, but if
the DEVICE_ADDED and SWITCH_TOGGLE events arrive in the same batch, the switch device is
not created before the SwitchEvent is created.
Comment 13 Fabian Vogt 2018-02-01 15:57:22 UTC
I made a patch which fixes the issue. It's not that pretty, but I don't think there is an easier way.
Comment 14 Martin Flöser 2018-02-03 15:03:40 UTC
Git commit 6c00cfb5c75dd318421bc92b3a7da5454d169028 by Martin Flöser.
Committed on 03/02/2018 at 15:03.
Pushed by graesslin into branch 'Plasma/5.12'.

[libinput] Ensure Event::device returns a proper Device

Summary:
This fixes a problem when a Device added and another event on the Device
are queued together. In that case the second event would not get the
Device set as it's not yet created.

This change ensures that when accessing device the pointer will be
updated.
FIXED-IN: 5.12.0

Reviewers: #kwin, #plasma, fvogt

Subscribers: plasma-devel, kwin

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D10236

M  +9    -1    libinput/events.cpp
M  +2    -4    libinput/events.h

https://commits.kde.org/kwin/6c00cfb5c75dd318421bc92b3a7da5454d169028