fix(xwayland): X11 apps cannot connect after treeland restart#1047
Draft
wineee wants to merge 1 commit into
Draft
fix(xwayland): X11 apps cannot connect after treeland restart#1047wineee wants to merge 1 commit into
wineee wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wineee 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 |
eff778f to
125f5b1
Compare
|
TAG Bot New tag: 0.8.13 |
ace6f3b to
64d90c0
Compare
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsures Xwayland helper and environment are correctly updated and resilient after treeland restarts, fixing invalid X11 auth cookies and orphaned processes. Sequence diagram for treeland-sd retrying XWaylandName after restartsequenceDiagram
participant treeland_sd as treeland_sd
participant Compositor1 as org_deepin_Compositor1
participant QTimer as QTimer
participant Systemd as systemd
loop retry_activateFd
treeland_sd->>Compositor1: XWaylandName
alt reply is valid
treeland_sd->>Compositor1: UpdateActivationEnvironment
treeland_sd->>Systemd: sd_notify READY=1
else reply is error
treeland_sd->>QTimer: QTimer::singleShot(500, app, activateFd)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The recursive use of
activateFdwithQTimer::singleShot(500, &app, activateFd)for the xwayland case has no retry limit or backoff, so consider adding a maximum retry count or exponential backoff to avoid an infinite retry loop if the compositor never comes up. - You're capturing
appandactivateFdby reference into a lambda that is scheduled for later execution; double-check that their lifetimes are clearly bounded bymainand that treeland-sd cannot exit while timers are still queued, or consider encapsulating this logic in an object with explicit lifetime management. - The error handling path logs
XWaylandNamefailures but then immediately retries only whenupdateFdis invalid; consider distinguishing between transient D-Bus errors and permanent failures to avoid noisy retries in cases where the service is misconfigured or unavailable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The recursive use of `activateFd` with `QTimer::singleShot(500, &app, activateFd)` for the xwayland case has no retry limit or backoff, so consider adding a maximum retry count or exponential backoff to avoid an infinite retry loop if the compositor never comes up.
- You're capturing `app` and `activateFd` by reference into a lambda that is scheduled for later execution; double-check that their lifetimes are clearly bounded by `main` and that treeland-sd cannot exit while timers are still queued, or consider encapsulating this logic in an object with explicit lifetime management.
- The error handling path logs `XWaylandName` failures but then immediately retries only when `updateFd` is invalid; consider distinguishing between transient D-Bus errors and permanent failures to avoid noisy retries in cases where the service is misconfigured or unavailable.
## Individual Comments
### Comment 1
<location path="src/systemd-socket.cpp" line_range="77-79" />
<code_context>
- auto active = [unixFileDescriptor, type, &tmpFiles](QDBusConnection connection) {
- auto activateFd = [unixFileDescriptor, type, &connection, &tmpFiles] {
+ auto active = [unixFileDescriptor, type, &tmpFiles, &app](QDBusConnection connection) {
+ std::function<void()> activateFd;
+ activateFd = [unixFileDescriptor, type, connection, &tmpFiles, &app, &activateFd] {
QDBusInterface updateFd("org.deepin.Compositor1",
"/org/deepin/Compositor1",
</code_context>
<issue_to_address>
**issue (bug_risk):** Recursive lambda captures its own std::function by reference, leading to a dangling reference when used with QTimer::singleShot.
Because the lambda captures `&activateFd`, the `std::function` stored in `activateFd` closes over a stack variable that is destroyed when `active()` returns. Any callbacks scheduled via `QTimer::singleShot` will then invoke a dangling reference, causing undefined behavior.
For safe recursion with `QTimer::singleShot`, ensure the callback does not capture stack references that outlive `active()`. For example, capture by value and move into `singleShot`, or restructure using a helper QObject/free function or a heap-allocated callable (e.g., `std::shared_ptr<std::function<void()>>`) with a lifetime that exceeds `active()`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
f22e635 to
49b103d
Compare
When treeland restarts, Xwayland generates a new MIT-MAGIC-COOKIE-1 but treeland-sd does not update the user's XAUTHORITY, causing auth failures. Replace the xwaylandDisplayChanged signal approach with a simpler retry mechanism: when treeland is not yet available on D-Bus, retry the XWaylandName query every 500ms until it succeeds. This handles both initial startup and treeland restart without requiring a new D-Bus signal. 问题 treeland 重启后,X11 应用报 Invalid MIT-MAGIC-COOKIE-1 key 无法连接 Xwayland。 原因 Xwayland 使用 MIT-MAGIC-COOKIE-1 认证。连接流程是: 1. treeland-xwayland helper 生成随机 cookie → 写入 /tmp/.xauth_:1 2. treeland-sd 通过 D-Bus 调用 XWaylandName() 读取 cookie → 写入用户的 $XAUTHORITY 文件 3. X11 客户端连接 Xwayland 时出示 cookie 进行认证 treeland 重启后,Xwayland 生成新的 cookie,但 treeland-sd 的 D-Bus 连接已断开(绑定在旧的 name owner 上),无法收到通知去更新用户的 $XAUTHORITY。导致用户环境中的 cookie 是旧的,认证失败。 修复 misc/systemd/treeland.service.in - 添加 ExecStartPre,启动前清理上次崩溃遗留的孤儿 Xwayland 进程 misc/systemd/dde-session-pre.target.wants/treeland-xwayland.service.in - 添加 Restart=on-failure,treeland-sd 进程异常退出后自动重启 src/systemd-socket.cpp - 添加重试机制:当 treeland 还未在 D-Bus 上注册时,每 500ms 重试 XWaylandName() 查询,直到成功 - 这样无论是首次启动、treeland 重启、还是 D-Bus 信号丢失,都能可靠地获取最新的 cookie 并更新 systemd 用户环境变量 ISSUE: 1071
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When treeland restarts, Xwayland generates a new MIT-MAGIC-COOKIE-1 but treeland-sd (xwayland helper) does not update the user's XAUTHORITY, causing "Invalid MIT-MAGIC-COOKIE-1 key" errors for all X11 apps.
Root cause: sessionChanged() was only emitted on user switch, not when the same session's Xwayland was recreated after a restart. treeland-sd listens for this signal to refresh DISPLAY/XAUTHORITY.
Changes:
Fix: #1071
Summary by Sourcery
Improve Xwayland activation robustness and recovery when treeland restarts.
Bug Fixes:
Enhancements:
Deployment: