Don't crash when X11Compositor tears down
Needs ReviewPublic

Authored by zzag on Sun, Aug 11, 3:22 PM.

Details

Reviewers
None
Group Reviewers
KWin
Summary

Compositor::stop() cannot be called from destructor of the Compositor
class because Workspace may call Compositor::updateCompositeBlocking()
when an instance of Deleted is removed/discarded.

Another solution is to call stop() method before destroying the compositor
object, but that's against RAII.

Test Plan

KWin no longer crashes when running kwin_x11 --replace command.

Diff Detail

Repository
R108 KWin
Branch
dont-call-pure-virtual-method
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 14993
Build 15011: arc lint + arc unit
zzag created this revision.Sun, Aug 11, 3:22 PM
Restricted Application added a project: KWin. · View Herald TranscriptSun, Aug 11, 3:22 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Sun, Aug 11, 3:22 PM

Why does it work with the other destructor then? Destructors are called one after the other, right?

zzag updated this revision to Diff 63552.Sun, Aug 11, 3:32 PM
zzag removed a subscriber: romangg.

Argh, we still need some X11 bits in WaylandCompositor.

zzag added a subscriber: romangg.Sun, Aug 11, 3:40 PM

Why does it work with the other destructor then? Destructors are called one after the other, right?

See section 15.7 [class.cdtor] in the C++ standard.

How about just telling me?

zzag added a comment.EditedSun, Aug 11, 3:49 PM

How about just telling me?

Any call made to a virtual method in constructor/destructor of the base class won't go to a derived class because that method may access either uninitialized or destroyed resources.

romangg added a comment.EditedSun, Aug 11, 3:54 PM
In D23098#510408, @zzag wrote:

How about just telling me?

Any call made to a virtual method in constructor/destructor of the base class won't go to a derived class because that method may access either uninitialized or destroyed resources.

Thanks, makes sense. Then just add a default implementation in base class doing nothing? Or will it then still try to access the overriding subclass method?

zzag added a comment.Sun, Aug 11, 4:20 PM

Thanks, makes sense. Then just add a default implementation in base class doing nothing? Or will it then still try to access the overriding subclass method?

It doesn't matter whether default implementation is provided. All updateCompositingBlocked() method calls will be dispatched to Compositor, not X11Compositor or WaylandCompositor.

In D23098#510410, @zzag wrote:

Thanks, makes sense. Then just add a default implementation in base class doing nothing? Or will it then still try to access the overriding subclass method?

It doesn't matter whether default implementation is provided. All updateCompositingBlocked() method calls will be dispatched to Compositor, not X11Compositor or WaylandCompositor.

I thought the root problem is that updateCompositeBlocking is pure virtual in Compositor and so not defined after child has been destroyed. Make it only virtual with a default implementation. Then on destroy updateCompositeBlocking calls will go to Compositor's default implementation and we won't crash. Something wrong with this logic?

zzag added a comment.Sun, Aug 11, 5:55 PM

Yes, it will work.

I don't like this solution though because the code will be more difficult to reason about, e.g. calls to updateCompositeBlocked usually go to X11Compositor, but sometimes they may go somewhere else (Compositor).

It makes sense on tear down to not process any updateCompositeBlocking calls any more since compositing won't be enabled any more.

zzag added a comment.Sun, Aug 11, 6:19 PM

It makes sense on tear down to not process any updateCompositeBlocking calls any more since compositing won't be enabled any more.

This is out of scope of this patch.

Replacing = 0 with {} in the header file is out of scope?

zzag added a comment.Sun, Aug 11, 6:22 PM

Replacing = 0 with {} in the header file is out of scope?

In D23098#510426, @zzag wrote:

Yes, it will work.

I don't like this solution though because the code will be more difficult to reason about, e.g. calls to updateCompositeBlocked usually go to X11Compositor, but sometimes they may go somewhere else (Compositor).

Are you doing this deliberately? I said on tear down it makes sense to not process the calls to updateCompositeBlocking...

since compositing won't be enabled any more.

Which exactly happens when the default implementation does nothing. So there is nothing unreasonable about it.