fix: Fix Xwayland not started after crash recover & switch user #49
Conversation
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
|
Hi @calsys456. Thanks for your PR. I'm waiting for a linuxdeepin member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsures identify-only authentication flows properly acknowledge successful login and activate the correct XDG session, fixing Xwayland failures after crash recovery and user switching, and simplifies session activation to rely directly on the provided xdgSessionId instead of reusing Auth objects. Sequence diagram for the identify-only login and session activation flowsequenceDiagram
actor User as "User"
participant Treeland as "Treeland Compositor"
participant Display as "DDM Display"
participant Server as "SingleWaylandDisplayServer"
participant Logind as "Logind Manager (logind1)"
"User" ->> "Treeland Compositor": "Initiate login (identify-only)"
"Treeland Compositor" ->> "DDM Display": "Authenticate request (identify-only)"
"DDM Display" -->> "Treeland Compositor": "Authentication succeeded (identify-only)"
note over Display: "Display handles auth success for identify-only"
"DDM Display" ->> "SingleWaylandDisplayServer": "onLoginSucceeded(user)"
"DDM Display" ->> "DDM Display": "Lookup xdgSessionId for user in m_auths"
"DDM Display" ->> "DDM Display": "switchToUser(user, xdgSessionId)"
note over Display: "switchToUser now requires valid xdgSessionId and no longer relies on Auth instance"
"DDM Display" ->> "SingleWaylandDisplayServer": "activateUser(user, xdgSessionId)"
alt "Logind available"
"DDM Display" ->> "Logind Manager (logind1)": "ActivateSession(QString::number(xdgSessionId))"
"Logind Manager (logind1)" -->> "DDM Display": "Session activated"
else "Logind not available"
"DDM Display" -->> "DDM Display": "Skip system session activation"
end
"SingleWaylandDisplayServer" -->> "Treeland Compositor": "Wayland/Xwayland session ready for user"
Updated class diagram for Display, Auth, and SingleWaylandDisplayServer session handlingclassDiagram
class Display {
- m_auths : QList< Auth* >
- m_displayServer : DisplayServer*
+ switchToUser(user : QString, xdgSessionId : int) : void
+ onAuthSucceeded(auth : Auth*) : void
}
class Auth {
+ user() : QString
+ xdgSessionId() : int
+ identifyOnly() : bool
+ isSingleMode() : bool
+ sessionId() : QString
}
class DisplayServer {
<<interface>>
+ stopped()
}
class SingleWaylandDisplayServer {
+ onLoginSucceeded(user : QString) : void
+ activateUser(user : QString, xdgSessionId : int) : void
}
class Logind {
<<static_helper>>
+ isAvailable() : bool
+ serviceName() : QString
+ managerPath() : QString
}
class OrgFreedesktopLogin1ManagerInterface {
+ OrgFreedesktopLogin1ManagerInterface(serviceName : QString, managerPath : QString, bus : QDBusConnection)
+ ActivateSession(sessionId : QString) : void
}
DisplayServer <|.. SingleWaylandDisplayServer : "inherits"
Display --> Auth : "maintains m_auths"
Display --> DisplayServer : "uses m_displayServer"
Display ..> SingleWaylandDisplayServer : "casts to control single-user session"
Display ..> Logind : "checks availability and metadata"
Display ..> OrgFreedesktopLogin1ManagerInterface : "activates session via D-Bus"
%% Highlight identify-only flow responsibility
Display ..> Auth : "reads identifyOnly() and xdgSessionId()"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In switchToUser, relying on Q_ASSERT(xdgSessionId > 0) may cause crashes in release builds with invalid input; consider gracefully handling a non-positive xdgSessionId (e.g., early return or logging) instead of asserting.
- In the identifyOnly branch, the inner for-loop reuses the name 'auth' and shadows the outer pointer, which makes the logic harder to follow; use a different loop variable name to improve readability.
- The TODO about multiple sessions for a user suggests potential ambiguity in which xdgSessionId is chosen; if feasible, clarify selection strategy or pass the exact session ID from the caller to avoid relying on the first matching auth.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In switchToUser, relying on Q_ASSERT(xdgSessionId > 0) may cause crashes in release builds with invalid input; consider gracefully handling a non-positive xdgSessionId (e.g., early return or logging) instead of asserting.
- In the identifyOnly branch, the inner for-loop reuses the name 'auth' and shadows the outer pointer, which makes the logic harder to follow; use a different loop variable name to improve readability.
- The TODO about multiple sessions for a user suggests potential ambiguity in which xdgSessionId is chosen; if feasible, clarify selection strategy or pass the exact session ID from the caller to avoid relying on the first matching auth.
## Individual Comments
### Comment 1
<location> `src/daemon/Display.cpp:228-235` </location>
<code_context>
- if (!auth) {
- return;
- }
+ Q_ASSERT(xdgSessionId > 0);
auto* server = reinterpret_cast<SingleWaylandDisplayServer*>(m_displayServer);
server->activateUser(user, xdgSessionId);
if (Logind::isAvailable()) {
OrgFreedesktopLogin1ManagerInterface manager(Logind::serviceName(), Logind::managerPath(), QDBusConnection::systemBus());
- manager.ActivateSession(QString::number(auth->xdgSessionId()));
+ manager.ActivateSession(QString::number(xdgSessionId));
}
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Relying solely on Q_ASSERT for xdgSessionId validity can cause issues in release builds.
Previously, missing Auth prevented calling ActivateSession with an invalid session id. Now that this check is gone and we only assert on xdgSessionId, release builds may call activateUser/ActivateSession with xdgSessionId <= 0 if something goes wrong upstream (e.g., DBus race, misconfiguration). Please add a runtime check (e.g., early return when xdgSessionId <= 0) in addition to the Q_ASSERT so release builds fail safely instead of using a bad session id.
</issue_to_address>
### Comment 2
<location> `src/daemon/Display.cpp:629-638` </location>
<code_context>
stateConfig.save();
- if (auth->isSingleMode()) {
+ if (auth->identifyOnly()) {
+ auto* server = reinterpret_cast<SingleWaylandDisplayServer*>(m_displayServer);
+ server->onLoginSucceeded(user);
+ // TODO: Use exact ID when there're multiple sessions for a user
+ int xdgSessionId = 0;
+ for (auto *auth : m_auths)
+ if (auth->user() == user && auth->xdgSessionId() > 0) {
+ xdgSessionId = auth->xdgSessionId();
+ break;
+ }
+ switchToUser(user, xdgSessionId);
+ } else if (auth->isSingleMode()) {
auto* server = reinterpret_cast<SingleWaylandDisplayServer*>(m_displayServer);
</code_context>
<issue_to_address>
**issue:** identifyOnly path can call switchToUser with xdgSessionId still 0 if no matching session is found.
Here xdgSessionId may stay 0 if no auth in m_auths matches the user and has xdgSessionId() > 0, yet we still call switchToUser(user, xdgSessionId). With the new Q_ASSERT(xdgSessionId > 0) this will assert in debug builds and still pass 0 in release builds. Please either guard the call (e.g. skip or handle as error if xdgSessionId <= 0) or ensure identifyOnly is only used when a valid session id is guaranteed.
</issue_to_address>
### Comment 3
<location> `src/daemon/Display.cpp:632-638` </location>
<code_context>
+ auto* server = reinterpret_cast<SingleWaylandDisplayServer*>(m_displayServer);
+ server->onLoginSucceeded(user);
+ // TODO: Use exact ID when there're multiple sessions for a user
+ int xdgSessionId = 0;
+ for (auto *auth : m_auths)
+ if (auth->user() == user && auth->xdgSessionId() > 0) {
+ xdgSessionId = auth->xdgSessionId();
</code_context>
<issue_to_address>
**suggestion:** The inner loop’s `auth` variable shadows the outer `auth` parameter/pointer and can be confusing.
In the identifyOnly branch, the range-for loop reuses the name `auth` for its loop variable, shadowing the outer `auth` pointer. This works but makes the code harder to read and easier to mis-edit later. Please rename the loop variable (e.g., `candidateAuth` or `sessionAuth`) to clearly distinguish it from the outer pointer.
```suggestion
// TODO: Use exact ID when there're multiple sessions for a user
int xdgSessionId = 0;
for (auto *sessionAuth : m_auths)
if (sessionAuth->user() == user && sessionAuth->xdgSessionId() > 0) {
xdgSessionId = sessionAuth->xdgSessionId();
break;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ddm should ack with switchToUser when treeland send an identify-only request. Lacking this acknowledgement will cause Xwayland not work after crash recover and user switch, which is a bug caused by previous commit.
zccrs
approved these changes
Nov 27, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: calsys456, 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
ddm should ack with switchToUser when treeland send an identify-only request. Lacking this acknowledgement will cause Xwayland not work after crash recover and user switch, which is a bug caused by previous commit.
Summary by Sourcery
Bug Fixes: