Review Request 126961: Fix the infamous Plasma::Applet::Actions crash

[Sebastian Kügler]

> On Feb. 2, 2016, 12:43 p.m., Sebastian Kügler wrote:
> > shell/shellcorona.cpp, line 629
> > <https://git.reviewboard.kde.org/r/126961/diff/1/?file=442514#file442514line629>
> >
> >     m_screenConfiguration may still be a nullptr here, since it's set only after GetConfigOperation returns.
> >     
> >     That may well be the reason why the sync QScreen's count is used here, but this leads to problems down the road, as you note.
> >     
> >     We could make sure we have the number of screens returned here by making GetConfigOperation sync, so using its exec() and thus blocking in the setShell call.
> 
> David Edmundson wrote:
>     ooh good catch.
>     
>     I /think/ a simple if (null) return 0; is semantically correct.
>     As this should be a count of screens plasma knows about.

That could prevent the crash, but then we'd fire more events than necessary (most likely, there will be at least one screen, so next thing that happens is a outputAdded signal). I think op->exec() is the more elegant solution here, especially since plasmashell has no business if there aren't any screens connected, we might as well just wait. The wait here can be quite significant as GetConfigOperation starts a separate process to run the backend in and then does IPC. (KSCREEN_BACKEND_INPROCESS=1 "fixes" that,  but is dangerous for XCB since we had a lot of crashers there, and looking at some bugreports, we still have.)


- Sebastian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126961/#review91946
-----------------------------------------------------------


On Feb. 1, 2016, 11:08 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126961/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2016, 11:08 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Repository: plasma-workspace
> 
> 
> Description
> -------
> 
> We were mixing KScreen and QScreen API badly.
> 
> Corona.cpp checks we are requesting a containment for a valid screen
> if (screen >= 0 && screen < numScreens()) {
> 
> This fails as numScreens() is Qt API based, whereas the signal we're
> adding the output for is ShellCorona::addOutput so we have an effective race condition.
> 
> BUG: 351777
> 
> 
> Diffs
> -----
> 
>   shell/shellcorona.cpp 762e503bf59fe648fb0f5b76a36229aa43c563e5 
> 
> Diff: https://git.reviewboard.kde.org/r/126961/diff/
> 
> 
> Testing
> -------
> 
> Started Plasma on dual screen.
> 
> Ideally we need to do more testing before backporting, as that entire codebase is a disgrace.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160202/edfde716/attachment.html>