Skip to content

fix: route screen lock through logind on Wayland#214

Open
yixinshark wants to merge 1 commit into
linuxdeepin:masterfrom
yixinshark:fix-wayland-lock-deadlock
Open

fix: route screen lock through logind on Wayland#214
yixinshark wants to merge 1 commit into
linuxdeepin:masterfrom
yixinshark:fix-wayland-lock-deadlock

Conversation

@yixinshark

Copy link
Copy Markdown
Contributor

Summary

  • On Wayland the lock screen is provided by treeland (driven by the logind Lock signal), but SessionManager::RequestLock() unconditionally called the X11-only org.deepin.dde.LockFront1 (dde-lock).
  • Under Wayland dde-lock.service is ExecCondition-disabled, so org.deepin.dde.LockFront1 has no provider — the call hangs on the 120 s D-Bus activation timeout and logs failed to lock. A Wayland guard previously existed on this path and was dropped in an earlier refactor.
  • Gate the lock path on Utils::IS_WAYLAND_DISPLAY:
    • RequestLock()m_login1SessionInter->Lock() so treeland shows the lock screen.
    • handleLoginSessionLocked() only syncs local lock state (treeland reacts to the same logind signal; re-entering RequestLock would loop).
    • handleLoginSessionUnlocked() only syncs state, skipping the X11 LockFront1-kill path that early-returns and leaves m_locked stale.

Evidence (before fix)

dde-session: failed to lock, async error: Did not receive a reply ...
dbus-daemon: Failed to activate service 'org.deepin.dde.LockFront1': timed out (service_start_timeout=120000ms)

Test plan

  • On a treeland/Wayland session, trigger a lock (lock shortcut / idle) and confirm the journal no longer shows the LockFront1 120 s activation timeout / failed to lock.
  • On X11, confirm the lock screen still comes up via dde-lock (LockFront1) as before.

On Wayland the lock screen is provided by treeland (driven by the logind Lock
signal), but RequestLock() unconditionally called the X11-only
org.deepin.dde.LockFront1 (dde-lock). Under Wayland dde-lock.service is
ExecCondition-disabled, so that D-Bus name has no provider and the call hangs
on the 120s activation timeout, logging "failed to lock". A Wayland guard used
to exist on this path and was dropped during an earlier refactor.

Gate the lock path on Utils::IS_WAYLAND_DISPLAY:
- RequestLock() asks logind to Lock() so treeland shows the lock screen.
- handleLoginSessionLocked() only syncs local lock state (treeland reacts to
  the same logind signal; re-entering RequestLock would loop).
- handleLoginSessionUnlocked() only syncs state, skipping the X11
  LockFront1-kill dance that early-returns and leaves m_locked stale.

在 Wayland 下锁屏由 treeland 提供(响应 logind 的 Lock 信号),但 RequestLock()
无条件调用了仅 X11 有效的 org.deepin.dde.LockFront1(dde-lock)。Wayland 下
dde-lock.service 被 ExecCondition 禁用,该 D-Bus 名无人提供,调用会卡在 120s 的
激活超时并打印 "failed to lock"。此路径原有的 Wayland 判断在之前的重构中被丢失。

按 Utils::IS_WAYLAND_DISPLAY 分流:
- RequestLock() 改为请求 logind Lock(),由 treeland 显示锁屏;
- handleLoginSessionLocked() 仅同步本地锁状态(treeland 已响应同一 logind 信号,
  再次进入 RequestLock 会形成回环);
- handleLoginSessionUnlocked() 仅同步状态,跳过会提前 return 并使 m_locked 不一致
  的 X11 LockFront1 kill 流程。

Log: route screen lock through logind on Wayland
Change-Id: Iab1b9648ec8f53eca3023891ac33cc76d7f8f59b

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @yixinshark, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yixinshark

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

这段代码主要是对SessionManager类中锁屏相关功能的改进,主要针对Wayland显示协议的支持。我来分析一下代码的改进点和潜在问题:

改进点分析:

  1. Wayland支持

    • 新增了对Wayland显示环境的特殊处理逻辑
    • 避免了在Wayland环境下调用不支持的X11锁屏服务
    • 通过logind系统实现锁屏功能,更符合Wayland的架构设计
  2. 代码结构优化

    • 通过Utils::IS_WAYLAND_DISPLAY条件判断,清晰地分离了不同显示环境下的处理逻辑
    • 每个分支都有明确的注释说明,提高了代码可读性
  3. 错误处理改进

    • 避免了在Wayland环境下调用不存在的服务导致的120秒D-Bus激活超时问题

潜在问题和建议:

  1. 状态同步问题

    m_locked = true;
    emitLockChanged(true);

    建议将状态同步操作封装到一个单独的函数中,避免重复代码。

  2. 日志记录
    虽然添加了必要的日志输出,但建议统一日志格式,例如:

    qDebug() << "[SessionManager] RequestLock on wayland: ask logind to lock";
  3. 代码复用
    handleLoginSessionLocked和handleLoginSessionUnlocked中的Wayland处理逻辑相似,可以考虑提取共同的状态同步逻辑。

  4. 性能考虑

    • QDBusInterface的创建是同步的,虽然使用了异步调用,但对象创建本身可能带来轻微性能开销
    • 考虑将常用的DBus接口对象保存为成员变量,避免重复创建
  5. 安全性建议

    • 在处理锁屏状态变化时,建议添加状态验证,防止状态不一致
    • 考虑添加异常处理机制,防止DBus调用失败导致的状态不同步
  6. 代码风格

    • 建议将条件判断后的代码块适当缩进,提高可读性
    • 可以考虑使用早期返回(early return)模式减少嵌套层级

具体改进建议示例:

void SessionManager::syncLockState(bool locked)
{
    if (m_locked == locked)
        return;
        
    m_locked = locked;
    emitLockChanged(locked);
    if (!locked)
        Q_EMIT Unlock();
}

void SessionManager::RequestLock()
{
    if (Utils::IS_WAYLAND_DISPLAY) {
        qDebug() << "[SessionManager] RequestLock on wayland: ask logind to lock";
        m_login1SessionInter->Lock();
        m_inCallRequestLock = false;
        return;
    }

    // X11处理逻辑
    ...
}

void SessionManager::handleLoginSessionLocked()
{
    if (Utils::IS_WAYLAND_DISPLAY) {
        qDebug() << "[SessionManager] handleLoginSessionLocked on wayland: sync local state";
        syncLockState(true);
        return;
    }

    // X11处理逻辑
    ...
}

void SessionManager::handleLoginSessionUnlocked()
{
    qDebug() << "[SessionManager] login session unlocked";

    if (Utils::IS_WAYLAND_DISPLAY) {
        qDebug() << "[SessionManager] handleLoginSessionUnlocked on wayland: sync local state";
        syncLockState(false);
        return;
    }

    // X11处理逻辑
    ...
}

这些改进建议旨在提高代码的可维护性、可读性和健壮性,同时保持原有功能的完整性。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants