Bug 1187324 - [Patch] Crash due to unsafe access to ShellCorona::m_screenConfiguration
Summary: [Patch] Crash due to unsafe access to ShellCorona::m_screenConfiguration
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: plasma-workspace
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: KDE SIG
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: F22Blocker-kde
TreeView+ depends on / blocked
 
Reported: 2015-01-29 19:00 UTC by Sandro Mani
Modified: 2015-03-02 08:47 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-03-02 08:47:00 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Backtrace (2.82 KB, text/plain)
2015-01-29 19:00 UTC, Sandro Mani
no flags Details
Proposed patch 1: check m_screenConfiguration in ShellCorona::reconsiderOutputs (681 bytes, patch)
2015-01-29 19:00 UTC, Sandro Mani
no flags Details | Diff
Proposed patch 2: initialize m_screenConfiguration with default constructed KScreen::Config (1.27 KB, patch)
2015-01-29 19:01 UTC, Sandro Mani
no flags Details | Diff


Links
System ID Private Priority Status Summary Last Updated
KDE Software Compilation 342413 0 None None None Never

Description Sandro Mani 2015-01-29 19:00:09 UTC
Created attachment 985711 [details]
Backtrace

Description of problem:
When changing displays, one reason plasmashell can crash is an unsafe access to ShellCorona::m_screenConfiguration which can be null, see attached backtrace (other crash reasons are various issues with Qt5, see #1083664).

The minimal fix for this particular crash is to check m_screenConfiguration in ShellCorona::reconsiderOutputs, there are however many other unsafe accesses to reconsiderOutputs all over. See first attached patch.

Another fix would be to ensure screenConfiguration is always initialized with a default constructed KScreen::Config and let KScreen::Config take care of returning sane values when no outputs are connected. See second attached patch.

Version-Release number of selected component (if applicable):
plasma-workspace-5.2.0-3.fc22.x86_64

Comment 1 Sandro Mani 2015-01-29 19:00:53 UTC
Created attachment 985712 [details]
Proposed patch 1: check m_screenConfiguration in ShellCorona::reconsiderOutputs

Comment 2 Sandro Mani 2015-01-29 19:01:43 UTC
Created attachment 985713 [details]
Proposed patch 2: initialize m_screenConfiguration with default constructed KScreen::Config

Comment 3 Daniel Vrátil 2015-01-30 13:06:20 UTC
Would you mind submitting the patches to upstream for review? You should upload it to git.reviewboard.kde.org and set group to "plasma".

Although I'm partially responsible for this code, the Plasma devs will know better if this is a sufficient and correct fix.

Comment 4 Sandro Mani 2015-01-30 21:59:34 UTC
Before posting upstream: this patch together with what I posted here [1] seems to stop things from crashing when changing display configuration. As noted in [1] however, plasmashell (and kwin) occasionally seem to get their geometry messed up, but I suspect this is a kscreen issue.

Daniel, since you are one of the authors of kscreen, do you have any idea whether the messed up geometry might indeed be a kscreen issue?

[1] https://bugreports.qt.io/browse/QTBUG-42985?focusedCommentId=271750

Comment 5 Rex Dieter 2015-01-31 15:13:13 UTC
See also,
https://bugs.kde.org/show_bug.cgi?id=342413

which looks a lot like what we're seeing here (I added a comment there saying as much)

Comment 6 Daniel Vrátil 2015-02-08 17:20:10 UTC
The geometry mess is probably caused by another problems in KScreen-Plasma integration, but you crash fix seems OK to me.

It should be tackled in a different patch.

Comment 7 Sandro Mani 2015-02-09 23:48:20 UTC
Posted for review: https://git.reviewboard.kde.org/r/122506/

By the way, to what extent are KDE upstream and the F21 Plasma5 feature authors aware of all these screen related issues? Upstream Qt5 issues have been open and largely ignored for the past two or so years. Some patches for the most severe issues (i.e. those resulting in unconditional crashes of any qt5 application as a result of screen switching) have now been accepted (resp. are being reviewing), but there are still a number of harder to debug issues causing pain, not only crashes but also things like menus popping up misplaced and windows not correctly redrawing anymore. Honestly, the chance of having a usable desktop after a screen configuration change as it stands now is very very low.

Unless there is a reasonable expectation of things getting more stable soonish, I really think that shipping Plasma5 in F21 should be reconsidered.

Comment 8 Rex Dieter 2015-02-10 07:18:22 UTC
Dan is one upstream kscreen developer, he's CC'd here and monitoring situation closely.

Comment 9 Sandro Mani 2015-03-02 08:47:00 UTC
I guess this has been fixed at the source with the fixes which landed in Qt (and are in the Fedora package). Closing.


Note You need to log in before you can comment on or make changes to this bug.