Skip to content

fix(device): load xrandr info asynchronously to avoid UI freeze - #733

Open
GongHeng2017 wants to merge 2 commits into
linuxdeepin:develop/eaglefrom
GongHeng2017:agent/pms-bug-bot/70b81645
Open

fix(device): load xrandr info asynchronously to avoid UI freeze#733
GongHeng2017 wants to merge 2 commits into
linuxdeepin:develop/eaglefrom
GongHeng2017:agent/pms-bug-bot/70b81645

Conversation

@GongHeng2017

@GongHeng2017 GongHeng2017 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

根因分析

设备管理器在 UI 线程(MainWindow::slotListItemClicked)上以 QThread::start() + QThread::wait() 同步等待执行 xrandr/xrandr --verbose 的工作线程,而子进程用 QProcess::waitForFinished(-1) 无超时等待。该同步等待冻结 Qt 主事件循环,导致点击模块切换时界面长时间无反应;AMD Oland/amdgpu 等机型 xrandr --verbose 耗时数秒时尤为明显。

关键证据(基于锁定基线 develop/eagle @ ccd4ab90):

  • MainWindow.cpp:549 slotListItemClicked 在 GUI 主线程 tx.start(); tx.wait();(Monitor/Overview :554-555、Display Adapter :558-559)。
  • ThreadExecXrandr.cpp:66 process.waitForFinished(-1) 永久等待,无超时。
  • MainWindow.cpp:575-577txgpu 块位于 if-else 链外(7f1adf6e 引入的回归),每次点击任意模块都无条件再跑一次同步 xrandr——"点其他模块也卡"的直接成因。
  • ThreadExecXrandr.cpp:43-44 vs :49-59m_isDXcbPlatform(Wayland 守卫)只写不读,平台分支失效。

修复方案

  1. xrandr 获取移出 UI 线程改异步slotListItemClicked 中显示设备/显示适配器不再 start()+wait(),改为异步启动 ThreadExecXrandrfinished 信号回到主线程后再 getDeviceList+updateDevice
  2. 移除无条件 txgpu 回归块:将其显示器热插拔检测(monitorNumber)合并到异步完成槽 slotXrandrFinished,仅在显示模块点击时触发;monitorNumber 改由 DBus(getResolutionFromDBus)派生,不再为热插拔检测额外跑 xrandr 子进程。
  3. runCmd 加超时waitForFinished(-1)waitForFinished(5000),超时 kill() 子进程,避免 xrandr 挂起拖死界面。
  4. 激活 Wayland 守卫run()!m_isDXcbPlatform(Wayland/无 X)时走 DBus 获取显示信息,不再执行 xrandr 子进程。
  5. 析构函数释放异步线程,避免退出悬空;快速切换/非显示模块打断的竞态已处理(m_xrandrItem 取消待刷新项)。

改动安全评估

风险等级:中风险(修改函数内部逻辑 + 引入异步生命周期,无签名变更、无公开 API 变更、受影响调用者 ≤ 2)。数据流不变(ThreadExecXrandr::run 仍写入 DeviceManager,完成后主线程刷新);并发模型与既有 mp_WorkingThread 异步写 DeviceManager 一致。详见附件 change-safety.md

未在本轮处理的同类 anti-pattern(建议后续单独修复)

  • MainWindow.cpp:123-125 构造期 txgpu.start()+wait()(启动期一次性阻塞,非本次点击卡死问题)。
  • MainWindow.cpp:562-563 CPU 分支 LoadCpuInfoThread 同步 start()+wait()LoadCpuInfoThread.cpp:27 同为 waitForFinished(-1);lscpu 通常很快,非本次卡死主因)。
  • 其他 waitForFinished(-1) 无超时调用点(GenerateDevice/CmdTool.cppHWGenerator.cpp 等,多在后台生成线程内)。

关联

Summary by Sourcery

Load xrandr-based display information asynchronously to prevent UI freezes when switching display-related modules.

Bug Fixes:

  • Resolve UI blocking caused by synchronously running xrandr in the main thread when opening Monitor, Overview, or Display Adapter pages.
  • Fix incorrect unconditional re-running of xrandr for GPU-related monitor hotplug checks on every module switch.

Enhancements:

  • Introduce an asynchronous ThreadExecXrandr flow with completion handling to refresh the appropriate display module once data is ready.
  • Add monitor hotplug detection and background refresh triggering to the asynchronous xrandr completion handler while avoiding unwanted UI page changes.
  • Ensure proper cleanup of the asynchronous xrandr thread on MainWindow destruction to avoid dangling threads and races.
  • Refactor display-page device refresh logic into a dedicated helper for reuse across synchronous and asynchronous flows.

Move the ThreadExecXrandr data fetch in slotListItemClicked off the UI
thread: start it asynchronously and refresh the device list on the main
thread when finished. Drop the txgpu block that sat outside the if-else
chain and unconditionally ran synchronous xrandr on every click, folding
its monitor hotplug detection into the async completion slot. Add a
5-second timeout to ThreadExecXrandr::runCmd so a hung xrandr subprocess
can no longer freeze the UI, and activate the m_isDXcbPlatform guard so
Wayland/no-X sessions fetch display info via DBus instead of spawning
xrandr. Release the async thread in the destructor to avoid dangling.

将 slotListItemClicked 中的 ThreadExecXrandr 显示信息获取移出 UI 线程:改为异步启动,完成后回到主线程刷新设备列表;移除位于 if-else 链外、每次点击都无条件同步执行 xrandr 的 txgpu 块,其显示器热插拔检测合并到异步完成槽中;ThreadExecXrandr::runCmd 增加子进程超时与 kill,避免 xrandr 挂起拖死界面;激活 m_isDXcbPlatform 守卫,Wayland/无 X 环境下走 DBus 获取显示信息,不再执行 xrandr 子进程;析构函数释放异步线程避免悬空。

Log: 修复设备管理器点击显示设备后切换其他模块界面长时间无响应的问题,xrandr 信息获取移出 UI 线程改异步并增加子进程超时与 Wayland 守卫
PMS: BUG-248321
Influence: 点击显示设备/显示适配器不再阻塞界面,切换其他模块即时响应;Wayland 环境下不再因 xrandr 子进程挂起导致卡死

@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 @GongHeng2017, 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: GongHeng2017

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

@sourcery-ai

sourcery-ai Bot commented Aug 17, 2026

Copy link
Copy Markdown

Reviewer's Guide

Make xrandr-based display info loading fully asynchronous to avoid blocking the UI thread, add timeouts and platform guards around xrandr subprocesses, and refactor monitor hot-plug detection and device refresh logic accordingly.

Sequence diagram for asynchronous xrandr-based display info loading

sequenceDiagram
    actor User
    participant MainWindow
    participant ThreadExecXrandr
    participant DBusInterface
    participant DeviceManager

    User->>MainWindow: slotListItemClicked(itemStr)
    MainWindow->>MainWindow: [itemStr is Monitor/Overview/DisplayAdapter]
    MainWindow->>MainWindow: startAsyncXrandr(itemStr)
    MainWindow->>ThreadExecXrandr: new ThreadExecXrandr(gpu, isDXcbPlatform)
    MainWindow->>ThreadExecXrandr: start()

    ThreadExecXrandr->>ThreadExecXrandr: run()
    alt gpu && m_isDXcbPlatform
        ThreadExecXrandr->>ThreadExecXrandr: getGpuInfoFromXrandr()
        ThreadExecXrandr->>QProcess: runCmd(info, cmd)
        QProcess-->>ThreadExecXrandr: waitForFinished(5000)
    else !gpu && (!m_isDXcbPlatform || boardVendorType==PGUV)
        ThreadExecXrandr->>DBusInterface: getResolutionRateFromDBus(lstMap)
    else
        ThreadExecXrandr->>ThreadExecXrandr: getMonitorInfoFromXrandrVerbose()
        ThreadExecXrandr->>QProcess: runCmd(info, cmd)
        QProcess-->>ThreadExecXrandr: waitForFinished(5000)
    end
    ThreadExecXrandr->>ThreadExecXrandr: getResolutionFromDBus(tmp)
    ThreadExecXrandr-->>MainWindow: finished()

    MainWindow->>MainWindow: slotXrandrFinished()
    MainWindow->>ThreadExecXrandr: getMonitorNumber()
    alt monitorNumber changed
        MainWindow->>DBusInterface: getInfo(is_server_running, info)
        MainWindow->>MainWindow: refreshDataBaseLater()
    end
    MainWindow->>MainWindow: [itemStr = m_xrandrItem]
    alt itemStr is empty
        MainWindow-->>User: no UI change
    else itemStr != m_xrandrStartedItem
        MainWindow->>MainWindow: startAsyncXrandr(itemStr)
    else
        MainWindow->>DeviceManager: getDeviceList(itemStr, lst)
        DeviceManager-->>MainWindow: lst
        MainWindow->>MainWindow: updateDeviceForItem(itemStr)
        MainWindow-->>User: display updated
    end
Loading

File-Level Changes

Change Details Files
Refactor display-related item click handling to use an asynchronous ThreadExecXrandr workflow and separate device-refresh logic from xrandr execution.
  • Replace synchronous ThreadExecXrandr start()+wait() calls for Monitor/Overview/Display Adapter with an asynchronous startAsyncXrandr flow based on a dedicated mp_XrandrThread member
  • Introduce m_xrandrItem and m_xrandrStartedItem to track the requested and actually-loaded display modules, and cancel or re-chain work when the user switches modules mid-execution
  • Extract device list retrieval and UI-refresh logic into updateDeviceForItem and reuse it from both slotListItemClicked and the xrandr completion path
  • Ensure that during data refresh (m_refreshing or mp_WorkingThread->isRunning) display UI updates are skipped to avoid concurrent refresh issues
deepin-devicemanager/src/Page/MainWindow.cpp
deepin-devicemanager/src/Page/MainWindow.h
Add a completion slot to handle async xrandr results, including monitor hot-plug detection and conditional backend refresh.
  • Add slotXrandrFinished to process mp_XrandrThread results once the thread finishes
  • Move monitor hot-plug detection logic from the previous txgpu block into slotXrandrFinished, using mp_XrandrThread->getMonitorNumber to compare and update monitorNumber
  • Trigger refreshDataBaseLater via DBus is_server_running checks when the monitor count changes, and log the refresh with qCDebug
  • Handle race conditions where the user switches away from display modules during async execution by skipping UI changes or re-starting xrandr for the latest requested item
deepin-devicemanager/src/Page/MainWindow.cpp
deepin-devicemanager/src/Page/MainWindow.h
Introduce proper lifecycle management for the asynchronous xrandr thread, including cleanup on MainWindow destruction.
  • Add a mp_XrandrThread pointer member to MainWindow initialized to nullptr
  • In startAsyncXrandr, delete any existing mp_XrandrThread via deleteLater before creating a new ThreadExecXrandr instance and connecting its finished signal
  • In the MainWindow destructor, wait up to 6000 ms for an active mp_XrandrThread to finish before deleting it and nulling the pointer
deepin-devicemanager/src/Page/MainWindow.cpp
deepin-devicemanager/src/Page/MainWindow.h
Strengthen ThreadExecXrandr platform handling and fallback logic to avoid running xrandr in Wayland or non-X environments and ensure monitor info is available via DBus when needed.
  • Update ThreadExecXrandr::run to only call getGpuInfoFromXrandr when running on an X11/dxcb platform (m_isDXcbPlatform)
  • Route non-GPU or PGUV-board cases on Wayland/non-X platforms through DBus resolution retrieval instead of xrandr, preserving getMonitorInfoFromXrandrVerbose on supported platforms
  • Add a fallback call to getResolutionFromDBus when m_monitorLst is empty to derive monitor count for hot-plug detection even when only DBus is used
deepin-devicemanager/src/Tool/ThreadExecXrandr.cpp
Add a timeout and safe termination behavior for subprocesses spawned by ThreadExecXrandr::runCmd to prevent indefinite UI freezes.
  • Replace QProcess::waitForFinished(-1) with waitForFinished(5000) in runCmd to bound the xrandr execution time
  • On timeout, kill the process and call waitForFinished(1000) to ensure the child process terminates cleanly before reading output
deepin-devicemanager/src/Tool/ThreadExecXrandr.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Address review M01 on PR linuxdeepin#733: the destructor's wait(6000) could time out
before the worker thread's worst-case runtime (~12s for two runCmd calls
plus DBus backfill), after which deleting mp_XrandrThread would free
m_monitorLst while run() still reads/writes it — a use-after-free when
closing the window during async xrandr load. Disconnect the finished
signal first to prevent the slot firing mid-destruction, then wait()
without a timeout (safe because runCmd now caps each subprocess at
5s+1s) so run() is guaranteed to exit before the object is deleted.
Also annotate the unreachable defensive branch in startAsyncXrandr
(review m03) and correct the destructor comment (review m04).

修复 review M01:析构函数 wait(6000) 可能早于工作线程最坏运行时长(双 runCmd 各 5s+1s 约 12s + DBus 回填)超时,此后 delete mp_XrandrThread 会释放 m_monitorLst 而 run() 仍在读写,导致显示加载期间关窗的 use-after-free;改为先 disconnect(this) 避免 finished 槽在析构中途触发,再 wait() 无超时等待(runCmd 已限每子进程 5s+1s,最长约 12s 内结束)保证 run() 退出后再释放。同时为 startAsyncXrandr 不可达防御分支加注释(m03)、订正析构注释(m04)。

Log: 修复 review 阻塞项 M01,析构释放异步线程改为等待 run() 退出后再 delete,避免关窗 use-after-free
PMS: BUG-248321
Influence: 显示模块异步加载期间关闭窗口不再释放正在运行的工作线程,消除 use-after-free 崩溃风险
@GongHeng2017

Copy link
Copy Markdown
Contributor Author

返工补充提交(review M01 修复)

新增 commit 0449c2e0 修复 review 阻塞项 M01,其余 review 建议项 m03/m04 一并处理:

  • [M01] MainWindow.cpp:143-152 ~MainWindow — 析构 mp_XrandrThread->wait(6000) 改为先 disconnect(this)(避免 finished 槽在析构中途触发)再 wait() 无超时等待。因 runCmd 已限每子进程 5s+1s,双 runCmd 最长约 12s 内结束,wait() 保证 run() 退出后再 delete,消除显示加载期间关窗的 use-after-free。
  • [m04] 同步订正析构注释,说明 ~12s 最坏时长与 use-after-free 防护。
  • [m03] startAsyncXrandr 首部不可达防御分支加注释,说明为兜底。

行为变更记录(review m02)

显示器热插拔检测(monitorNumber 变化 → 请求后台 refreshDataBaseLater)的触发时机由每次点击任意模块(原 :575-577 无条件 txgpu 块)收窄为仅点击显示设备/概况/显示适配器时slotXrandrFinished 内)。这是删除无条件回归块的合理副作用——非显示模块(CPU/网络/电池)的点击不涉及显示器热插拔场景。monitorNumber 改由 DBus(getResolutionFromDBus 填充 m_monitorLst)派生,不再为热插拔检测额外 spawn xrandr 子进程。建议回归覆盖此项。

未处理(建议项,不阻塞)

  • [m01] 构造期 txgpuMainWindow.cpp:122-126)、CPU 分支 LoadCpuInfoThread:567-569LoadCpuInfoThread.cpp:27 waitForFinished(-1))仍为 UI 线程同步等待,建议后续单独 issue 处理(本轮 PR 描述已注明)。

测试建议(M01 相关)

