[x11] Fix crash during tear down
ClosedPublic

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

Details

Summary

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

For example, let's consider the following two classes

class Base {
public:
    Base() { foo()->bar(); }
    virtual ~Base() { foo()->bar(); }

    virtual Foo* foo() const { return nullptr; }
};

class Derived : public Base {
public:
    Derived() : mFoo(new Foo) {}
    ~Derived() override { delete mFoo; }

    Foo* foo() const override { return mFoo; }

private:
    Foo* mFoo;
};

When an instance of Derived class is created, constructors will run in
the following order:

Base()
Derived()

It's not safe to dispatch foo() method call to Derived class because
constructor of Derived hasn't initialized yet mFoo.

Same story with destructors, they'll run in the following order:

~Derived()
~Base()

It's not safe to dispatch foo() method call in the destructor of Base
class to Derived class because mFoo was deleted.

So, what does that weird C++ behavior has something to do with KWin? Well,
recently Compositor class was split into two classes - WaylandCompositor,
and X11Compositor. Some functionality from X11 doesn't make sense on
Wayland. Therefore methods that implement that stuff were "purified," i.e.
they became pure virtual methods. Unfortunately, when Compositor tears
down it may call pure virtual methods on itself. Given that those calls
cannot be dispatched to X11Compositor or WaylandCompositor, the only
choice that C++ runtime has is to throw an exception.

The fix for this very delicate problem is very simple - do not call virtual
methods from constructors and the destructor. Avoid doing that if you can!

This change moves Compositor::updateClientCompositeBlocking to X11Compositor
so it longer has to be a virtual method. Also, it kind of doesn't make sense
to keep it in base Compositor class because compositing can be blocked only
on X11.

BUG: 411049

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 14991
Build 15009: arc lint + arc unit
zzag created this revision.Aug 11 2019, 3:22 PM
Restricted Application added a project: KWin. · View Herald TranscriptAug 11 2019, 3:22 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Aug 11 2019, 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.Aug 11 2019, 3:32 PM
zzag removed a subscriber: romangg.

Argh, we still need some X11 bits in WaylandCompositor.

zzag added a subscriber: romangg.Aug 11 2019, 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.EditedAug 11 2019, 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.EditedAug 11 2019, 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.Aug 11 2019, 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.Aug 11 2019, 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.Aug 11 2019, 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.Aug 11 2019, 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.

zzag updated this revision to Diff 65009.Fri, Aug 30, 4:50 PM
zzag retitled this revision from Don't crash when X11Compositor tears down to [x11] Fix crash during tear down.
zzag edited the summary of this revision. (Show Details)
zzag removed a subscriber: romangg.

Different approach.

zzag edited the summary of this revision. (Show Details)Fri, Aug 30, 4:52 PM
zzag edited the summary of this revision. (Show Details)
zzag planned changes to this revision.EditedFri, Aug 30, 5:06 PM

argh, checking isActive in X11Compositor::isActive is incorrect. I'll remove that check altogether

zzag updated this revision to Diff 65010.Fri, Aug 30, 5:10 PM

Remove isActive() check in X11Compositor::updateClientCompositeBlocking().

It's safe to call updateClientCompositeBlocking() during tear down as
calls to suspend() and resume() are queued and X11Compositor will be
removed before returning to the event loop.

zzag edited the summary of this revision. (Show Details)Fri, Aug 30, 5:19 PM
anthonyfieroni added inline comments.
composite.cpp
1078

qobject_cast is slower in some cases can you do it

static QPointer<X11Compositor> x11;
if (!x11) {
    x11 = qobject_cast<X11Compositor *>(Compositor::self());
}
return x11;
zzag added inline comments.Fri, Aug 30, 6:23 PM
composite.cpp
1078

qobject_cast is slower in some cases

When?

zzag added inline comments.Fri, Aug 30, 6:26 PM
composite.cpp
1078

Also static in a method or a function means taking a lock on a mutex... Performance impact won't be huge, but still...

anthonyfieroni added inline comments.Fri, Aug 30, 7:38 PM
composite.cpp
1078

Being at construction time is no-op, cannot compare to qt cast. At least one commit in this repo is removing of qobject_cast being slow in drawing.

zzag added inline comments.Fri, Aug 30, 7:51 PM
composite.cpp
1078

Which commit?

zzag edited the summary of this revision. (Show Details)Fri, Aug 30, 8:58 PM
zzag edited the summary of this revision. (Show Details)Fri, Aug 30, 9:08 PM
romangg accepted this revision.Fri, Aug 30, 10:09 PM

That's a nice solution. On Wayland we spare us the additional connections and we don't need the boiler plate virtual function in Compositor class.

composite.cpp
1078

I can't imagine it making a big difference. For now we only need it in two setup functions being called not regularly. But do you have some data on the difference in performance? If it's called more often in the future it might be an idea to benchmark it.

This revision is now accepted and ready to land.Fri, Aug 30, 10:09 PM
This revision was automatically updated to reflect the committed changes.