Skip to content

Conversation

@18202781743
Copy link
Contributor

@18202781743 18202781743 commented Feb 11, 2026

  1. Modified DPluginLoader destructor to check Qt application state
    before calling destroy()
  2. Added condition to only call destroy() when qApp exists and
    application is not closing down
  3. Added warning message when skipping destruction to avoid crash during
    static cleanup
  4. This prevents crashes when DPluginLoader is destroyed after Qt global
    resources have been cleaned up

Log: Fixed potential crash during application shutdown when using static
plugin loader

Influence:

  1. Test application shutdown process with static plugin loader
  2. Verify no crashes occur during normal application exit
  3. Test edge cases where Qt resources might be cleaned up early
  4. Monitor for warning messages about skipped destruction during
    abnormal shutdown
  5. Verify memory cleanup still occurs properly during normal shutdown

fix: 防止 DPluginLoader 静态析构时崩溃

  1. 修改 DPluginLoader 析构函数,在调用 destroy() 前检查 Qt 应用程序状态
  2. 添加条件判断,仅当 qApp 存在且应用程序未关闭时才调用 destroy()
  3. 添加警告信息,当跳过析构以避免静态清理期间的崩溃时进行提示
  4. 防止在 Qt 全局资源已清理后销毁 DPluginLoader 时发生崩溃

Log: 修复使用静态插件加载器时应用程序关闭可能崩溃的问题

Influence:

  1. 测试使用静态插件加载器时的应用程序关闭过程
  2. 验证正常应用程序退出时不会发生崩溃
  3. 测试 Qt 资源可能提前清理的边缘情况
  4. 监控异常关闭期间关于跳过析构的警告信息
  5. 验证正常关闭期间内存清理仍能正确进行

PMS: BUG-350887

Summary by Sourcery

Bug Fixes:

  • Prevent crashes when DPluginLoader is destroyed after Qt global resources have been cleaned up by only invoking destroy() while the Qt application is still active.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743

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
Copy link

sourcery-ai bot commented Feb 11, 2026

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

The PR hardens DPluginLoader’s destructor against application shutdown edge cases by only invoking its destroy() logic while a Qt application instance is alive and not closing, and logging a warning instead of tearing down children during static destruction after Qt global cleanup to avoid crashes.

Sequence diagram for DPluginLoader destruction behavior during shutdown

sequenceDiagram
    participant OS
    participant StaticDPluginLoader as DPluginLoader_static
    participant QCoreApplication as QCoreApplication

    OS->>QCoreApplication: start application
    OS->>DPluginLoader_static: construct static instance

    note over QCoreApplication,DPluginLoader_static: Normal shutdown path
    QCoreApplication->>QCoreApplication: emit aboutToQuit
    QCoreApplication->>DPluginLoader_static: explicit destroy() (normal teardown)
    DPluginLoader_static->>DPluginLoader_static: destroy
    QCoreApplication->>QCoreApplication: begin closingDown
    QCoreApplication-->>OS: cleanup Qt global resources

    note over OS,DPluginLoader_static: Static destruction after Qt cleanup
    OS->>DPluginLoader_static: call ~DPluginLoader
    alt qApp exists and !closingDown
        DPluginLoader_static->>DPluginLoader_static: destroy
    else qApp is null or closingDown
        DPluginLoader_static->>DPluginLoader_static: skip destroy
        DPluginLoader_static->>OS: qWarning skipping child destruction
    end
Loading

Class diagram for updated DPluginLoader destructor logic

classDiagram
    class DPluginLoader {
        +DPluginLoader()
        +~DPluginLoader()
        +static DPluginLoader *instance()
        -void destroy()
    }

    class QCoreApplication {
        +static bool closingDown()
    }

    class QApplication {
    }

    DPluginLoader ..> QCoreApplication : checks_closingDown
    DPluginLoader ..> QApplication : uses_qApp
Loading

File-Level Changes

Change Details Files
Gate DPluginLoader destruction logic on Qt application lifetime to avoid crashes during static destruction.
  • Wrap the destroy() call in a conditional that checks qApp is non-null and QCoreApplication::closingDown() is false before running plugin teardown.
  • Add a warning branch when destruction is skipped in cases where DPluginLoader is destroyed after Qt global resources have already been cleaned up, documenting that children are intentionally not destroyed.
  • Document in comments that when DPluginLoader is a Q_APPLICATION_STATIC object its destructor may run after Qt global cleanup, so skipping destroy() avoids crashes at the cost of benign leaks at process exit.
frame/pluginloader.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 found 1 issue, and left some high level feedback:

  • Consider reducing the noise of the qWarning in the destructor (e.g., only logging once or behind a debug/verbosity flag), since static destruction paths can be part of normal shutdown and this message may appear on every exit.
  • To make the behavior clearer and avoid future regressions, it might be helpful to centralize the shutdown logic (e.g., track via a flag when destroy() has been called during aboutToQuit) so the destructor can distinguish between an intentionally cleaned-up instance and a late static-destruction case.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider reducing the noise of the `qWarning` in the destructor (e.g., only logging once or behind a debug/verbosity flag), since static destruction paths can be part of normal shutdown and this message may appear on every exit.
- To make the behavior clearer and avoid future regressions, it might be helpful to centralize the shutdown logic (e.g., track via a flag when `destroy()` has been called during `aboutToQuit`) so the destructor can distinguish between an intentionally cleaned-up instance and a late static-destruction case.

## Individual Comments

### Comment 1
<location> `frame/pluginloader.cpp:238-242` </location>
<code_context>
+    if (qApp && !QCoreApplication::closingDown()) {
+        // 正常情况:在 aboutToQuit 中调用的 destroy(),Qt 资源有效
+        destroy();
+    } else {
+        // 异常情况:静态析构阶段,Qt 全局资源可能已清理
+        // 不调用 destroy(),避免触发子对象析构
+        // 内存泄漏无关紧要,程序即将退出,操作系统会回收所有资源
+        qWarning("DPluginLoader destroyed after Qt global cleanup, skipping child destruction to avoid crash.");
+    }
 }
</code_context>

<issue_to_address>
**issue (bug_risk):** Using `qWarning` in the branch that assumes Qt is already torn down could still touch Qt-global state.

Since this path assumes Qt globals may already be torn down, `qWarning` itself could still touch Qt internals (logging categories, handlers, TLS, etc.) and reintroduce a crash risk. Consider either dropping this log or replacing it with a mechanism independent of Qt (e.g., `fprintf(stderr, ...)`) if this code can run after Qt shutdown.
</issue_to_address>

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.

1. Modified DPluginLoader destructor to check Qt application state
before calling destroy()
2. Added condition to only call destroy() when qApp exists and
application is not closing down
3. Added warning message when skipping destruction to avoid crash during
static cleanup
4. This prevents crashes when DPluginLoader is destroyed after Qt global
resources have been cleaned up

Log: Fixed potential crash during application shutdown when using static
plugin loader

Influence:
1. Test application shutdown process with static plugin loader
2. Verify no crashes occur during normal application exit
3. Test edge cases where Qt resources might be cleaned up early
4. Monitor for warning messages about skipped destruction during
abnormal shutdown
5. Verify memory cleanup still occurs properly during normal shutdown

fix: 防止 DPluginLoader 静态析构时崩溃

1. 修改 DPluginLoader 析构函数,在调用 destroy() 前检查 Qt 应用程序状态
2. 添加条件判断,仅当 qApp 存在且应用程序未关闭时才调用 destroy()
3. 添加警告信息,当跳过析构以避免静态清理期间的崩溃时进行提示
4. 防止在 Qt 全局资源已清理后销毁 DPluginLoader 时发生崩溃

Log: 修复使用静态插件加载器时应用程序关闭可能崩溃的问题

Influence:
1. 测试使用静态插件加载器时的应用程序关闭过程
2. 验证正常应用程序退出时不会发生崩溃
3. 测试 Qt 资源可能提前清理的边缘情况
4. 监控异常关闭期间关于跳过析构的警告信息
5. 验证正常关闭期间内存清理仍能正确进行

PMS: BUG-350887
@deepin-ci-robot
Copy link

