[wayland] Fix teardown order
ClosedPublic

Authored by davidedmundson on Apr 7 2020, 9:36 PM.

Details

Summary

Valgrind flags an error on teardown.

EventQueue has a pointer to ConnectionThread internally
Registry has a pointer to the EventQueue internally

teardown order needs to be

Registry
EventQueue
Connection

registry was explicitly deleted before connectionthread already, we just
need to put event queue in the right place.

Test Plan

Ran kwin_wayland nested in valgrind

Diff Detail

Repository
R108 KWin
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24927
Build 24945: arc lint + arc unit
davidedmundson created this revision.Apr 7 2020, 9:36 PM
Restricted Application added a project: KWin. · View Herald TranscriptApr 7 2020, 9:36 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
davidedmundson requested review of this revision.Apr 7 2020, 9:36 PM
cblack accepted this revision.Apr 7 2020, 10:23 PM
This revision is now accepted and ready to land.Apr 7 2020, 10:23 PM
apol accepted this revision.Apr 8 2020, 10:28 AM
apol added a subscriber: apol.

Saw this warning yesterday too.

+1 thanks!

zzag added a subscriber: zzag.Apr 14 2020, 3:29 PM
zzag added inline comments.
wayland_server.cpp
640

Wouldn't it be better to destroy the event queue explicitly in destroyInternalConnection()? It would be a more robust (what if other globals need to access the event queue during tear down too?) and easier solution (a "flat" tear down sequence is much easier to understand than the one where children are destroyed together with their parent implicitly), imho.

[1] In fact, we got a bug or two because of "unexpected" (technically, it was okay; but conceptually it wasn't) tear down order of parents and their children.

zzag added inline comments.Apr 14 2020, 3:30 PM
wayland_server.cpp
640

implicitly [1])*

Wouldn't it be better to destroy the event queue explicitly in destroyInternalConnection()

I don't think it'd be better or worse.

I went with this as there's a relationship between the registry and the event queue, that we want to explicitly convey. It encapsulates the lifespan logic.
I would probably have gone with that if the event queue was a member var usable by others.

This revision was automatically updated to reflect the committed changes.