fix: prevent crash during static destruction of DPluginLoader#1448
fix: prevent crash during static destruction of DPluginLoader#1448deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe 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 shutdownsequenceDiagram
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
Class diagram for updated DPluginLoader destructor logicclassDiagram
class DPluginLoader {
+DPluginLoader()
+~DPluginLoader()
+static DPluginLoader *instance()
-void destroy()
}
class QCoreApplication {
+static bool closingDown()
}
class QApplication {
}
DPluginLoader ..> QCoreApplication : checks_closingDown
DPluginLoader ..> QApplication : uses_qApp
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider reducing the noise of the
qWarningin 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 duringaboutToQuit) 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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 pr auto review这段代码修改主要是为了解决在程序退出时,静态对象析构顺序可能导致的崩溃问题。这是一个典型的 C++ "静态析构灾难" (Static Destruction Order Fiasco) 问题。 以下是对这段代码的详细审查意见: 1. 语法逻辑审查结果:通过
2. 代码质量审查结果:良好,建议微调
3. 代码性能审查结果:无影响
4. 代码安全审查结果:通过
总结与改进建议这段代码是一个高质量的防御性修改,有效解决了静态对象析构顺序导致的崩溃问题。 改进建议(可选): DPluginLoader::~DPluginLoader()
{
// 1. 始终执行非 Qt 依赖的清理(如保存关键配置)
cleanupSystemResources();
// 2. 仅在 Qt 环境安全时执行 Qt 对象清理
if (qApp && !QCoreApplication::closingDown()) {
destroyQtObjects();
}
// 否则,依赖操作系统回收内存
}结论: 当前修改是合理的,可以直接合并。它正确地权衡了"程序退出时的完美清理"与"程序退出时的稳定性"。 |
|
/forcemerge |
|
This pr force merged! (status: blocked) |
before calling destroy()
application is not closing down
static cleanup
resources have been cleaned up
Log: Fixed potential crash during application shutdown when using static
plugin loader
Influence:
abnormal shutdown
fix: 防止 DPluginLoader 静态析构时崩溃
Log: 修复使用静态插件加载器时应用程序关闭可能崩溃的问题
Influence:
PMS: BUG-350887
Summary by Sourcery
Bug Fixes: