Span wayland objects to lifespan of the QApplication
ClosedPublic

Authored by davidedmundson on Jun 24 2019, 4:51 PM.

Details

Summary

We must release all wayland objects before the QPA connection is closed.

We used to do this explicitly, but this still left an awkward code path
where we could still try to recreate a BlurManager if it's called after
the QApplication is closed.

Instead we can scope all the wayland objects to the QApplication. The
objects themselves were not leaked as public API so this is safe. Calls
after this will simply no-op.

This fixes crashes on tear down of plasma.

BUG: 372789

Test Plan

kquitapp5 plasmashell

Diff Detail

Repository
R130 Frameworks integration plugin using KWayland
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.Jun 24 2019, 4:51 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 24 2019, 4:51 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidedmundson requested review of this revision.Jun 24 2019, 4:51 PM
zzag added a subscriber: zzag.EditedJun 24 2019, 5:09 PM

Is there any reason for not making WaylandIntegration a child of qApp? (I don't have full insight into the problem)

anthonyfieroni added inline comments.
src/windowsystem/waylandintegration.cpp
95–99

Since qApp is the parent are they needed? When destroyed qApp will call delete on m_waylandBlurManager, no?

anthonyfieroni added inline comments.Jun 24 2019, 5:11 PM
src/windowsystem/waylandintegration.cpp
95–99

Or that's when removed during the normal execution?

This powers the plugin backend behind KWindowSystem.
That consists of static methods that can be invoked at any time. We can't change that API.

Scoping WaylandIntegration would absolutely work but it then means putting guards over all the users of it. (windoweffects.cpp / windowsystem.cpp etc) whereas we already have guards for the individual proxies as the compositor could remove the interfaces at runtime.

src/windowsystem/waylandintegration.cpp
95–99

Yeah, exactly that.

davidedmundson edited the summary of this revision. (Show Details)Jun 25 2019, 10:10 AM

Confirmed that this fixes 372789 for my Wayland session!

apol added a subscriber: apol.Jun 25 2019, 2:11 PM

+1
Really cool! this fixes the crash on closing plasmashell!

zzag added a comment.Jun 25 2019, 2:49 PM

Hmm, okay, I see what the problem is.

The proposed patch seems to be reasonable, but I don't like the new object tree. We could fix this bug by introducing m_isTerminating, e.g.

// In setupKWaylandIntegration.
connect(qApp, &QCoreApplication::aboutToQuick, this, [] {
    // ...
    m_registry->release();
    m_isTerminating = true;
});

// In each proxy getter.

// A deep touching comment that explains why this proxy is guarded.
if (m_isTerminating) {
    return nullptr;
}

these two approaches are inter-changeable, but solution with m_isTerminating has a bit nicer object tree.

In either case, this patch is good to go. I have just one question.

src/windowsystem/waylandintegration.cpp
125

Is it safe to drop this line? What was the reasoning behind this change?

apol added inline comments.Jun 25 2019, 10:13 PM
src/windowsystem/waylandintegration.cpp
125

Yes, note it's a QPointer now.

zzag added inline comments.Jun 26 2019, 11:28 AM
src/windowsystem/waylandintegration.cpp
125

Is it okay to access waylandBlurManager() right after it's been deleted (or rather scheduled for removal)?

davidedmundson added inline comments.Jun 26 2019, 7:39 PM
src/windowsystem/waylandintegration.cpp
125

There's two questions here. I'll answer both.


Is it safe to call a method on BlurManager after the registry removes it?

It should be.

Typically protocols works as follows:

  • server tells the client we're removing an interface

(it should then stops listening for events on that interface) and the registry removes it

  • client calls release on the object
  • only then does the wayland object get removed

That way we're race free.

Not all wayland globals behave quite the same, because that would be far too convenient!


If someone uses it after the QApp destruction could the QPA have got deleted in the meantime (and hit the original bug)

For any normal signal, maybe. But QApp is special QCoreApplication::execCleanup happens before QGuiApp destructor which removes the

zzag accepted this revision.Jun 27 2019, 8:27 AM
This revision is now accepted and ready to land.Jun 27 2019, 8:27 AM
This revision was automatically updated to reflect the committed changes.