fix: normal termination for Treeland#731
Conversation
Reviewer's GuideAdjusts scene graph and renderer cleanup, XWayland teardown, and Treeland shutdown sequencing to ensure normal termination and safer resource destruction, plus adds a temporary emergency restart keybinding. Sequence diagram for scene graph invalidation cleanupsequenceDiagram
participant QQuickWindow
participant WBufferRenderer
participant RhiManager
participant QSGRenderer
%% WBufferRenderer connection
QQuickWindow->>WBufferRenderer: sceneGraphInvalidated
activate WBufferRenderer
WBufferRenderer->>WBufferRenderer: invalidateSceneGraph()
WBufferRenderer->>WBufferRenderer: m_textureProvider.reset()
loop for each Source in m_sourceList
WBufferRenderer->>QSGRenderer: delete Source.renderer
WBufferRenderer->>WBufferRenderer: Source.renderer = nullptr
end
deactivate WBufferRenderer
%% RhiManager connection
QQuickWindow->>RhiManager: sceneGraphInvalidated
activate RhiManager
RhiManager->>RhiManager: delete this
deactivate RhiManager
destroy RhiManager
RhiManager-->>QSGRenderer: ~RhiManager()
destroy QSGRenderer
Sequence diagram for WXWayland destruction and XWayland teardownsequenceDiagram
participant Session
participant Helper
participant ShellHandler
participant WXWayland
participant QWXWayland
participant wlr_xwayland
Session->>Session: ~Session()
alt Helper instance exists
Session->>Helper: Helper::instance()
Helper-->>Session: pointer
Session->>Helper: shellHandler()
Helper-->>Session: ShellHandler*
Session->>ShellHandler: removeXWayland(m_xwayland)
ShellHandler->>WXWayland: destroy(WServer*)
WXWayland->>QWXWayland: handle()
WXWayland->>QWXWayland: destroy()
QWXWayland->>wlr_xwayland: wlr_xwayland_destroy()
WXWayland->>WXWayland: clear surfaceList
WXWayland-->>ShellHandler: destruction complete
ShellHandler-->>Session: WXWayland removed
Session->>Session: m_xwayland = nullptr
else Helper instance is null
Session->>Session: m_xwayland kept for WServer cleanup
Session->>Session: m_xwayland = nullptr
end
Updated class diagram for renderer, XWayland, and shutdown managementclassDiagram
class WBufferRenderer {
+WBufferRenderer(QQuickItem* parent)
+~WBufferRenderer()
+invalidateSceneGraph() void
+releaseResources() void
-cleanTextureProvider() void
-resetSources() void
-renderWindow() WOutputRenderWindow*
-outputWindow() WOutputRenderWindow*
-m_textureProvider
-m_sourceList
}
class RhiManager {
+RhiManager(QQuickWindow* owner)
+~RhiManager()
-renderer QSGRenderer*
-isBatchRenderer bool
}
class Helper {
+Helper(QObject* parent)
+~Helper()
+beforeDisposeEvent(WSeat* seat, QWindow* window, QInputEvent* event) bool
+addSocket(WSocket* socket) void
+createXWayland() WXWayland*
+personalization() PersonalizationV1*
+shellHandler() ShellHandler*
+static instance() Helper*
-m_instance static Helper*
+requestQuit()
}
class Session {
+Session(QObject* parent)
+~Session()
+id() int
-m_xwayland WXWayland*
-m_socket WSocket*
-m_settingManager
}
class WXWayland {
+create(WServer* server) void
+destroy(WServer* server) void
-surfaceList
-handle() QWXWayland*
}
class QWXWayland {
+get_xwm_connection() xcb_connection_t*
+destroy() void
}
class Treeland {
+quit() void
}
class TreelandPrivate {
+~TreelandPrivate()
-qmlEngine QQmlEngine*
-pluginTs
}
class QQuickWindow
class QQuickItem
class WOutputRenderWindow
class QSGRenderer
class WSeat
class QWindow
class QInputEvent
class WSocket
class ShellHandler
class WServer
class PersonalizationV1
WBufferRenderer --> QQuickItem : inherits
WBufferRenderer --> QQuickWindow : window()
WBufferRenderer --> WOutputRenderWindow : renderWindow()
WBufferRenderer --> QSGRenderer : uses in sources
RhiManager --> QQuickWindow : owner
RhiManager --> QSGRenderer : renderer
Helper --> ShellHandler : owns
Helper --> WSocket : manages
Helper --> WXWayland : factory
Session --> WXWayland : m_xwayland
Session --> Helper : uses instance
Session --> WSocket : m_socket
WXWayland --> QWXWayland : handle
WXWayland --> WServer : create, destroy
QWXWayland --> wlr_xwayland : wraps
Treeland --> TreelandPrivate : holds d_ptr
TreelandPrivate --> QQmlEngine : qmlEngine
TreelandPrivate --> UserModel : singleton
Helper ..> Treeland : requestQuit signal
QQuickWindow ..> WBufferRenderer : sceneGraphInvalidated signal
QQuickWindow ..> RhiManager : sceneGraphInvalidated signal
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The direct
connect(window(), &QQuickWindow::sceneGraphInvalidated, ...)inWBufferRenderer's constructor riskswindow()being null at construction time (as your own TODO notes); consider wiring this up viawindowChanged,componentComplete, or a lazy one-shot connection when a non-null window is first available. - Unconditionally calling
std::terminate()at the end ofTreeland::quit()forces an abnormal shutdown and bypasses normal object destruction; it might be safer to guard this behind a debug flag or dedicated emergency path so that non-debug quits can still follow a regular teardown sequence.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The direct `connect(window(), &QQuickWindow::sceneGraphInvalidated, ...)` in `WBufferRenderer`'s constructor risks `window()` being null at construction time (as your own TODO notes); consider wiring this up via `windowChanged`, `componentComplete`, or a lazy one-shot connection when a non-null window is first available.
- Unconditionally calling `std::terminate()` at the end of `Treeland::quit()` forces an abnormal shutdown and bypasses normal object destruction; it might be safer to guard this behind a debug flag or dedicated emergency path so that non-debug quits can still follow a regular teardown sequence.
## Individual Comments
### Comment 1
<location> `waylib/src/server/qtquick/private/wbufferrenderer.cpp:90-91` </location>
<code_context>
-
+ // ensure graphical resources are deleted when scene graph is invalidated
+ // TODO: WBufferRenderer created through WOutputViewport has window() == nullptr?
+ connect(window(), &QQuickWindow::sceneGraphInvalidated,
+ this, &WBufferRenderer::invalidateSceneGraph,
+ Qt::DirectConnection);
}
</code_context>
<issue_to_address>
**issue:** Connecting to window() once in the ctor can miss later window changes or operate on nullptr.
This assumes the item already has a permanent window. For items constructed before being added to a window, `connect(window(), …)` is effectively a no-op, so `sceneGraphInvalidated` will never be received. If the item later moves to a different window, the connection will still target the original one. Instead, listen to `QQuickItem::windowChanged` and (dis)connect `sceneGraphInvalidated` when the window becomes available or changes.
</issue_to_address>
### Comment 2
<location> `src/core/treeland.cpp:106-107` </location>
<code_context>
it->second->deleteLater();
pluginTs.erase(it++);
}
+ delete qmlEngine->singletonInstance<UserModel *>("Treeland", "UserModel");
+ qmlEngine->clearSingletons();
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Manually deleting a QML singleton instance is likely a double-free; ownership belongs to the engine.
`singletonInstance<T>()` objects are owned and destroyed by the QML engine (e.g. during teardown or `clearSingletons()`). Deleting this pointer here and then calling `clearSingletons()` can cause a double delete or other lifetime issues if the engine still tracks it. Please avoid deleting it directly; rely on `clearSingletons()` or change the singleton registration so the engine does not own the instance if you need manual control.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Improve shutdown/termination stability by explicitly releasing Qt Quick scene graph resources on invalidation, fixing XWayland destruction ownership, and adjusting Treeland teardown ordering (with a temporary emergency quit key).
中文:通过在 scene graph 失效时显式释放 Qt Quick 图形资源、修正 XWayland 的销毁所有权与调用路径,并调整 Treeland 的销毁顺序,来提升退出/销毁阶段的稳定性(并临时加入紧急退出快捷键)。
Changes:
- Release Qt Quick renderers/texture providers on
QQuickWindow::sceneGraphInvalidatedand fixWBufferRendererwindow lookup. - Expose and call XWayland
destroy()from the compositor side; adjust Session/XWayland cleanup path. - Adjust Helper/Treeland destruction timing and add a temporary Meta+F12 emergency quit path.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
waylib/src/server/qtquick/private/wrenderbuffernode.cpp |
Attempt to destroy RhiManager on scene graph invalidation to release QSGRenderer earlier. |
waylib/src/server/qtquick/private/wbufferrenderer_p.h |
Use window() instead of parent() to obtain the WOutputRenderWindow. |
waylib/src/server/qtquick/private/wbufferrenderer.cpp |
Connect invalidation handler and free texture providers/renderers on scene graph invalidation. |
waylib/src/server/protocols/wxwayland.cpp |
Explicitly destroy underlying XWayland handle during teardown. |
qwlroots/src/types/qwxwayland.h |
Expose qw_xwayland::destroy() binding. |
src/session/session.cpp |
Make Session teardown resilient when Helper is already destroyed. |
src/seat/helper.h |
Remove Helper::removeXWayland declaration (cleanup now via ShellHandler). |
src/seat/helper.cpp |
Clear Helper::m_instance earlier during destruction; add Meta+F12 quit signal emission. |
src/core/treeland.cpp |
Adjust singleton teardown and force-terminate on quit. |
3847114 to
c05e744
Compare
|
TAG Bot New tag: 0.8.3 |
| // ensure graphical resources are released before scene graph is invalidated | ||
| QMetaObject::Connection windowConn; | ||
| if (window()) | ||
| windowConn = connect(window(), &QQuickWindow::sceneGraphInvalidated, this, &WBufferRenderer::invalidateSceneGraph); |
There was a problem hiding this comment.
应该不需要,这个slot会在QQuickWindowPrivate::cleanupNodesOnShutdown种被主动调用。
There was a problem hiding this comment.
需要的,它那个自动调用并不稳定
5ce0523 to
d4605dc
Compare
| // ensure graphical resources are released before scene graph is invalidated | ||
| QMetaObject::Connection windowConn; | ||
| if (window()) | ||
| windowConn = connect(window(), &QQuickWindow::sceneGraphInvalidated, this, &WBufferRenderer::invalidateSceneGraph); |
Changes: - RhiManager: destruct on `QQuickWindow::sceneGraphInvalidated`: RhiManager holds a `QSGRenderer*` which must be released before scene graph invalidation. RhiManager itself cannot function without that renderer. - WBufferRenderer: instance method `outputWindow()` should use `window()` instead of `parent()`, as its parents are generally not of class `WOutputRenderWindow`. - WBufferRenderer: release texture providers and renderers upon scene graph invalidation. - WBufferRenderer: explicitly connect slot `invalidateSceneGraph` to `QQuickWindow::sceneGraphInvalidated`, as Qt auto discovery can fail in practice.
`wlr_xwayland` is a compositor owned object, and thus should be destroyed compositor-side.
WXWayland instances are managed by `ShellHandler`, and should be deleted by it if possible. Otherwise, let WServer clean up. The underlying wlr_xwayland object for XWayland instances must be explicitly destroyed through wlr_xwayland_destroy, to clean up wayland event listensers of wlr_xwayland_shell_v1.
`Helper::m_instance` should be cleared immidiately when `Helper` begin destruction. Destroy UserModel before Helper to clean user sessions and surfaces, for their destruction often requires assistence of Helper. Destroy Helper before QmlEngine, as `SurfaceWrapper::createNewOrClose` requires a functional engine. Miscellaneous: cleaned unimplemented instance method `Helper::removeXWayland`.
Bring back the hard-coded Meta+F12 to crash Treeland, only for debug builds. Remapped the default normal quitting keybind to Meta+Shift+Q
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: misaka18931, zccrs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
fix: waylib: release renderers before scene graph invalidation
Changes:
QQuickWindow::sceneGraphInvalidated:RhiManager holds a
QSGRenderer*which must be released before scenegraph invalidation. RhiManager itself cannot function without that
renderer.
outputWindow()should usewindow()instead ofparent(), as its parents are generally not ofclass
WOutputRenderWindow.graph invalidation.
invalidateSceneGraphtoQQuickWindow::sceneGraphInvalidated, as Qt auto discovery can failin practice.
fix: qwlroots: expose qw_xwayland::destroy()
wlr_xwaylandis a compositor owned object, and thus should bedestroyed compositor-side.
fix: SessionManager: correct destruction routine of WXWayland instances
WXWayland instances are managed by
ShellHandler, and should be deletedby it if possible. Otherwise, let WServer clean up.
The underlying wlr_xwayland object for XWayland instances must be
explicitly destroyed through wlr_xwayland_destroy, to clean up wayland
event listensers of wlr_xwayland_shell_v1.
fix: Treeland destruction timing
Helper::m_instanceshould be cleared immidiately whenHelperbegindestruction.
Destroy UserModel before Helper to clean user sessions and surfaces,
for their destruction often requires assistence of Helper.
Destroy Helper before QmlEngine, as
SurfaceWrapper::createNewOrCloserequires a functional engine.
Miscellaneous: cleaned unimplemented instance method
Helper::removeXWayland.fix: temp: emergency restart key
Bring back the hard-coded Meta+F12 to crash Treeland, for debug purpose.
Currently treeland & ddm cannot handle quitting qwq.
Even a successful normal termination of Treeland leave the outputs with
the last rendered frame.
This should be reverted after a rework of the termination sequence.
TODO
Currently, some
qw_buffers aren't properly destroyed, which can cause assertion failures, if new windows are opened before quitting.Summary by Sourcery
Ensure Treeland and its Wayland/XWayland integrations shut down cleanly by fixing destruction order, resource release, and adding a temporary emergency restart shortcut.
New Features:
Bug Fixes:
Enhancements: