Bug 377808

Summary: Crash on disconnect of primary monitor
Product: [Plasma] plasmashell Reporter: Roman Gilg <subdiff>
Component: generic-multiscreenAssignee: Aleix Pol <aleixpol>
Status: RESOLVED FIXED    
Severity: crash CC: agrar.pfurtz, ddash.rice, kde, notmart, plasma-bugs
Priority: NOR    
Version: master   
Target Milestone: 1.0   
Platform: Neon   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:

Description Roman Gilg 2017-03-19 17:28:46 UTC
Problem seems to be that it tries to change the primary screen at the same time as one of the screens gets removed.
Comment 1 Roman Gilg 2017-03-19 17:30:14 UTC
----------
Backtrace:
----------
Thread 1 (Thread 0x7f919e6388c0 (LWP 16430)):
[KCrash Handler]
#6  0x00007f9198bf5428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#7  0x00007f9198bf702a in __GI_abort () at abort.c:89
#8  0x00007f91993a0811 in QMessageLogger::fatal(char const*, ...) const () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#9  0x00007f919939bdbe in qt_assert(char const*, char const*, int) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#10 0x000000000044b94c in ShellCorona::screenInvariants (this=0x13286b0) at /home/roman/Entwicklung/kde/source/kde/workspace/plasma-workspace/shell/shellcorona.cpp:761
#11 0x000000000044e72b in ShellCorona::reconsiderOutputs (this=0x13286b0) at /home/roman/Entwicklung/kde/source/kde/workspace/plasma-workspace/shell/shellcorona.cpp:1172
#12 0x000000000044b4c9 in ShellCorona::primaryOutputChanged (this=0x13286b0) at /home/roman/Entwicklung/kde/source/kde/workspace/plasma-workspace/shell/shellcorona.cpp:725
#13 0x000000000046b83f in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (ShellCorona::*)()>::call(void (ShellCorona::*)(), ShellCorona*, void**) (f=(void (ShellCorona::*)(ShellCorona * const)) 0x44b46c <ShellCorona::primaryOutputChanged()>, o=0x13286b0, arg=0x7ffff4176db0) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:141
#14 0x0000000000469d3b in QtPrivate::FunctionPointer<void (ShellCorona::*)()>::call<QtPrivate::List<>, void>(void (ShellCorona::*)(), ShellCorona*, void**) (f=(void (ShellCorona::*)(ShellCorona * const)) 0x44b46c <ShellCorona::primaryOutputChanged()>, o=0x13286b0, arg=0x7ffff4176db0) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:160
#15 0x0000000000466125 in QtPrivate::QSlotObject<void (ShellCorona::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (which=1, this_=0x1a0c790, r=0x13286b0, a=0x7ffff4176db0, ret=0x0) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobject_impl.h:120
#16 0x00007f91995b21f6 in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#17 0x00007f91998c08c2 in QGuiApplication::primaryScreenChanged(QScreen*) () from /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
#18 0x00007f91998b3820 in QPlatformIntegration::destroyScreen(QPlatformScreen*) () from /usr/lib/x86_64-linux-gnu/libQt5Gui.so.5
#19 0x00007f918bb8e116 in QtWaylandClient::QWaylandDisplay::registry_global_remove(unsigned int) () from /usr/lib/x86_64-linux-gnu/libQt5WaylandClient.so.5
#20 0x00007f91940e1e40 in ffi_call_unix64 () from /usr/lib/x86_64-linux-gnu/libffi.so.6
#21 0x00007f91940e18ab in ffi_call () from /usr/lib/x86_64-linux-gnu/libffi.so.6
#22 0x00007f9197e1c1e8 in ?? () from /usr/lib/x86_64-linux-gnu/libwayland-client.so.0
#23 0x00007f9197e194f0 in ?? () from /usr/lib/x86_64-linux-gnu/libwayland-client.so.0
#24 0x00007f9197e1956c in ?? () from /usr/lib/x86_64-linux-gnu/libwayland-client.so.0
#25 0x00007f9197e1a2d4 in wl_display_dispatch_queue_pending () from /usr/lib/x86_64-linux-gnu/libwayland-client.so.0
#26 0x00007f918bb8c7a2 in QtWaylandClient::QWaylandDisplay::flushRequests() () from /usr/lib/x86_64-linux-gnu/libQt5WaylandClient.so.5
#27 0x00007f91995b1e89 in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#28 0x00007f919962a99e in QSocketNotifier::activated(int, QSocketNotifier::QPrivateSignal) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#29 0x00007f91995be3eb in QSocketNotifier::event(QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#30 0x00007f9199e65ecc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#31 0x00007f9199e6d8c6 in QApplication::notify(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Widgets.so.5
#32 0x00007f91995870c8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#33 0x00007f91995dabfd in ?? () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#34 0x00007f9194536197 in g_main_context_dispatch () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#35 0x00007f91945363f0 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#36 0x00007f919453649c in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#37 0x00007f91995da73f in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#38 0x00007f91995850ba in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#39 0x00007f919958d6cc in QCoreApplication::exec() () from /usr/lib/x86_64-linux-gnu/libQt5Core.so.5
#40 0x0000000000421cff in main (argc=1, argv=0x7ffff4177a88) at /home/roman/Entwicklung/kde/source/kde/workspace/plasma-workspace/shell/main.cpp:166
Comment 2 Kai Uwe Broulik 2017-03-21 09:20:24 UTC
screenInvariants() is only called in debug builds from what I can tell, can you launch plasmashell from console and see the Q_ASSERT message it prints?
Comment 3 Roman Gilg 2017-03-21 12:31:03 UTC
Good call!

ASSERT: "m_desktopViewforId.keys().count() <= QGuiApplication::screens().count()" in file /home/roman/Entwicklung/kde/source/kde/workspace/plasma-workspace/shell/shellcorona.cpp, line 761
Comment 4 Roman Gilg 2017-03-25 23:40:33 UTC
Details worth mentioning:

This only happens when the 2nd screen was already connected in SDDM. After login it shows the panel on the 2nd screen (it also doesn't remember the position from last time) and then disconnecting leads to the crash.

Connecting and disconnecting while in the session works without problems.
Comment 5 Marco Martin 2017-04-04 11:56:56 UTC
the screen being disconnected is the primary screen?
Comment 6 Roman Gilg 2017-04-04 13:02:01 UTC
Yes, but the external monitor only is the primary screen, if I login from sddm with the monitor already being connected in sddm. I also can't change the primary screen in session with the KCM (but I assume that's unrelated).
Comment 7 Kai Uwe Broulik 2017-04-05 08:30:38 UTC
*** Bug 378408 has been marked as a duplicate of this bug. ***
Comment 8 Kai Uwe Broulik 2017-04-05 08:37:34 UTC
I can reproduce:

1.) have two screens, calling it Left and Right here. Make Left the primary.
2.) quit plasmashell
3.) now unplug the cable for Left, kscreen will remove the screen and Right will become primary
4.) start plasmashell (it is important that plasmashell started with one screen only, otherwise it works fine, ie. just plugging it out and back in is fine)
5.) plug the cable back in, kscreen will restore the previous screen config, leading to Left becoming primary again
6.) boom

From what I can tell, crashes in shellcorona.cpp:730 QScreen *oldPrimary = m_desktopViewforId.value(0)->screen();

It calls reconsiderOutputs() which might remove and delete a view (at one point we only have one in the above scenario), so m_desktopViewforId is now empty, afterwards we do m_desktopViewforId.value(0)->screen() which returns a nullptr and we crash.

I tried doing moving reconsiderOutputs() first and then check if m_desktopViewForId.contains(0), while it won't crash anymore it also resulted in views and panels being messed up. It seems we need to be able to deal with m_desktopViewForId being empty and oldScreen being nullptr (this variable is used a couple of times in that function, actually, so might be again Pandora's box to fix :/)

(also crashes on X11, re-assigning to "Multi Screen")
Comment 9 Marco Martin 2017-04-05 09:37:55 UTC
(In reply to Roman Gilg from comment #4)
> Details worth mentioning:
> 
> This only happens when the 2nd screen was already connected in SDDM. After
> login it shows the panel on the 2nd screen (it also doesn't remember the
> position from last time) and then disconnecting leads to the crash.
> 
> Connecting and disconnecting while in the session works without problems.

can you also try if the crash happens as well on a plasmashell that was started manually from a terminal, to "emulate" the shell being in a new session?
Comment 10 Marco Martin 2017-04-05 09:41:00 UTC
(In reply to Kai Uwe Broulik from comment #8)
> 
> It calls reconsiderOutputs() which might remove and delete a view (at one
> point we only have one in the above scenario), so m_desktopViewforId is now
> empty, afterwards we do m_desktopViewforId.value(0)->screen() which returns
> a nullptr and we crash.

what looks strange to me, is that reconsiderOutputs() removes screens only when one is considered "redundant"...
why this should ever happen when you pass from one screen to two, that are not overlapping?
Comment 11 Marco Martin 2017-04-06 12:43:03 UTC
also: what's the Qt version? some of this problems may depend on it (and why i can't reproduce any)
Comment 12 Kai Uwe Broulik 2017-04-06 12:46:07 UTC
Qt 5.7.1 for me
Comment 13 Marco Martin 2017-04-06 13:40:07 UTC
the theory i can make is that a new primary screen arrives, but has a wrong geometry for a while, making the old screen "redundant", then it gets moved/resized (maybe even without emitting signals) so things get very wrong
Comment 14 Marco Martin 2017-04-06 14:15:37 UTC
one more thing that has been discovered: it appears this also depends from the primary screen (that is getting removed or added) to be at 0,0 position and the other screen being at the right of it.

this makes the new screen overlap the old one for a bit, as events in kscreen don't seem atomic
Comment 15 Marco Martin 2017-04-06 17:11:58 UTC
possible fix: https://phabricator.kde.org/D5323
Comment 16 Marco Martin 2017-04-07 10:14:55 UTC
Git commit e0a8df2910fa272f5ed5913ea38b578f5c7145d0 by Marco Martin.
Committed on 07/04/2017 at 10:14.
Pushed by mart into branch 'master'.

Correctly handle when a new primary screen displaces the old

Summary:
this is for the following setup:
the primary screen is at position 0,0 and gets disconnected.
the other screen will be moved at 0,0 and becomes primary

the screen is reconnected, the events arrive in the followin order:
1) a new screen gets added, at 0,0 position
   (not primary yet, it may be markedredundant)
2) the screen becomes primary, both screens still at 0,0
3) the old screen gets moved out of the way

in the end result none of the two need to be redundant.
adding the old one in the redundant list, will cause reconsideroutputs
to consider it and create a view for it.

Test Plan:
added and removed sevaral times a primary screen at 0,0
also tried other positions of screens to check it doesn't make regressions

Reviewers: #plasma, sebas, subdiff, broulik

Subscribers: plasma-devel

Tags: #plasma

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

M  +15   -6    shell/shellcorona.cpp

https://commits.kde.org/plasma-workspace/e0a8df2910fa272f5ed5913ea38b578f5c7145d0
Comment 17 Marco Martin 2017-04-11 13:53:56 UTC
*** Bug 378416 has been marked as a duplicate of this bug. ***