Make sure OpenGL Context is valid before deleting shader
ClosedPublic

Authored by davidedmundson on Sep 20 2017, 3:18 PM.

Details

Summary

Deleting the lanczos filter deletes it's GLShader, this calls
glDeleteProgram

glFooBar always needs to have an openGL context, we don't know we have
this on a screen changed event as it is called from outside the normal
render methods.

BUG: 384884

Test Plan

Ran on my wayland session. Switched geometry a lot, couldn't reproduce the crash.
Ran on my desktop session, seemed the same as before

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.
davidedmundson created this revision.Sep 20 2017, 3:18 PM
Restricted Application added a project: KWin. · View Herald TranscriptSep 20 2017, 3:18 PM
Restricted Application added subscribers: KWin, kwin, plasma-devel. · View Herald Transcript
graesslin requested changes to this revision.Sep 20 2017, 3:23 PM
graesslin added a subscriber: graesslin.
graesslin added inline comments.
scene_opengl.cpp
1191–1194

This combination also looks dangerous. If the context was already current it might disable it for another user inside KWin.

This revision now requires changes to proceed.Sep 20 2017, 3:23 PM
davidedmundson added inline comments.Sep 20 2017, 3:36 PM
scene_opengl.cpp
1191–1194

changed is called from a timer, so we can be pretty confident that we're called mid-doing anything else, but I'm happy to do something else?

to be clear are you suggesting we:

  • makeOpenGLContextCurrent() but then don't call doneOpenGLContextCurrent?

or

  • set a flag, and delete it next perfectPaintWindow

or

  • something completely different

Note that I have this same pattern in my BlurEffect mod I phab'd yesterday, so I'll make that follow this.

graesslin added inline comments.Sep 20 2017, 3:47 PM
scene_opengl.cpp
1191–1194

best would be if we knew that the context is current. But that's unfortunately wishful thinking.

My suggestion would be to do your option 1:

makeOpenGLContextCurrent() but then don't call doneOpenGLContextCurrent?

In the good old days of pre-Qt5 KWin made the context current exactly once and then never called done. This changed with QtQuick also making a context current. So IIRC we can just make it current but don't have to call done. But I'm not 100 % sure.

davidedmundson added inline comments.Sep 20 2017, 4:01 PM
scene_opengl.cpp
1191–1194

There's a few parts of EffectsHandlerImp that calls make() but not done() in response to non-render related things.

Seems to be an established safe pattern.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptSep 20 2017, 4:02 PM
graesslin accepted this revision.Sep 20 2017, 4:43 PM

Feel free to push either of the two revisions.

This revision is now accepted and ready to land.Sep 20 2017, 4:43 PM
This revision was automatically updated to reflect the committed changes.