feat: add crash handler with cache cleanup #2891
Open
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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: caixr23 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 |
deepin pr auto review我来对这个 diff 进行代码审查：
改进建议： #ifndef QT_DEBUG
void sig_crash(int signum)
{
// 记录崩溃信息
qCritical() << "Application crashed with signal:" << signum;
// 崩溃了，尝试清下缓存，防止下次还起不来
QString qmlCache = QStandardPaths::writableLocation(QStandardPaths::GenericCacheLocation) + "/deepin/dde-control-center/qmlcache";
QDir dir(qmlCache);
if (dir.exists()) {
if (!dir.removeRecursively()) {
qWarning() << "Failed to remove QML cache directory:" << qmlCache;
}
}
// 重新触发信号，产生coredump
signal(signum, SIG_DFL);
raise(signum);
}
#endif
// 在 main 函数中添加错误检查
#ifndef QT_DEBUG
// 设置信号处理函数
struct sigaction sa;
sa.sa_handler = sig_crash;
sigemptyset(&sa.sa_mask);
sa.sa_flags = SA_RESETHAND;
// 检查信号处理函数设置是否成功
if (sigaction(SIGSEGV, &sa, nullptr) == -1 ||
sigaction(SIGILL, &sa, nullptr) == -1 ||
sigaction(SIGABRT, &sa, nullptr) == -1 ||
sigaction(SIGFPE, &sa, nullptr) == -1) {
qWarning() << "Failed to set signal handlers";
}
#endif这些改进可以提高代码的健壮性和可维护性，同时保持原有的功能不变。 |
Reviewer's GuideAdds a crash signal handler in main.cpp (enabled only in non-debug builds) that deletes the QML cache directory on crash, then re-raises the original signal so a core dump is still produced, and wires it up using sigaction for several common fatal signals. Sequence diagram for crash handling and QML cache cleanupsequenceDiagram
actor User
participant App as dde_control_center
participant OS as OperatingSystem
participant Handler as sig_crash
participant FS as FileSystem
User->>App: Run application
App->>OS: Execute
OS-->>App: Runtime execution
OS-->>App: Fatal signal (SIGSEGV/SIGILL/SIGABRT/SIGFPE)
OS->>Handler: Invoke sig_crash(signum)
Handler->>FS: Get GenericCacheLocation
Handler->>FS: Remove qmlcache directory recursively
Handler->>OS: signal(signum, SIG_DFL)
Handler->>OS: raise(signum)
OS-->>App: Deliver default signal handler
OS-->>User: Application terminates and core dump is generated
Flow diagram for sig_crash handler logicflowchart TD
A[Receive fatal signal in release build] --> B[Enter sig_crash with signum]
B --> C[Compute qmlCache path using QStandardPaths::GenericCacheLocation]
C --> D[Create QDir for qmlcache directory]
D --> E{Does qmlcache directory exist?}
E -- Yes --> F[Call removeRecursively on qmlcache]
E -- No --> G[Skip removal]
F --> H["Set default handler for signum using signal(signum, SIG_DFL)"]
G --> H["Set default handler for signum using signal(signum, SIG_DFL)"]
H --> I["Call raise(signum) to re-deliver signal"]
I --> J[Process receives default signal handler]
J --> K[Application terminates and core dump is produced]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The crash signal handler performs complex Qt and filesystem operations (QString, QDir, QStandardPaths, removeRecursively), which are not async-signal-safe and may lead to undefined behavior or deadlocks on crash; consider deferring cleanup to a safer mechanism (e.g., crash marker file checked on next startup) instead of doing it directly in the signal handler.
- You’re both setting SA_RESETHAND on the sigaction and calling signal(signum, SIG_DFL) inside the handler; this is redundant and can be simplified by relying on SA_RESETHAND and just calling raise(signum) (or using sigaction without manipulating handlers inside the callback).
- The struct sigaction sa is not zero-initialized, so any unused fields may contain garbage; it’s safer to use
struct sigaction sa = {};before assigning sa_handler, sa_mask, and sa_flags.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The crash signal handler performs complex Qt and filesystem operations (QString, QDir, QStandardPaths, removeRecursively), which are not async-signal-safe and may lead to undefined behavior or deadlocks on crash; consider deferring cleanup to a safer mechanism (e.g., crash marker file checked on next startup) instead of doing it directly in the signal handler.
- You’re both setting SA_RESETHAND on the sigaction and calling signal(signum, SIG_DFL) inside the handler; this is redundant and can be simplified by relying on SA_RESETHAND and just calling raise(signum) (or using sigaction without manipulating handlers inside the callback).
- The struct sigaction sa is not zero-initialized, so any unused fields may contain garbage; it’s safer to use `struct sigaction sa = {};` before assigning sa_handler, sa_mask, and sa_flags.
## Individual Comments
### Comment 1
<location> `src/dde-control-center/main.cpp:30-39` </location>
<code_context>
+DCORE_USE_NAMESPACE
+#ifndef QT_DEBUG
+void sig_crash(int signum)
+{
+ // 崩溃了，尝试清下缓存，防止下次还起不来
+ QString qmlCache = QStandardPaths::writableLocation(QStandardPaths::GenericCacheLocation) + "/deepin/dde-control-center/qmlcache";
+ QDir dir(qmlCache);
+ if (dir.exists()) {
+ dir.removeRecursively();
+ }
+ // 重新触发信号，产生coredump
+ signal(signum, SIG_DFL);
+ raise(signum);
+}
+#endif
</code_context>
<issue_to_address>
**issue (bug_risk):** The signal handler uses non–async-signal-safe Qt/stdlib APIs, which is undefined behavior and may prevent the core dump or cache cleanup from working reliably.
In `sig_crash`, calls to `QStandardPaths::writableLocation`, `QString` operations, and `QDir::removeRecursively` are not async-signal-safe, so using them in a crash handler (SIGSEGV/SIGABRT, etc.) is undefined behavior and can deadlock or interfere with core dump generation.
To keep the behavior safe while still clearing the qmlcache:
- Limit the handler to async-signal-safe actions only (e.g., set a `sig_atomic_t` flag or write to a pipe).
- Move cache cleanup to a safe context, such as on next startup (check a crash marker file and then clear the cache) or in a separate helper process outside the crashing context.
</issue_to_address>
### Comment 2
<location> `src/dde-control-center/main.cpp:75-79` </location>
<code_context>
{
+#ifndef QT_DEBUG
+ // 设置信号处理函数
+ struct sigaction sa;
+ sa.sa_handler = sig_crash;
+ sigemptyset(&sa.sa_mask);
+ // 在处理完信号后恢复默认信号处理
+ sa.sa_flags = SA_RESETHAND;
+
+ sigaction(SIGSEGV, &sa, nullptr);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `sigaction` struct is only partially initialized; it’s safer to fully zero-initialize it before use.
On some platforms `struct sigaction` has extra fields (e.g., `sa_restorer`) that you’re not initializing here, leaving them with indeterminate values. A safer pattern is to zero-initialize first:
```cpp
struct sigaction sa;
memset(&sa, 0, sizeof(sa));
sa.sa_handler = sig_crash;
sigemptyset(&sa.sa_mask);
sa.sa_flags = SA_RESETHAND;
```
This guarantees all fields are in a defined state across platforms.
Suggested implementation:
```cpp
#ifndef QT_DEBUG
// 设置信号处理函数
struct sigaction sa;
memset(&sa, 0, sizeof(sa));
sa.sa_handler = sig_crash;
sigemptyset(&sa.sa_mask);
// 在处理完信号后恢复默认信号处理
sa.sa_flags = SA_RESETHAND;
```
1. At the top of `src/dde-control-center/main.cpp`, ensure you have either `#include <cstring>` or `#include <string.h>` so that `memset` is declared.
</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
+30
to
+39
| void sig_crash(int signum) | ||
| { | ||
| // 崩溃了，尝试清下缓存，防止下次还起不来 | ||
| QString qmlCache = QStandardPaths::writableLocation(QStandardPaths::GenericCacheLocation) + "/deepin/dde-control-center/qmlcache"; | ||
| QDir dir(qmlCache); | ||
| if (dir.exists()) { | ||
| dir.removeRecursively(); | ||
| } | ||
| // 重新触发信号，产生coredump | ||
| signal(signum, SIG_DFL); |
There was a problem hiding this comment.
issue (bug_risk): The signal handler uses non–async-signal-safe Qt/stdlib APIs, which is undefined behavior and may prevent the core dump or cache cleanup from working reliably.
In sig_crash, calls to QStandardPaths::writableLocation, QString operations, and QDir::removeRecursively are not async-signal-safe, so using them in a crash handler (SIGSEGV/SIGABRT, etc.) is undefined behavior and can deadlock or interfere with core dump generation.
To keep the behavior safe while still clearing the qmlcache:
- Limit the handler to async-signal-safe actions only (e.g., set a
sig_atomic_tflag or write to a pipe). - Move cache cleanup to a safe context, such as on next startup (check a crash marker file and then clear the cache) or in a separate helper process outside the crashing context.
Comment on lines
+75
to
+79
| struct sigaction sa; | ||
| sa.sa_handler = sig_crash; | ||
| sigemptyset(&sa.sa_mask); | ||
| // 在处理完信号后恢复默认信号处理 | ||
| sa.sa_flags = SA_RESETHAND; |
There was a problem hiding this comment.
suggestion (bug_risk): The sigaction struct is only partially initialized; it’s safer to fully zero-initialize it before use.
On some platforms struct sigaction has extra fields (e.g., sa_restorer) that you’re not initializing here, leaving them with indeterminate values. A safer pattern is to zero-initialize first:
struct sigaction sa;
memset(&sa, 0, sizeof(sa));
sa.sa_handler = sig_crash;
sigemptyset(&sa.sa_mask);
sa.sa_flags = SA_RESETHAND;This guarantees all fields are in a defined state across platforms.
Suggested implementation:
#ifndef QT_DEBUG
// 设置信号处理函数
struct sigaction sa;
memset(&sa, 0, sizeof(sa));
sa.sa_handler = sig_crash;
sigemptyset(&sa.sa_mask);
// 在处理完信号后恢复默认信号处理
sa.sa_flags = SA_RESETHAND;
- At the top of
src/dde-control-center/main.cpp, ensure you have either#include <cstring>or#include <string.h>so thatmemsetis declared.
Added signal handlers for common crash signals (SIGSEGV, SIGILL, SIGABRT, SIGFPE) to automatically clean up QML cache directory when crashes occur in release builds. This prevents corrupted cache from causing repeated startup failures. The handler first removes the qmlcache directory, then re-raises the signal to generate core dump for debugging. Influence: 1. Test application startup after simulating crashes to verify cache cleanup works 2. Verify normal operation is unaffected in debug builds (QT_DEBUG defined) 3. Check that core dumps are still generated for crash analysis 4. Test QML loading performance after cache cleanup to ensure proper regeneration feat: 添加崩溃处理及缓存清理功能 为常见崩溃信号（SIGSEGV、SIGILL、SIGABRT、SIGFPE）添加信号处理程序，在 发布版本中发生崩溃时自动清理QML缓存目录。这可以防止损坏的缓存导致重复启 动失败。处理程序会先删除qmlcache目录，然后重新触发信号以生成核心转储用于 调试。 Influence: 1. 模拟崩溃后测试应用启动，验证缓存清理功能正常工作 2. 验证调试版本（定义了QT_DEBUG）的正常操作不受影响 3. 检查崩溃分析所需的核心转储是否仍然生成 4. 测试缓存清理后的QML加载性能，确保正确重新生成 PMS: BUG-313293
|
TAG Bot New tag: 6.1.64 |
|
TAG Bot New tag: 6.1.65 |
|
TAG Bot New tag: 6.1.66 |
|
TAG Bot New tag: 6.1.67 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Added signal handlers for common crash signals (SIGSEGV, SIGILL, SIGABRT, SIGFPE) to automatically clean up QML cache directory when crashes occur in release builds. This prevents corrupted cache from causing repeated startup failures. The handler first removes the qmlcache directory, then re-raises the signal to generate core dump for debugging.
Influence:
feat: 添加崩溃处理及缓存清理功能
为常见崩溃信号（SIGSEGV、SIGILL、SIGABRT、SIGFPE）添加信号处理程序，在
发布版本中发生崩溃时自动清理QML缓存目录。这可以防止损坏的缓存导致重复启
动失败。处理程序会先删除qmlcache目录，然后重新触发信号以生成核心转储用于
调试。
Influence:
PMS: BUG-313293
Summary by Sourcery
New Features: