Make sure that effect windows outlive effects

Summary:
Compositing is suspended/finished in a very hard way fashion, effect
windows are destroyed without notifying effects about it.

AnimationEffect tries gracefully release deleted windows, but because
in some cases(like when suspending compositing) a deleted window can
be already destroyed, a segmentation fault can happen.

This change adjusts the order in which effect windows and effects are
destroyed, so AnimationEffect (and other effects) cannot access dangling
pointers.

BUG: 400788
FIXED-IN: 5.15.0

Reviewers: #kwin, graesslin

Reviewed By: #kwin, graesslin

Subscribers: graesslin, kwin

Tags: #kwin

Differential Revision: https://phabricator.kde.org/D17311

autotests/integration/CMakeLists.txt

@@ -57,6 +57,7 @@ integrationTest(WAYLAND_ONLY NAME testShellClientRules SRCS shell_client_rules_t
 integrationTest(WAYLAND_ONLY NAME testIdleInhibition SRCS idle_inhibition_test.cpp)
 integrationTest(WAYLAND_ONLY NAME testColorCorrectNightColor SRCS colorcorrect_nightcolor_test.cpp)
 integrationTest(WAYLAND_ONLY NAME testDontCrashCursorPhysicalSizeEmpty SRCS dont_crash_cursor_physical_size_empty.cpp)
+integrationTest(WAYLAND_ONLY NAME testDontCrashReinitializeCompositor SRCS dont_crash_reinitialize_compositor.cpp)
 
 if (XCB_ICCCM_FOUND)
     integrationTest(NAME testMoveResize SRCS move_resize_window_test.cpp LIBS XCB::ICCCM)

autotests/integration/dont_crash_reinitialize_compositor.cpp

@@ -0,0 +1,150 @@
+/********************************************************************
+KWin - the KDE window manager
+This file is part of the KDE project.
+
+Copyright (C) 2018 Vlad Zagorodniy <vladzzag@gmail.com>
+
+This program is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 2 of the License, or
+(at your option) any later version.
+
+This program is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*********************************************************************/
+
+#include "kwin_wayland_test.h"
+
+#include "abstract_client.h"
+#include "composite.h"
+#include "deleted.h"
+#include "effects.h"
+#include "platform.h"
+#include "screens.h"
+#include "shell_client.h"
+#include "wayland_server.h"
+#include "workspace.h"
+
+#include <KWayland/Client/surface.h>
+#include <KWayland/Client/xdgshell.h>
+
+namespace KWin
+{
+
+static const QString s_socketName = QStringLiteral("wayland_test_kwin_dont_crash_reinitialize_compositor-0");
+
+class DontCrashReinitializeCompositorTest : public QObject
+{
+    Q_OBJECT
+
+private Q_SLOTS:
+    void initTestCase();
+    void init();
+    void cleanup();
+
+    void testReinitializeCompositor_data();
+    void testReinitializeCompositor();
+};
+
+void DontCrashReinitializeCompositorTest::initTestCase()
+{
+    qputenv("XDG_DATA_DIRS", QCoreApplication::applicationDirPath().toUtf8());
+
+    qRegisterMetaType<KWin::AbstractClient *>();
+    qRegisterMetaType<KWin::Deleted *>();
+    qRegisterMetaType<KWin::ShellClient *>();
+    QSignalSpy workspaceCreatedSpy(kwinApp(), &Application::workspaceCreated);
+    QVERIFY(workspaceCreatedSpy.isValid());
+    kwinApp()->platform()->setInitialWindowSize(QSize(1280, 1024));
+    QMetaObject::invokeMethod(kwinApp()->platform(), "setVirtualOutputs", Qt::DirectConnection, Q_ARG(int, 2));
+    QVERIFY(waylandServer()->init(s_socketName.toLocal8Bit()));
+
+    qputenv("KWIN_COMPOSE", QByteArrayLiteral("O2"));
+    qputenv("KWIN_EFFECTS_FORCE_ANIMATIONS", QByteArrayLiteral("1"));
+
+    kwinApp()->start();
+    QVERIFY(workspaceCreatedSpy.wait());
+    QCOMPARE(screens()->count(), 2);
+    QCOMPARE(screens()->geometry(0), QRect(0, 0, 1280, 1024));
+    QCOMPARE(screens()->geometry(1), QRect(1280, 0, 1280, 1024));
+    waylandServer()->initWorkspace();
+}
+
+void DontCrashReinitializeCompositorTest::init()
+{
+    QVERIFY(Test::setupWaylandConnection());
+}
+
+void DontCrashReinitializeCompositorTest::cleanup()
+{
+    Test::destroyWaylandConnection();
+}
+
+void DontCrashReinitializeCompositorTest::testReinitializeCompositor_data()
+{
+    QTest::addColumn<QString>("effectName");
+
+    QTest::newRow("Fade")   << QStringLiteral("kwin4_effect_fade");
+    QTest::newRow("Glide")  << QStringLiteral("glide");
+    QTest::newRow("Scale")  << QStringLiteral("kwin4_effect_scale");
+}
+
+void DontCrashReinitializeCompositorTest::testReinitializeCompositor()
+{
+    // This test verifies that KWin doesn't crash when the compositor settings
+    // have been changed while a scripted effect animates the disappearing of
+    // a window.
+
+    // Make sure that we have the right effects ptr.
+    auto effectsImpl = qobject_cast<EffectsHandlerImpl *>(effects);
+    QVERIFY(effectsImpl);
+
+    // Unload all effects.
+    effectsImpl->unloadAllEffects();
+    QVERIFY(effectsImpl->loadedEffects().isEmpty());
+
+    // Create the test client.
+    using namespace KWayland::Client;
+
+    QScopedPointer<Surface> surface(Test::createSurface());
+    QVERIFY(!surface.isNull());
+    QScopedPointer<XdgShellSurface> shellSurface(Test::createXdgShellStableSurface(surface.data()));
+    QVERIFY(!shellSurface.isNull());
+    ShellClient *client = Test::renderAndWaitForShown(surface.data(), QSize(100, 50), Qt::blue);
+    QVERIFY(client);
+
+    // Make sure that only the test effect is loaded.
+    QFETCH(QString, effectName);
+    QVERIFY(effectsImpl->loadEffect(effectName));
+    QCOMPARE(effectsImpl->loadedEffects().count(), 1);
+    QCOMPARE(effectsImpl->loadedEffects().first(), effectName);
+    Effect *effect = effectsImpl->findEffect(effectName);
+    QVERIFY(effect);
+    QVERIFY(!effect->isActive());
+
+    // Close the test client.
+    QSignalSpy windowClosedSpy(client, &ShellClient::windowClosed);
+    QVERIFY(windowClosedSpy.isValid());
+    shellSurface.reset();
+    surface.reset();
+    QVERIFY(windowClosedSpy.wait());
+
+    // The test effect should start animating the test client. Is there a better
+    // way to verify that the test effect actually animates the test client?
+    QVERIFY(effect->isActive());
+
+    // Re-initialize the compositor, effects will be destroyed and created again.
+    Compositor::self()->slotReinitialize();
+
+    // By this time, KWin should still be alive.
+}
+
+} // namespace KWin
+
+WAYLANDTEST_MAIN(KWin::DontCrashReinitializeCompositorTest)
+#include "dont_crash_reinitialize_compositor.moc"

composite.cpp

@@ -368,6 +368,13 @@ void Compositor::finish()
         return;
     m_finishing = true;
     m_releaseSelectionTimer.start();
+
+    // Some effects might need access to effect windows when they are about to
+    // be destroyed, for example to unreference deleted windows, so we have to
+    // make sure that effect windows outlive effects.
+    delete effects;
+    effects = nullptr;
+
     if (Workspace::self()) {
         foreach (Client * c, Workspace::self()->clientList())
             m_scene->windowClosed(c, NULL);
@@ -403,8 +410,6 @@ void Compositor::finish()
             c->finishCompositing();
         }
     }
-    delete effects;
-    effects = NULL;
     delete m_scene;
     m_scene = NULL;
     compositeTimer.stop();

effects.cpp

@@ -1654,6 +1654,19 @@ KSharedConfigPtr EffectsHandlerImpl::inputConfig() const
     return kwinApp()->inputConfig();
 }
 
+Effect *EffectsHandlerImpl::findEffect(const QString &name) const
+{
+    auto it = std::find_if(loaded_effects.constBegin(), loaded_effects.constEnd(),
+        [name] (const EffectPair &pair) {
+            return pair.first == name;
+        }
+    );
+    if (it == loaded_effects.constEnd()) {
+        return nullptr;
+    }
+    return (*it).second;
+}
+
 //****************************************
 // EffectWindowImpl
 //****************************************

effects.h

@@ -262,6 +262,15 @@ public:
 
     void windowToDesktops(EffectWindow *w, const QVector<uint> &desktops);
 
+    /**
+     * Finds an effect with the given name.
+     *
+     * @param name The name of the effect.
+     * @returns The effect with the given name @p name, or nullptr if there
+     *     is no such effect loaded.
+     **/
+    Effect *findEffect(const QString &name) const;
+
 public Q_SLOTS:
     void slotCurrentTabAboutToChange(EffectWindow* from, EffectWindow* to);
     void slotTabAdded(EffectWindow* from, EffectWindow* to);