fix: prevent crash during static destruction of DPluginLoader #1448
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
18202781743
requested review from
BLumia,
fly602,
mhduiy,
robertkill,
wjyrich and
yixinshark
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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
|
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 Initialization Order Fiasco）。 以下是对这段代码的详细审查意见，分为逻辑、质量、性能和安全四个方面： 1. 语法逻辑评价：逻辑正确，处理了边界情况。
2. 代码质量评价：良好，注释清晰，权衡合理。
3. 代码性能评价：无影响。
4. 代码安全评价：提高了程序的稳定性（安全性）。
总结这是一个高质量、必要的修复。 它正确地处理了 C++ 静态析构顺序问题，通过牺牲进程退出时刻的“完美清理”（实际上由操作系统代劳），换取了程序退出时的稳定性。 最终建议： |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
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.
Comment on lines
+238
to
+242
| } else { | ||
| // 异常情况：静态析构阶段，Qt 全局资源可能已清理 | ||
| // 不调用 destroy()，避免触发子对象析构 | ||
| // 内存泄漏无关紧要，程序即将退出，操作系统会回收所有资源 | ||
| qWarning("DPluginLoader destroyed after Qt global cleanup, skipping child destruction to avoid crash."); |
There was a problem hiding this comment.
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.
BLumia
reviewed
Feb 11, 2026
| // 异常情况：静态析构阶段，Qt 全局资源可能已清理 | ||
| // 不调用 destroy()，避免触发子对象析构 | ||
| // 内存泄漏无关紧要，程序即将退出，操作系统会回收所有资源 | ||
| qWarning("DPluginLoader destroyed after Qt global cleanup, skipping child destruction to avoid crash."); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: