Differential D11015

Sanity check WindowQuad before trying to create a grid out of it
ClosedPublic
Actions

Authored by graesslin on Mar 4 2018, 10:29 AM.

Details

Reviewers
None
Group Reviewers
KWin
Plasma
Commits
R108:e1afef3d4563: Sanity check WindowQuad before trying to create a grid out of it
Summary

When one uses:

  • breeze as of 5.12
  • wobbly windows
  • shaded window
  • a distribution building with assert enabled

and starts to move a shaded window, KWin asserts. The root cause for
this is that WindowQuad::makeSubQuad has an assert for y1 being smaller
than y2. With the combination listed above this is not guaranteed. For
the left shadow quad the y1 and y2 are identical and thus trying to
split it, results in the assert condition.

The problem of the shadow quad having an invalid size might be addressed
as well with D10811. Due to that the generation of the quads is not
touched. Instead a sanity check is introduced to not try to split
already invalid sized quads.

BUG: 390953
FIXED-IN: 5.12.X

Test Plan

Added unit test hit the assert, now doesn't hit it any more

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
graesslin created this revision.Mar 4 2018, 10:29 AM
Restricted Application added a project: KWin. · View Herald TranscriptMar 4 2018, 10:29 AM
Restricted Application added a subscriber: kwin. · View Herald Transcript
graesslin requested review of this revision.Mar 4 2018, 10:29 AM
zzag added a subscriber: zzag.Mar 4 2018, 1:28 PM

With the combination listed above this is not guaranteed. For the left shadow quad the y1 and y2 are identical and thus trying to split it, results in the assert condition.

Hmm, that's weird. I believe with current implementation of SceneOpenGLShadow & current Breeze shadows the height of the left/right shadow quad should be > 1, even for shaded windows.

libkwineffects/kwineffects.cpp
1159

Comparison of floating point numbers should be done in a different way.

1219

Same here.

graesslin added inline comments.Mar 4 2018, 4:31 PM
libkwineffects/kwineffects.cpp
1159

There are more places where doubles are compared exactly like that. E.g. in WindowQuadList::splitAtX, sliptAtY, WindowMotionManager::moveWindow, WindowQuad::smoothNeeded()

Basically that's everywhere wrong.

My suggestion is to commit as is and afterwards do a correction for all cases in one go.

zzag added a comment.EditedMar 4 2018, 10:23 PM

Does it make sense to add a quad which has no size(i.e. width or height is 0)? Could empty quads be rejected? (I'm talking about line 1220)

libkwineffects/kwineffects.cpp
1159

Seems reasonable.

graesslin added a comment.Mar 5 2018, 5:30 AM
In D11015#218603, @zzag wrote:

Does it make sense to add a quad which has no size(i.e. width or height is 0)? Could empty quads be rejected? (I'm talking about line 1220)

No, but it was already in the list and due to that I decided to not filter it out. That was the place where I thought most about it.

zzag added a comment.Mar 5 2018, 12:47 PM

I suggest to revise it at some point in the future. Maybe, there could be some sort of sanitization...

About D10811, in some sort it addresses this problem... There won't be top/right/bottom/left shadow quads with zero size unless inner shadow rect has been put somewhere on the edge. In that case, any shadow quad(top-left, left, etc) could have "invalid" size.

Anyway, I think this patch is good(FWIW, splitAtX and splitAtY do the same). Even though I'm not a member of either KWin or Plasma, I could review it. :)

Off topic: maybe, the assert statement can be replaced with the Q_ASSERT statement.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 5 2018, 6:33 PM
Closed by commit R108:e1afef3d4563: Sanity check WindowQuad before trying to create a grid out of it (authored by graesslin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 28741

View Options

autotests/integration/effects/CMakeLists.txt

Loading...
View Options

autotests/integration/effects/wobbly_shade_test.cpp

Loading...
View Options

libkwineffects/kwineffects.cpp

Loading...