Skip to content

Conversation

@BLumia
Copy link
Member

@BLumia BLumia commented Jan 23, 2026

避免因 handler deleteLater 被实际删除之前触发信号.

Summary by Sourcery

Bug Fixes:

  • Prevent signals from being emitted to AppMgr after a handler has been scheduled for deletion when an app item is removed.

避免因 handler deleteLater 被实际删除之前触发信号.

Log:
@BLumia BLumia requested a review from 18202781743 January 23, 2026 05:52
@sourcery-ai
Copy link

sourcery-ai bot commented Jan 23, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Ensure signal connections from the app item handler to AppMgr are disconnected immediately when an app item is removed, before scheduling the handler object for deferred deletion, to avoid signals being emitted during the deletion window.

Sequence diagram for updated app item removal and handler disconnection

sequenceDiagram
    participant AppMgr
    participant AppItem
    participant Handler
    participant QtEventLoop

    AppMgr->>AppMgr: watchingAppItemRemoved(key)
    AppMgr->>AppItem: get handler
    alt handler exists
        AppMgr->>Handler: disconnect(AppMgr)
        AppMgr->>Handler: deleteLater()
        AppMgr->>AppMgr: m_appItems.remove(key)
        QtEventLoop-->>Handler: process deferred deletion
        Handler-->>Handler: destroyed
    else no handler
        AppMgr->>AppMgr: m_appItems.remove(key)
    end
Loading

File-Level Changes

Change Details Files
Ensure handler object disconnects from AppMgr before being scheduled for deferred deletion to avoid late signal emissions.
  • Add an explicit disconnect call on the handler object, passing AppMgr (this) as the sender/receiver context, right before calling deleteLater
  • Preserve existing behavior of logging and deferred deletion while tightening the object lifetime and signal connection semantics on app removal
src/ddeintegration/appmgr.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

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • If the intent is to prevent any further signal emissions from the handler (not just those targeting this object), consider using a broader disconnect (e.g., handler->disconnect() or the appropriate overload) instead of handler->disconnect(this).
  • After scheduling handler->deleteLater(), consider explicitly nulling appItem->handler (and any other references) to reduce the risk of accidental use-after-free on the handler pointer before the event loop processes the deletion.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- If the intent is to prevent any further signal emissions from the handler (not just those targeting this object), consider using a broader disconnect (e.g., `handler->disconnect()` or the appropriate overload) instead of `handler->disconnect(this)`.
- After scheduling `handler->deleteLater()`, consider explicitly nulling `appItem->handler` (and any other references) to reduce the risk of accidental use-after-free on the handler pointer before the event loop processes the deletion.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link

deepin pr auto review

这段代码修改主要是在删除应用程序处理器(handler)之前,显式地调用 disconnect() 断开其与 AppMgr 对象之间的信号槽连接。这是一个很好的改进,下面我将从语法逻辑、代码质量、代码性能和代码安全四个方面进行详细审查:

1. 语法逻辑

  • 审查意见:语法正确。handler->disconnect(this) 是 Qt 中标准的断开信号槽连接的写法,用于断开 handler 对象发出的所有信号连接到 this(即 AppMgr 实例)的槽。
  • 逻辑分析
    • deleteLater() 之前断开连接是合理的。
    • deleteLater() 会将对象放入事件循环中等待删除。如果在对象被销毁前,AppMgr 再次接收到来自该 handler 的信号(例如网络回调或异步操作完成),可能会导致 AppMgr 调用已标记为删除的对象的方法,从而引发潜在的崩溃或未定义行为。
    • 显式 disconnect 确保了在对象销毁前,不再有新的事件触发 AppMgr 中的槽函数。

2. 代码质量

  • 审查意见:提高了代码的健壮性和可维护性。
  • 解释
    • 防御性编程:显式断开连接是一种防御性编程实践。它不依赖于 Qt 对象销毁时自动断开连接的机制(虽然 QObject 析构时会自动 disconnect),而是主动切断联系,防止在对象生命周期的末期(即 deleteLater 到实际析构之间的这段时间)发生意外的交互。
    • 意图明确:代码清晰地表达了"在移除应用时,不仅要标记删除 handler,还要立即停止与其通信"的意图。

3. 代码性能

  • 审查意见:对性能影响微乎其微,甚至可能有正面影响。
  • 解释
    • disconnect 操作通常非常快,它主要涉及内部连接列表的查找和移除。
    • 相比于潜在的错误(如信号触发导致的崩溃或异常处理逻辑)带来的性能损耗,这里的显式调用开销可以忽略不计。
    • 断开连接后,后续不会再有信号传递和槽函数调用的开销,略微减少了系统负载。

4. 代码安全

  • 审查意见:显著提高了安全性,防止了潜在的野指针访问和逻辑错误。
  • 解释
    • 防止悬空信号:如果不调用 disconnect,且 handler 在事件循环中排队等待删除期间发出了信号,AppMgr 的槽函数可能会被调用。此时槽函数内部如果访问了 handler,虽然 Qt 的信号槽机制通常能保证发送者对象的有效性(通过 QPointersender() 检查),但在复杂的异步逻辑中,显式断开更安全。
    • 避免竞态条件:在多线程环境下(虽然 deleteLater 通常在对象所在线程执行),显式断开连接能更清晰地界定对象的生命周期边界,减少竞态条件发生的可能性。

改进建议

虽然当前的修改已经很好,但还可以考虑以下微调:

  1. 使用 QPointer(如果适用):
    如果 handler 是一个原始指针,且在其他地方(非移除逻辑中)也有可能被访问,建议考虑将其提升为 QPointer<HandlerType>QPointer 会在对象被销毁后自动置空,可以防止在槽函数中访问已销毁的对象。

    // 示例概念
    QPointer<HandlerType> handler = appItem->handler;
    if (handler) {
        handler->disconnect(this);
        handler->deleteLater();
    }
  2. 日志记录
    当前的日志记录了 "Deleting handler...",如果 disconnect 失败(虽然罕见,除非信号槽从未连接过),可能不会有提示。不过对于此处的逻辑,这不是必须的。

  3. 空指针检查
    代码中已经使用了 if (auto handler = appItem->handler) 进行了检查,这是很好的做法。

总结

这个修改是非常必要且正确的。它通过显式断开信号槽连接,消除了对象销毁前可能发生的异步交互风险,增强了程序的稳定性和安全性。建议采纳该修改。

@BLumia BLumia requested a review from 18202781743 January 23, 2026 07:06
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, BLumia

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

@BLumia BLumia merged commit 4c5ac37 into linuxdeepin:master Jan 23, 2026
10 checks passed
@BLumia BLumia deleted the Iceyer-patch-selected branch January 23, 2026 11:39
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