As pointed out by the Valgrind trace in #363224, delaying the signal causes the EffectWindow* argument to become invalid before the connected slot is called (this is because Qt discards only delayed signal->slot calls where the receiver gets deleted meanwhile, not the sender and definitely not a random argument.
If the supposed glitches really happen, they should get fixed correctly, and for all cases (I doubt only desktop number would be involved but not e.g. shaded or minimized states).
Details
Details
Diff Detail
Diff Detail
- Repository
- R108 KWin
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
i'm never experienced such a crash, can you try my suggestion ?
| effects.cpp | ||
|---|---|---|
| 146 | Add here after } | |
Comment Actions
Given that it might introduce graphical issues (assuming that there was a good reason for the delaying) only in master please.
| effects.cpp | ||
|---|---|---|
| 146 | no, the Qt::QueuedConnection would result in the same problem again. | |
Comment Actions
To keep the delay, one would have to wire the call through a secure dispatcher which guarantees (by searching the list of all windows) the effectwindow still points valid memory before passing the function call on (or use a guarded pointer)
Effects::safeEmitDesktopPresenceChanged(QPointer<EffectWindow> w, int old, int new) {
if (w) emit desktopPresenceChanged(w, old, new);
}
Comment Actions
Are you sure about that? The bugreport has quite a number of duplicates, and possible graphical issues during a rare operation seem preferable to crashes, even if it's a non-default decoration.
Btw, how do I now push the patch? Do I have to do it manually using git?
No need for anything so hackish. Pending queued signals are discarded if the receiver of the signal gets deleted. But, as I said, one way or another I don't see a valid reason for a delayed signal here.
Comment Actions
No need for anything so hackish. Pending queued signals are discarded if the receiver of the signal gets deleted. But, as I said, one way or another I don't see a valid reason for a delayed signal here.
Afaiu it's not a sender/receiver, but a parameter (the effect window) that gets deleted interim. Otherwise there should have been no problem itfp, yesno? Effects fires, ScriptedEffect receives, does some w/ the window and falls flat bc. the EffectWindow is meanwhile gone.
Comment Actions
I was commenting on your "secure dispatcher" idea that I quoted. Qt itself is the secure dispatcher as long as the deleted object would be the signal receiver.
Comment Actions
Yes. We still have a few weeks till the next bugfix release, so we can backport if it doesn't show an issue. My main concern is that the crash only happens in a weird event sequence and even then not everywhere (distro, compiler, phase of moon something is playing into it). But if it causes a regression it might hit everyone.
Btw, how do I now push the patch? Do I have to do it manually using git?
Just use git push. If you didn't create this phab request through arc patch, you have to add the differential revision url as last line for auto closing.
Comment Actions
As reference, the (last) line should have been:
Differential Revision: https://phabricator.kde.org/D5164
You can still close it manually of course.