在联想 M630z / 兆芯 + AMD Oland 机型验证:

  1. 点击显示设备后切其他模块即时响应;
  2. 显示适配器/显示设备信息正常刷新;
  3. Wayland 会话不卡死;
  4. 显示模块异步加载期间关窗不崩溃(M01 修复点)。

PMS: https://pms.uniontech.com/bug-view-248321.html

@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

★ 总体评分:100分

■ 【总体评价】

代码将同步阻塞调用改为异步信号槽机制,并增加了超时保护与析构安全清理
逻辑严密且无安全漏洞,达到优秀标准

■ 【详细分析】

  • 1.语法逻辑(基本正确)✓

修复方案通过引入异步回调与状态变量(m_xrandrItem、m_xrandrStartedItem)精确控制了快速切换模块时的时序问题。在slotXrandrFinished中先保存局部变量item再清理线程指针,并在startAsyncXrandr中加入防御性判空,有效避免了空指针解引用与野指针风险。析构函数中先disconnect再wait和delete的顺序正确,防止了析构中途触发槽函数及use-after-free。
潜在问题:slotListItemClicked中LoadCpuInfoThread仍采用同步wait方式,虽非本次修复范围,但未来可考虑一并异步化
建议:后续迭代中将其他类似同步等待子进程的逻辑统一改造为异步模式

  • 2.代码质量(良好)✓

代码结构优化显著,将重复的界面刷新逻辑提取为updateDeviceForItem函数,符合单一职责原则。新增的startAsyncXrandr与slotXrandrFinished函数分工明确,注释详尽且准确解释了防御性编程的意图、超时时间的计算依据以及避免回切界面的设计考量,可读性与可维护性高。
潜在问题:无
建议:无

  • 3.代码性能(高效)✓

彻底消除了UI线程同步等待xrandr子进程导致的界面卡死问题。为QProcess增加5秒超时与kill保护,避免了特定硬件或无X环境下子进程无限挂起。同时在ThreadExecXrandr::run中增加平台判断,在Wayland或无X环境下直接走DBus,减少了不必要的系统调用开销。
潜在问题:无
建议:无

  • 4.代码安全(存在0个安全漏洞)✓

漏洞对比统计:新增漏洞 0 个,减少漏洞 0 个,持平 0 个
代码未引入新的安全风险。runCmd中的cmd参数由内部硬编码拼接,不受外部输入控制,不存在命令注入风险。DBus调用获取状态后使用toInt()转换,转换失败时走默认逻辑,不构成安全漏洞。异步机制与超时保护从设计上消除了原有的拒绝服务(界面卡死)风险。

  • 建议:保持当前的硬编码调用方式,避免未来重构时将不受信的外部输入直接拼接到cmd字符串中

■ 【改进建议代码示例】

// 当前代码已具备较高的安全性与健壮性,以下示例仅展示对潜在逻辑瑕疵(如toInt转换失败)的防御性增强
void MainWindow::slotXrandrFinished()
{
    if (!mp_XrandrThread) {
        return;
    }

    if (monitorNumber != mp_XrandrThread->getMonitorNumber()) {
        QString info;
        bool ok = false;
        DBusInterface::getInstance()->getInfo("is_server_running", info);
        int isRunning = info.toInt(&ok);
        //请求后台更新信息:仅在明确返回非1(或转换失败)时请求,增强逻辑严密性
        if (!ok || isRunning != 1) {
            refreshDataBaseLater();
        }
        qCDebug(appLog) << "Monitor refreshInfo" << __LINE__ << QDateTime::currentDateTime().toString("hh:mm:ss") << info << monitorNumber;
        monitorNumber = mp_XrandrThread->getMonitorNumber();
    }

    QString item = m_xrandrItem;
    mp_XrandrThread->deleteLater();
    mp_XrandrThread = nullptr;

    if (item.isEmpty()) {
        return;
    }
    if (item != m_xrandrStartedItem) {
        startAsyncXrandr(item);
        return;
    }

    if (m_refreshing || mp_WorkingThread->isRunning()) return;

    updateDeviceForItem(item);
}

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