deepin pr auto review

这段代码修改主要针对 DPluginLoader 的析构函数进行了安全性增强,以防止在程序退出阶段因 Qt 全局资源被提前释放而导致的崩溃。以下是对该代码的详细审查和改进意见:

1. 语法逻辑审查

当前实现

DPluginLoader::~DPluginLoader()
{
    if (qApp && !QCoreApplication::closingDown()) {
        destroy();
    } else {
        // 不调用 destroy(),避免触发子对象析构
    }
}

分析

  • 逻辑清晰,通过检查 qAppQCoreApplication::closingDown() 判断是否处于正常退出阶段。
  • 在静态析构阶段(Qt 资源已清理)跳过 destroy() 调用,避免访问无效资源。

改进建议

  • 可以进一步明确注释中的“正常情况”和“异常情况”的具体场景,例如:
    // 正常情况:Qt 事件循环尚未结束,资源有效
    // 异常情况:静态析构阶段,Qt 资源已释放

2. 代码质量审查

优点

  • 注释详细,解释了修改的背景和原因。
  • 避免了静态析构时的潜在崩溃问题。

改进建议

  1. 代码风格

    • if-else 改为更简洁的写法(如果逻辑允许):
      if (qApp && !QCoreApplication::closingDown()) {
          destroy();
      }
    • 如果 else 分支为空,可以省略 else 块。
  2. 可读性

    • 将注释拆分为多行,提高可读性:
      // 防止在 Qt 全局资源清理后析构导致崩溃
      // 当作为 Q_APPLICATION_STATIC 静态对象时,析构可能发生在 Qt 全局资源清理之后
      // 此时子 Applet 的析构可能访问已清理的 Qt 全局资源(如 QThreadStorage)

3. 代码性能审查

当前实现

  • 检查 qAppQCoreApplication::closingDown() 的开销很小,不会影响性能。

改进建议

  • 无需优化,性能已足够高效。

4. 代码安全审查

潜在问题

  1. 静态析构顺序问题

    • 如果 DPluginLoader 是静态对象,且其他静态对象依赖它,可能会在静态析构阶段出现未定义行为。
    • 改进方法:确保 DPluginLoader 是最后析构的静态对象(例如使用 Q_GLOBAL_STATIC)。
  2. 内存泄漏

    • 跳过 destroy() 会导致内存泄漏,但注释已说明“程序即将退出,操作系统会回收所有资源”,这是可以接受的。
  3. 线程安全

    • 如果 destroy() 不是线程安全的,且析构函数可能在多线程环境中调用,需要加锁保护。

改进建议

  1. 使用 Q_GLOBAL_STATIC 替代手动静态对象:

    Q_GLOBAL_STATIC(DPluginLoader, loaderInstance)
    • Q_GLOBAL_STATIC 会自动管理静态对象的生命周期,避免析构顺序问题。
  2. 确保 destroy() 是线程安全的:

    • 如果 destroy() 涉及多线程操作,使用 QMutex 保护:
      QMutex mutex;
      void DPluginLoader::destroy() {
          QMutexLocker locker(&mutex);
          // 清理逻辑
      }

5. 最终改进版本

DPluginLoader::~DPluginLoader()
{
    // 防止在 Qt 全局资源清理后析构导致崩溃
    // 当作为 Q_APPLICATION_STATIC 静态对象时,析构可能发生在 Qt 全局资源清理之后
    // 此时子 Applet 的析构可能访问已清理的 Qt 全局资源(如 QThreadStorage)
    if (qApp && !QCoreApplication::closingDown()) {
        // 正常情况:Qt 事件循环尚未结束,资源有效
        destroy();
    }
    // 异常情况:静态析构阶段,Qt 资源已释放,跳过 destroy()
}

总结

  • 优点:逻辑清晰,注释详细,解决了静态析构时的崩溃问题。
  • 改进点
    1. 使用 Q_GLOBAL_STATIC 替代手动静态对象。
    2. 确保 destroy() 是线程安全的。
    3. 简化代码风格(省略空 else 块)。
  • 安全性:当前实现已足够安全,但需注意静态析构顺序和线程安全。

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