Skip to content

Comments

fix: prevent crash during static destruction of DPluginLoader#1448

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
18202781743:fix
Feb 24, 2026
Merged

fix: prevent crash during static destruction of DPluginLoader#1448
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
18202781743:fix

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.

@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.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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

这段代码修改主要是为了解决在程序退出时,静态对象析构顺序可能导致的崩溃问题。这是一个典型的 C++ "静态析构灾难" (Static Destruction Order Fiasco) 问题。

以下是对这段代码的详细审查意见:

1. 语法逻辑

审查结果:通过

  • 代码逻辑清晰,使用了 qAppQCoreApplication::closingDown() 来判断当前 Qt 的状态。
  • 条件分支明确:如果 Qt 核心对象仍然存在且未处于关闭流程,则执行清理;否则跳过清理。
  • 注释详细解释了为什么要这样做(防止访问已释放的 QThreadStorage 等全局资源)。

2. 代码质量

审查结果:良好,建议微调

  • 优点:注释非常详尽,清晰地解释了潜在的崩溃场景和解决方案(即程序退出时的内存泄漏是可以接受的)。这有助于后续维护者理解这段看似"泄漏"的代码的意图。
  • 建议
    • 虽然逻辑正确,但依赖 QCoreApplication::closingDown() 来决定是否析构子对象是一种防御性编程。建议确认 destroy() 函数内部是否也涉及了非 Qt 的系统资源(如文件句柄、网络连接等)。
      • 如果 destroy() 仅涉及 Qt 对象和内存释放,当前的修改是完美的。
      • 如果 destroy() 涉及必须释放的系统资源(例如需要写入配置文件、断开数据库连接),即使 Qt UI 资源失效了,这些操作可能仍需执行。如果是这种情况,可能需要将 destroy() 拆分为 destroyQtResources()destroySystemResources(),后者即使在静态析构阶段也应执行。

3. 代码性能

审查结果:无影响

  • 这段代码位于析构函数中,仅在程序退出时执行一次。
  • 增加的判断逻辑(if 条件)开销极低,对性能无影响。

4. 代码安全

审查结果:通过

  • 安全性提升:该修改显著提高了程序的稳定性。在静态析构阶段访问已释放的 Qt 全局对象(如 QThreadStorage)会导致未定义行为(通常是段错误)。通过在检测到 Qt 环境即将失效时放弃析构子对象,有效避免了这种崩溃。
  • 资源泄漏说明:正如代码注释所言,在进程退出时让操作系统回收内存是标准的 C++ 实践,不属于安全漏洞。

总结与改进建议

这段代码是一个高质量的防御性修改,有效解决了静态对象析构顺序导致的崩溃问题。

改进建议(可选):
如果未来 destroy() 函数的逻辑变得复杂,建议重构析构逻辑,例如:

DPluginLoader::~DPluginLoader()
{
    // 1. 始终执行非 Qt 依赖的清理(如保存关键配置)
    cleanupSystemResources(); 

    // 2. 仅在 Qt 环境安全时执行 Qt 对象清理
    if (qApp && !QCoreApplication::closingDown()) {
        destroyQtObjects();
    }
    // 否则,依赖操作系统回收内存
}

结论: 当前修改是合理的,可以直接合并。它正确地权衡了"程序退出时的完美清理"与"程序退出时的稳定性"。

@18202781743
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link

deepin-bot bot commented Feb 24, 2026

This pr force merged! (status: blocked)

@deepin-bot deepin-bot bot merged commit 08ae201 into linuxdeepin:master Feb 24, 2026
8 of 11 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.

4 participants