fix: fix autoExitTimer crash in DBusControlCenterGrandSearchService#3309
Closed
fly602 wants to merge 0 commit into
Closed
fix: fix autoExitTimer crash in DBusControlCenterGrandSearchService#3309fly602 wants to merge 0 commit into
fly602 wants to merge 0 commit into
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds QPointer-based lifetime protection around the auto-exit timer callback in DBusControlCenterGrandSearchService and ensures the timer is safely disconnected and stopped in the destructor to prevent use-after-free crashes. Sequence diagram for autoExitTimer with QPointer lifetime protectionsequenceDiagram
participant Service as DBusControlCenterGrandSearchService
participant Manager as DccManager
participant AutoExitTimer as QTimer
participant App as QCoreApplication
Service->>AutoExitTimer: setInterval(10000)
Service->>AutoExitTimer: setSingleShot(true)
Service->>AutoExitTimer: connect(timeout, lambda[self, manager])
Service->>AutoExitTimer: start()
AutoExitTimer-->>Service: timeout()
alt [self && manager]
AutoExitTimer->>Manager: mainWindow()
Manager-->>AutoExitTimer: mainWindow
AutoExitTimer->>Manager: mainWindow()->isVisible()
alt [!isVisible]
AutoExitTimer->>App: quit()
end
else [!self || !manager]
AutoExitTimer-->>AutoExitTimer: return
end
destroy Service
Service->>AutoExitTimer: disconnect(m_autoExitTimer, nullptr, this, nullptr)
Service->>AutoExitTimer: stop()
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 - I've found 1 issue, and left some high level feedback:
- The local
QPointer<DBusControlCenterGrandSearchService> selfis captured into the lambda but never used, so it can be removed from both the local variables and capture list to simplify the code. - In the destructor, consider guarding the
disconnect/stopcalls with a null check onm_autoExitTimer(or a clear invariant comment) to make it explicit that the timer is always constructed and valid at this point.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The local `QPointer<DBusControlCenterGrandSearchService> self` is captured into the lambda but never used, so it can be removed from both the local variables and capture list to simplify the code.
- In the destructor, consider guarding the `disconnect`/`stop` calls with a null check on `m_autoExitTimer` (or a clear invariant comment) to make it explicit that the timer is always constructed and valid at this point.
## Individual Comments
### Comment 1
<location path="src/dde-control-center/controlcenterdbusadaptor.cpp" line_range="149-157" />
<code_context>
- if (!this->parent()->mainWindow()->isVisible())
+
+ // 使用 QPointer 同时保护 adaptor 和 manager，防止 Use-After-Free
+ QPointer<DBusControlCenterGrandSearchService> self = this;
+ QPointer<DccManager> manager = parent;
+
+ connect(m_autoExitTimer, &QTimer::timeout, this, [self, manager]() {
+ // 检查 adaptor 和 manager 都还存在
+ if (self && manager && !manager->mainWindow()->isVisible())
</code_context>
<issue_to_address>
**suggestion:** Consider simplifying lifetime handling by avoiding redundant `QPointer` + context usage.
Because `connect` uses `this` as the context, Qt will already disconnect the lambda when `DBusControlCenterGrandSearchService` is destroyed, so `QPointer<...> self` guards the same lifetime twice. That redundancy makes the lifetime model harder to follow without adding real safety.
You could either:
- Keep the context object (`this`) and drop `self` from the capture, only guarding `manager` with `QPointer`, or
- Use `nullptr` as the context and rely solely on `QPointer` for both `self`/`manager`, avoiding raw `this`.
Both options keep use‑after‑free protection while simplifying lifetime management.
```suggestion
// 使用 QPointer 保护 manager，避免管理器销毁后访问悬空指针
QPointer<DccManager> manager = parent;
connect(m_autoExitTimer, &QTimer::timeout, this, [manager]() {
// 检查 manager 仍然存在且主窗口不可见
if (manager && !manager->mainWindow()->isVisible())
QCoreApplication::quit();
});
```
</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
149
to
157
| // 使用 QPointer 同时保护 adaptor 和 manager，防止 Use-After-Free | ||
| QPointer<DBusControlCenterGrandSearchService> self = this; | ||
| QPointer<DccManager> manager = parent; | ||
|
|
||
| connect(m_autoExitTimer, &QTimer::timeout, this, [self, manager]() { | ||
| // 检查 adaptor 和 manager 都还存在 | ||
| if (self && manager && !manager->mainWindow()->isVisible()) | ||
| QCoreApplication::quit(); | ||
| }); |
There was a problem hiding this comment.
suggestion: Consider simplifying lifetime handling by avoiding redundant QPointer + context usage.
Because connect uses this as the context, Qt will already disconnect the lambda when DBusControlCenterGrandSearchService is destroyed, so QPointer<...> self guards the same lifetime twice. That redundancy makes the lifetime model harder to follow without adding real safety.
You could either:
- Keep the context object (
this) and dropselffrom the capture, only guardingmanagerwithQPointer, or - Use
nullptras the context and rely solely onQPointerfor bothself/manager, avoiding rawthis.
Both options keep use‑after‑free protection while simplifying lifetime management.
Suggested change
| // 使用 QPointer 同时保护 adaptor 和 manager，防止 Use-After-Free | |
| QPointer<DBusControlCenterGrandSearchService> self = this; | |
| QPointer<DccManager> manager = parent; | |
| connect(m_autoExitTimer, &QTimer::timeout, this, [self, manager]() { | |
| // 检查 adaptor 和 manager 都还存在 | |
| if (self && manager && !manager->mainWindow()->isVisible()) | |
| QCoreApplication::quit(); | |
| }); | |
| // 使用 QPointer 保护 manager，避免管理器销毁后访问悬空指针 | |
| QPointer<DccManager> manager = parent; | |
| connect(m_autoExitTimer, &QTimer::timeout, this, [manager]() { | |
| // 检查 manager 仍然存在且主窗口不可见 | |
| if (manager && !manager->mainWindow()->isVisible()) | |
| QCoreApplication::quit(); | |
| }); |
deepin pr auto review★ 总体评分：100分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // 当前代码已为最佳实践，无需进一步修改，此处展示其核心逻辑以供参考
#include <QPointer>
DBusControlCenterGrandSearchService::DBusControlCenterGrandSearchService(DccManager *parent)
: QDBusAbstractAdaptor(parent)
, m_autoExitTimer(new QTimer(this))
{
m_autoExitTimer->setInterval(10000);
m_autoExitTimer->setSingleShot(true);
QPointer<DBusControlCenterGrandSearchService> self = this;
QPointer<DccManager> manager = parent;
connect(m_autoExitTimer, &QTimer::timeout, this, [self, manager]() {
if (self && manager && !manager->mainWindow()->isVisible())
QCoreApplication::quit();
});
m_autoExitTimer->start();
}
DBusControlCenterGrandSearchService::~DBusControlCenterGrandSearchService()
{
disconnect(m_autoExitTimer, nullptr, this, nullptr);
m_autoExitTimer->stop();
} |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602 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 |
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
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.
Log: Fixed control center crash caused by autoExitTimer Use-After-Free
Influence:
fix: 修复DBusControlCenterGrandSearchService中autoExitTimer崩溃问题
Log: 修复控制中心因autoExitTimer Use-After-Free导致的崩溃
Influence:
PMS: BUG-362939
Summary by Sourcery
Prevent DBus control center grand search auto-exit timer from causing use-after-free crashes by guarding lifetime and cleaning up on destruction.
Bug Fixes: