fix occassional crash caused by needlessly delayed signals (bko#363224)
ClosedPublic

Authored by lunakl on Mar 24 2017, 9:38 AM.

Details

Reviewers
graesslin
Group Reviewers
KWin
Summary

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).

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
lunakl created this revision.Mar 24 2017, 9:38 AM
Restricted Application added a subscriber: kwin. · View Herald TranscriptMar 24 2017, 9:38 AM

i'm never experienced such a crash, can you try my suggestion ?

effects.cpp
146

Add here after }
, Qt::QueuedConnection

graesslin accepted this revision.Mar 24 2017, 4:17 PM
graesslin added a subscriber: graesslin.

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.

This revision is now accepted and ready to land.Mar 24 2017, 4:17 PM

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);

}

Given that it might introduce graphical issues (assuming that there was a good reason for the delaying) only in master please.

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?

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)

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.

In D5164#98159, @lunakl wrote:

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.

In D5164#98159, @lunakl wrote:

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.

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.

In D5164#98159, @lunakl wrote:

Given that it might introduce graphical issues (assuming that there was a good reason for the delaying) only in master please.

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.

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.

In D5164#98159, @lunakl wrote:

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.

As reference, the (last) line should have been:

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

You can still close it manually of course.

graesslin closed this revision.Apr 17 2017, 7:33 AM