Skip to content

fix(reader): fix render thread quit race and adapt page-change unit test - #373

Merged
lzwind merged 1 commit into
linuxdeepin:masterfrom
add-uos:fix-pagereader-quit-race-ut-guard
Sep 9, 2026
Merged

lzwind merged 1 commit into
linuxdeepin:masterfrom
add-uos:fix-pagereader-quit-race-ut-guard

Conversation

@add-uos

@add-uos add-uos commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

问题 / Problem

  1. 进程退出挂死: PageRenderThread::destroyForever() 在线程进入 run() 前设置 m_quit = true,会被 run() 第一行的 m_quit = false 无条件覆盖,导致 wait() 永久阻塞(应用退出时恰好存在文档关闭任务即可触发;单测环境高概率复现)。
  2. 单测失败: f15ae39cDocSheet::onBrowserPageChanged 新增了可见性守卫,离屏单测环境中 m_browser->isVisible() 恒为 false,页码不回写,UT_DocSheet_onBrowserPageChanged_001 断言失败。

修复 / Fix

  • PageRenderThread::run() 不再无条件重置 m_quit(实例创建时默认即为 false),避免吞掉退出请求
  • m_quit 改为 std::atomic<bool>,消除跨线程读写数据竞争
  • 单测补 isVisibleSheetSidebar::setCurrentPage 桩,适配新守卫并避免单测触发渲染线程

自测 / Test

  • TestDocSheet.UT_DocSheet_onBrowserPageChanged_001 单跑通过且进程正常退出(修复前挂死)
  • 全量套件 1120/1120 通过,failures="0"

Summary by Sourcery

Fix the render-thread shutdown race and adapt page-change testing to the browser visibility guard.

Bug Fixes:

  • Prevent render-thread shutdown hangs by preserving early quit requests and making the quit state safe for cross-thread access.

Tests:

  • Update the page-change unit test to account for visibility guarding and avoid starting rendering work during the test.

Remove the unconditional m_quit reset in run(): if destroyForever() set
the quit flag before the thread entered run(), the reset swallowed the
request and wait() blocked forever at process exit. Make m_quit atomic
and stub isVisible/sidebar in the unit test to match the new visibility
guard added for reading-progress restore.

run() 不再无条件重置 m_quit,避免吞掉 destroyForever 的退出请求,
导致进程退出阶段 wait() 永久阻塞;m_quit 改为原子量消除数据竞争。
单测补 isVisible/sidebar 桩,适配阅读进度恢复新增的可见性守卫。

Log: 修复渲染线程退出竞争挂死并适配页码变化单测
Influence: 渲染线程退出时序修复,应用退出更稳;单测恢复全部通过。

@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 @add-uos, you've used your own review budget of 250,000 diff characters for the last 7 days.

You can request another review in 1 day and 8 hours by commenting @sourcery-ai review. Upgrade to get a review now.

@sourcery-ai

sourcery-ai Bot commented Sep 9, 2026

Copy link
Copy Markdown
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Fixes a shutdown race where PageRenderThread could overwrite a pre-start quit request and block forever in wait(), using an atomic quit flag and preserving its initial state. The affected page-change unit test now stubs visibility and sidebar updates to match the production guard without triggering render-thread activity.

Sequence diagram for the PageRenderThread shutdown race fix

sequenceDiagram
    participant MainThread
    participant PageRenderThread

    MainThread->>PageRenderThread: destroyForever()
    PageRenderThread->>PageRenderThread: m_quit = true
    MainThread->>PageRenderThread: start()
    PageRenderThread->>PageRenderThread: run()
    PageRenderThread->>PageRenderThread: read atomic m_quit
    PageRenderThread-->>MainThread: exits run()
    MainThread->>PageRenderThread: wait()
Loading

Sequence diagram for the guarded page-change unit test

sequenceDiagram
    participant Test
    participant DocSheet
    participant Browser
    participant SheetSidebar

    Test->>Browser: isVisible()
    Browser-->>DocSheet: true
    DocSheet->>SheetSidebar: setCurrentPage()
    SheetSidebar-->>Test: page updated
Loading

File-Level Changes

Change Details Files
Prevent the page-render thread from losing a pre-start shutdown request and make quit signaling thread-safe.
  • Removed the unconditional quit-flag reset at the start of run().
  • Changed the quit flag to std::atomic for synchronized cross-thread access.
reader/browser/PageRenderThread.cpp
reader/browser/PageRenderThread.h
Adapt the page-change unit test to the browser-visibility guard while isolating it from thumbnail rendering.
  • Stubbed QWidget::isVisible() to allow page-number writeback.
  • Stubbed SheetSidebar::setCurrentPage() to avoid starting rendering work during the test.
tests/uiframe/ut_docsheet.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

@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

AI 代码审查报告

PR: linuxdeepin/deepin-reader#373
标题: fix(reader): fix render thread quit race and adapt page-change unit test
作者: add-uos
分支: fix-pagereader-quit-race-ut-guard → master
审查时间: 2026-09-09


总体评价

总分:100 分
评级:优秀
风险等级:None

本次 PR 修复了 PageRenderThread 渲染线程退出时的竞态条件问题,同时适配了受可见性守卫影响的单元测试。代码变更精准、注释详尽、测试适配合理,未引入任何安全漏洞。修复方案正确地解决了 destroyForever() 设置 m_quit = true 后被 run()m_quit = false 覆盖导致 wait() 永久阻塞的问题。


漏洞统计

指标 数量
当前漏洞总数 0
新增漏洞 0
减少漏洞 0
持平 0

漏洞对比统计:新增漏洞 0 个,减少漏洞 0 个,持平 0 个


四维度评分

维度1:语法逻辑 ✓(25/25 分)

评价词:语法正确,逻辑清晰

分析:

  1. std::atomic<bool> m_quit {false}; 语法正确,使用 C++11 花括号初始化,符合现代 C++ 规范
  2. #include <atomic> 头文件已正确添加,位置合理
  3. 移除 run() 中的 m_quit = false; 不影响初始化状态,因为成员变量在类定义中已通过 std::atomic<bool> m_quit {false}; 完成初始化
  4. 测试桩函数 isVisible_stub_true(定义于 ut_docsheet.cpp:1174)和 setCurrentPage_stub(定义于 ut_docsheet.cpp:87)均在同文件中已定义,编译无问题
  5. Stub 类的 set 方法和 ADDR 宏使用正确,与文件中其他测试用例用法一致
  6. 修复逻辑正确:destroyForever()run() 启动前设置 m_quit = true,移除 run() 中的重置后,退出信号不再被吞掉,while (!m_quit) 循环将正确退出

无编译错误,无逻辑缺陷,边界条件处理完善。


维度2:代码质量 ✓(25/25 分)

评价词:代码结构清晰,注释完整

分析:

  1. 注释完整性(5/5)

    • PageRenderThread.cpp:317-319:详细注释解释了为什么不能在 run() 中重置 m_quit,包括 destroyForever() 的竞态场景和 wait() 永久阻塞的后果
    • PageRenderThread.h:287:注释说明了使用 atomic 的原因(主线程写、渲染线程读,避免数据竞争)
    • ut_docsheet.cpp:1286-1289:注释解释了桩函数的必要性,包括可见性守卫和避免渲染线程竞争两个原因
  2. 代码重复(5/5):无重复代码,变更最小化

  3. 结构合理性(5/5):变更精准,仅修改必要部分,不影响现有代码结构

  4. 调试信息清理(5/5):无残留调试代码(qCDebug 在 PR 前已存在,非本次新增)

  5. 版权信息更新(5/5)SPDX-FileCopyrightText 年份从 2023 更新为 2023 - 2026,合理

  6. 需求匹配度:代码实现完全匹配 PR 描述中的修复目标——修复渲染线程退出竞态 + 适配单元测试


维度3:代码性能 ✓(20/20 分)

评价词:性能良好,资源使用合理

分析:

  1. std::atomic<bool> 使用 CPU 原子指令实现,开销极小,比 QMutex 加锁方案更高效
  2. 移除 run() 中不必要的 m_quit = false 赋值是微优化,减少了一次原子写操作
  3. m_quit 在渲染线程的主循环中被频繁读取(while (!m_quit) 和多处 if (m_quit)),使用 std::atomic<bool> 的读操作在 x86 上等同于普通读(仅保证内存序),性能无影响
  4. 无性能瓶颈,无资源泄漏

维度4:代码安全 ✓(30/30 分)

存在0个安全漏洞

分析:

本次变更不仅未引入安全漏洞,还修复了一个数据竞争(C++ 中的未定义行为),是安全改进:

  1. 修复的数据竞争:原代码中 m_quit 为普通 bool,主线程(destroyForever())写、渲染线程(run())读,构成数据竞争。改为 std::atomic<bool> 后消除了未定义行为
  2. 无命令注入风险:不涉及外部命令执行
  3. 无 SQL 注入风险:不涉及数据库操作
  4. 无路径遍历风险:不涉及文件路径操作
  5. 无敏感信息泄露:代码中无硬编码密钥、密码等敏感信息
  6. 无缓冲区溢出风险:不涉及缓冲区操作
  7. 无用户输入未校验:不涉及用户输入处理

漏洞对比统计:新增漏洞 0 个,减少漏洞 0 个,持平 0 个


修改文件详情

1. reader/browser/PageRenderThread.h

变更类型 内容
新增头文件 #include <atomic>
类型变更 bool m_quit = false;std::atomic<bool> m_quit {false};
新增注释 说明 atomic 的用途(跨线程读写避免数据竞争)

评价:正确将跨线程访问的标志位改为原子类型,消除数据竞争。

2. reader/browser/PageRenderThread.cpp

| 变更类型 | 内容 |
|:---|:---|:---|
| 移除代码 | m_quit = false;(run() 函数首行) |
| 新增注释 | 详细说明为什么不能在此处重置 m_quit |

评价:核心修复点。移除 m_quit = false 后,destroyForever() 设置的退出信号不再被覆盖,wait() 不会再永久阻塞。注释解释清晰,便于后续维护理解。

3. tests/uiframe/ut_docsheet.cpp

变更类型 内容
版权更新 20232023 - 2026
新增桩函数 isVisibleisVisible_stub_true(返回 true)
新增桩函数 setCurrentPagesetCurrentPage_stub(空实现)
新增注释 说明桩函数的必要性

评价:测试适配合理。onBrowserPageChanged 新增了可见性守卫(m_browser->isVisible() 检查),单测环境窗口未 show,需要桩函数绕过。同时桩掉 setCurrentPage 避免触发渲染线程导致退出阶段竞争挂死。桩函数均已在本文件中定义。


改进建议

本次 PR 代码质量优秀,无安全漏洞,无改进建议。代码变更精准解决了竞态条件问题,注释详尽,测试适配合理。


审查结论

本次 PR 是一个高质量的 Bug 修复提交:

  1. 正确识别了 PageRenderThread::run()m_quit = false 重置导致的竞态条件
  2. 采用 std::atomic<bool> 消除数据竞争,方案简洁高效
  3. 注释详尽,便于后续维护
  4. 单元测试适配完整,全量套件 1120/1120 通过
  5. 无安全漏洞引入

建议合并。

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: add-uos, lzwind

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

@lzwind
lzwind merged commit 366512c into linuxdeepin:master Sep 9, 2026
9 checks passed
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.

3 participants