fix: fix application tray crash due to parent-child ownership and null pointer issues#464
Open
18202781743 wants to merge 7 commits into
Open
fix: fix application tray crash due to parent-child ownership and null pointer issues#46418202781743 wants to merge 7 commits into
18202781743 wants to merge 7 commits into
Conversation
Add XdgActivationClient that uses QWaylandClientExtension to bind dde-shell's xdg_activation_v1 global and request activation tokens via standard Wayland protocol before calling SNI Activate. Modified sniprotocolhandler to request token asynchronously before calling Activate, setting XDG_ACTIVATION_TOKEN env var. Log: SNI托盘图标Wayland下点击时申请xdgactivation token激活DBus窗口 Issue: linuxdeepin#6
Use captured provider pointer instead of sender() which returns nullptr in lambda context and causes crash when calling deleteLater(). Fixes: sender()->deleteLater() crash
pointer issues 1. Fix crash in DDEindicatorTrayProtocol by using window() instead of parent() for event filter and content updates 2. Add null pointer checks in DDEindicatorTrayProtocol event filter 3. Fix crash in SniTrayProtocolHandler by using window() instead of parent() and adding null checks 4. Fix tooltip removal order in TrayPlugin to avoid dangling pointer references 5. Fix double-delete crash in TrayWidget by not setting handler as child widget 6. Add null handler checks in TrayWidget showEvent and paintEvent Log: Fixed application tray crashes when tray items are added/removed Influence: 1. Test adding and removing multiple tray items 2. Test tray indicator protocol operations 3. Test sni protocol tray operations 4. Test tray widget paint and show events 5. Test tray icon updates and tooltip display 6. Test application tray with various indicator types feat: 修复应用托盘因父子关系和空指针导致的崩溃问题 1. 修复 DDEindicatorTrayProtocol 中，使用 window() 替代 parent() 进行事 件过滤和内容更新 2. 在 DDEindicatorTrayProtocol 的事件过滤器中添加空指针检查 3. 修复 SniTrayProtocolHandler 中，使用 window() 替代 parent() 并添加空 指针检查 4. 修复 TrayPlugin 中 tooltip 移除顺序，避免悬空指针引用 5. 修复 TrayWidget 中不将 handler 设置为子控件导致的二次删除崩溃 6. 在 TrayWidget 的 showEvent 和 paintEvent 中添加 handler 空指针检查 Log: 修复添加/移除托盘项时应用托盘崩溃的问题 Influence: 1. 测试添加和移除多个托盘项 2. 测试托盘指示协议操作 3. 测试 sni 协议托盘操作 4. 测试托盘控件的绘制和显示事件 5. 测试托盘图标更新和提示信息显示 6. 测试各种指示类型下的应用托盘功能
|
[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 GuideRefactors application tray SNI handling to avoid parent/child vs smart-pointer double-ownership, fixes null/dangling pointer paths in event handling and painting, and adds Wayland xdg-activation integration so left-click activation is properly tokenized on Wayland while keeping X11 behavior intact. Sequence diagram for SNI left-click activation with Wayland xdg-activationsequenceDiagram
actor User
participant TrayWidget
participant SniTrayProtocolHandler
participant XdgActivationClient
participant StatusNotifierItem
User->>TrayWidget: [Left click]
TrayWidget->>SniTrayProtocolHandler: eventFilter(QEvent)
alt [activationClient isActive]
SniTrayProtocolHandler->>XdgActivationClient: instance()
SniTrayProtocolHandler->>XdgActivationClient: requestToken(QWindow, QString, TokenCallback)
XdgActivationClient-->>SniTrayProtocolHandler: TokenCallback(token)
alt [token not empty]
SniTrayProtocolHandler->>StatusNotifierItem: ProvideXdgActivationToken(token)
end
SniTrayProtocolHandler->>StatusNotifierItem: Activate(0, 0)
else [activationClient not active]
SniTrayProtocolHandler->>StatusNotifierItem: Activate(0, 0)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review这份代码变更主要实现了在 Wayland 环境下通过 整体来看，代码逻辑清晰，对 Wayland 底层协议的封装和使用基本正确。但经过仔细审查，在语法逻辑、代码质量、性能和安全方面仍有以下改进意见： 1. 语法与逻辑1.1 单例模式存在线程安全与内存泄漏风险文件: XdgActivationClient *XdgActivationClient::instance()
{
static XdgActivationClient *s_instance = nullptr;
if (!s_instance) {
s_instance = new XdgActivationClient(qApp);
}
return s_instance;
}问题:
改进建议: XdgActivationClient *XdgActivationClient::instance()
{
static XdgActivationClient s_instance{qApp};
return &s_instance;
}注意：如果改为这种方式，需要将构造函数和析构函数从 private 移到 public（或保持 private 并通过 friend 访问），并删除类声明中的 1.2
|
![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 2 issues, and left some high level feedback:
- In the left-button branch of
SniTrayProtocolHandler::eventFilter,window()->windowHandle()is used without a null check onwindow(), unlike the right-click path and other handlers; consider guardingwindow()itself to avoid a null dereference when no window is associated. - The new
XdgActivationClientclass is declared in thetraynamespace, but the call sites (e.g., insniprotocolhandler.cpp) referenceXdgActivationClient::instance()without a namespace qualifier; either addusing namespace tray;locally or fully qualify the type to ensure this compiles and avoids namespace ambiguity. - In
plugins/application-tray/CMakeLists.txt,Qt${QT_VERSION_MAJOR}::GuiPrivateandQt${QT_VERSION_MAJOR}::WaylandClientPrivateare linked unconditionally, but the correspondingfind_packagecalls are gated onQt${QT_VERSION_MAJOR}_VERSION >= 6.10; for earlier Qt versions these targets may not exist, so consider guarding their usage or linking only when the private components are actually found.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the left-button branch of `SniTrayProtocolHandler::eventFilter`, `window()->windowHandle()` is used without a null check on `window()`, unlike the right-click path and other handlers; consider guarding `window()` itself to avoid a null dereference when no window is associated.
- The new `XdgActivationClient` class is declared in the `tray` namespace, but the call sites (e.g., in `sniprotocolhandler.cpp`) reference `XdgActivationClient::instance()` without a namespace qualifier; either add `using namespace tray;` locally or fully qualify the type to ensure this compiles and avoids namespace ambiguity.
- In `plugins/application-tray/CMakeLists.txt`, `Qt${QT_VERSION_MAJOR}::GuiPrivate` and `Qt${QT_VERSION_MAJOR}::WaylandClientPrivate` are linked unconditionally, but the corresponding `find_package` calls are gated on `Qt${QT_VERSION_MAJOR}_VERSION >= 6.10`; for earlier Qt versions these targets may not exist, so consider guarding their usage or linking only when the private components are actually found.
## Individual Comments
### Comment 1
<location path="plugins/application-tray/sniprotocolhandler.cpp" line_range="327-328" />
<code_context>
+ if (!win)
+ return false;
+
+ auto sniInter = m_sniInter;
+ activationClient->requestToken(win, QString(), [sniInter](const QString &token) {
+ if (!token.isEmpty()) {
+ sniInter->ProvideXdgActivationToken(token);
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against a potentially dangling `sniInter` in the async activation callback.
The lambda captures `sniInter` as a raw pointer and may run after the tray item is removed, leading to undefined behavior when calling `ProvideXdgActivationToken` / `Activate` on a freed object.
Use a guarded pointer and check it before use, e.g.:
```cpp
QPointer<decltype(m_sniInter)> sniInter = m_sniInter;
activationClient->requestToken(win, QString(), [sniInter](const QString &token) {
if (!sniInter)
return;
if (!token.isEmpty()) {
sniInter->ProvideXdgActivationToken(token);
}
sniInter->Activate(0, 0);
});
```
This prevents the async callback from accessing freed memory if the SNI interface disappears while the request is in flight.
</issue_to_address>
### Comment 2
<location path="plugins/application-tray/sniprotocolhandler.cpp" line_range="323-324" />
<code_context>
- m_sniInter->Activate(0, 0);
+ auto *activationClient = XdgActivationClient::instance();
+ if (activationClient->isActive()) {
+ auto *win = window()->windowHandle();
+ if (!win)
+ return false;
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider a fallback path when `windowHandle()` is null to avoid silently dropping clicks.
When `window()->windowHandle()` is null, this now returns `false` and drops the click, whereas before we still activated via `Activate(0, 0)`. This can occur when the window isn’t yet backed by a native handle. To avoid a behavior regression and lost clicks, consider either falling back to the previous activation path when `win` is null, or at least logging this case and still calling `Activate(0, 0)` without a token.
Suggested implementation:
```cpp
auto *win = window()->windowHandle();
if (!win) {
qCWarning(LOG_SNI) << "No native window handle available for XDG activation, falling back to legacy SNI activation";
m_sniInter->Activate(0, 0);
return false;
}
```
These changes assume:
1. `LOG_SNI` is an existing logging category defined elsewhere (commonly in `util.h` in this codebase). If it is named differently, replace `LOG_SNI` with the correct category.
2. `m_sniInter` is the same interface previously used for `Activate(0, 0)` and is still available in this context. If the member has a different name, adjust `m_sniInter` accordingly.
</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
+327
to
+328
| auto sniInter = m_sniInter; | ||
| activationClient->requestToken(win, QString(), [sniInter](const QString &token) { |
There was a problem hiding this comment.
issue (bug_risk): Guard against a potentially dangling sniInter in the async activation callback.
The lambda captures sniInter as a raw pointer and may run after the tray item is removed, leading to undefined behavior when calling ProvideXdgActivationToken / Activate on a freed object.
Use a guarded pointer and check it before use, e.g.:
QPointer<decltype(m_sniInter)> sniInter = m_sniInter;
activationClient->requestToken(win, QString(), [sniInter](const QString &token) {
if (!sniInter)
return;
if (!token.isEmpty()) {
sniInter->ProvideXdgActivationToken(token);
}
sniInter->Activate(0, 0);
});This prevents the async callback from accessing freed memory if the SNI interface disappears while the request is in flight.
Comment on lines
+323
to
+324
| auto *win = window()->windowHandle(); | ||
| if (!win) |
There was a problem hiding this comment.
suggestion (bug_risk): Consider a fallback path when windowHandle() is null to avoid silently dropping clicks.
When window()->windowHandle() is null, this now returns false and drops the click, whereas before we still activated via Activate(0, 0). This can occur when the window isn’t yet backed by a native handle. To avoid a behavior regression and lost clicks, consider either falling back to the previous activation path when win is null, or at least logging this case and still calling Activate(0, 0) without a token.
Suggested implementation:
auto *win = window()->windowHandle();
if (!win) {
qCWarning(LOG_SNI) << "No native window handle available for XDG activation, falling back to legacy SNI activation";
m_sniInter->Activate(0, 0);
return false;
}
These changes assume:
LOG_SNIis an existing logging category defined elsewhere (commonly inutil.hin this codebase). If it is named differently, replaceLOG_SNIwith the correct category.m_sniInteris the same interface previously used forActivate(0, 0)and is still available in this context. If the member has a different name, adjustm_sniInteraccordingly.
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.
问题描述
应用程序托盘在 SNI (StatusNotifierItem) 托盘项注销时崩溃。
根本原因
SniTrayProtocolHandler同时被QSharedPointer和 QObject 父子关系管理，导致双重删除m_tooltip中存储的 widget 指针在 handler 销毁后变成悬空指针eventFilter中使用parent()获取 widget，但移除setParent(this)后返回错误对象修复内容
traywidget.cpp
m_handler->setParent(this)避免双重生命周期管理showEvent和paintEvent中添加m_handler空指针检查sniprotocolhandler.cpp
eventFilter中将parent()替换为window()windowHandle()的空指针检查ddeindicatortrayprotocol.cpp
updateContent中将q->parent()替换为q->window()eventFilter中将parent()替换为window()，添加空指针检查trayplugin.cpp
removelambda 中，先清理m_tooltip，再调用itemRemoved，避免访问悬空指针测试
修复后需要验证：
🤖 Generated with Claude Code
Summary by Sourcery
Integrate Wayland xdg-activation support into the application tray and harden tray item lifecycle handling to prevent crashes when SNI items are removed.
New Features:
Bug Fixes:
Enhancements:
Build: