<table><tr><td style="">zzag updated this revision to Diff 65009.<br />zzag retitled this revision from "Don't crash when X11Compositor tears down" to "[x11] Fix crash during tear down".<br />zzag removed a subscriber: romangg.<br />zzag edited the summary of this revision. <a href="https://phabricator.kde.org/transactions/detail/PHID-XACT-DREV-43p7bxqdt6ggl3r/">(Show Details)</a><br />zzag added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D23098">View Revision</a></tr></table><br /><div><div><p>Different approach.</p></div></div><br /><div><strong>CHANGES TO REVISION SUMMARY</strong><div><div style="white-space: pre-wrap; color: #74777D;"><span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">Compositor::stop() cannot be called from destructor of the Compositor</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">Any call made to a virtual method in constructor/destructor of a base<br />
class won't go to a derived class. The reason for that is protect the<br />
base class from accessing either deleted or uninitialized resources.<br />
<br />
For example, let's consider the following two classes<br />
<br />
class Base {<br />
public:<br />
Base() { foo()->bar(); }<br />
virtual ~Base() { foo()->bar(); }<br />
<br />
virtual Foo* foo() const { return nullptr; }<br />
};<br />
<br />
class Derived : public Base {<br />
public:<br />
Derived() : mFoo(new Foo) {}<br />
~Derived() override { delete mFoo; }<br />
<br />
Foo *foo() const override { return mFoo; }<br />
<br />
private:<br />
Foo* mFoo;<br />
};<br />
<br />
When an instance of Derived class is created, constructors will run in<br />
the following order:<br />
<br />
Base()<br />
Derived()<br />
<br />
It's not safe to dispatch call to foo() method to Derived class because<br />
constructor of Derived hasn't initialized yet mFoo.<br />
<br />
Same story with destructors, they'll run in the following order:<br />
<br />
~Derived()<br />
~Base()<br />
<br />
It's not safe to dispatch call to foo() method in the destructor of Base<br />
class because mFoo was deleted.<br />
<br />
So, what does that weird C++ behavior has something to do with KWin? Well,<br />
recently Compositor class was split into two classes - WaylandCompositor,<br />
and X11Compositor. Some functionality from X11 doesn't make sense on<br />
Wayland. Therefore methods that implement that stuff were "purified," i.e.<br />
they became pure virtual methods. Unfortunately, when Compositor tears<br />
down it may call pure virtual methods on itself. Given that those calls<br />
cannot be dispatched to X11Compositor or WaylandCompositor, the only</span><br />
c<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">lass because Workspace may call Compositor::updateCompositeBlocking()</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">hoice that C++ runtime has is to throw an exception.<br />
<br />
The fix for this very delicate problem is very simple - do not call virtual</span><br />
<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">when an instance of Deleted is removed/discarded.</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">methods from constructors and the destructor. Avoid doing that if you can!</span><br />
<br />
<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">Another solution is to call stop() method before destroying the compositor</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">This change moves Compositor::updateClientCompositeBlocking to X11Compositor<br />
so it longer has to be a virtual method. Also, it kind of doesn't make sense<br />
to keep it in base Compositor class because compositing can be blocked only</span><br />
<span style="padding: 0 2px; color: #333333; background: rgba(251, 175, 175, .7);">object, but that's against RAII.</span><span style="padding: 0 2px; color: #333333; background: rgba(151, 234, 151, .6);">on X11.<br />
<br />
BUG: (TODO: there was a bug report which I cannot find)</span></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R108 KWin</div></div></div><br /><div><strong>CHANGES SINCE LAST UPDATE</strong><div><a href="https://phabricator.kde.org/D23098?vs=63552&id=65009">https://phabricator.kde.org/D23098?vs=63552&id=65009</a></div></div><br /><div><strong>BRANCH</strong><div><div>fix-crash</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D23098">https://phabricator.kde.org/D23098</a></div></div><br /><div><strong>AFFECTED FILES</strong><div><div>composite.cpp<br />
composite.h<br />
workspace.cpp</div></div></div><br /><div><strong>To: </strong>zzag, KWin<br /><strong>Cc: </strong>kwin, LeGast00n, The-Feren-OS-Dev, sbergeron, jraleigh, fbampaloukas, GB_2, mkulinski, ragreen, jackyalcine, Pitel, iodelay, crozbo, bwowk, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, hardening, romangg, jensreuterberg, abetts, sebas, apol, mart<br /></div>