fix: prevent crash during preview item handling#512
Merged
zccrs merged 1 commit intolinuxdeepin:masterfrom Aug 14, 2025
Merged
Conversation
1. Replace WWrapPointer with QPointer for SurfaceWrapper 2. Add shellSurface() check before accessing preview item properties 3. Fix race condition where SurfaceWrapper might be invalid The changes prevent a crash that occurred when accessing a SurfaceWrapper object after it had been invalidated. Using QPointer with an explicit shellSurface() validity check ensures we don't access dangling pointers, as the SurfaceWrapper::aboutToBeInvalidated signal doesn't nullify WWrapPointer in time. fix: 修复预览项处理时的崩溃问题 1. 将 SurfaceWrapper 的指针类型从 WWrapPointer 改为 QPointer 2. 在访问预览项属性前增加 shellSurface() 有效性检查 3. 解决 SurfaceWrapper 被标记为无效后仍被访问的竞态条件 这些修改解决了在 SurfaceWrapper 对象被销毁后仍然尝试访问其属性导致的崩溃 问题。使用 QPointer 并配合显式的 shellSurface() 有效性检查,可以确保不会 访问到悬空指针,因为 SurfaceWrapper::aboutToBeInvalidated 信号无法及时将 WWrapPointer 置空。
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis pull request replaces the custom WWrapPointer with Qt’s QPointer for tracking SurfaceWrapper, and introduces an explicit shellSurface() validity check in the preview flow to eliminate a race condition that could lead to dangling pointer access. Sequence diagram for preview item handling with validity checksequenceDiagram
participant Workspace
participant SurfaceWrapper
participant Model
Workspace->>SurfaceWrapper: Check shellSurface() validity
alt shellSurface() is valid
Workspace->>Model: modelFromId(workspaceId())
Workspace->>SurfaceWrapper: setOpacity(...)
Workspace->>SurfaceWrapper: setHideByWorkspace(...)
else shellSurface() is invalid
Workspace-->>Workspace: Skip property access
end
Class diagram for Workspace pointer management updateclassDiagram
class Workspace {
- QPointer<SurfaceWrapper> m_previewingItem
+ void startPreviewing(SurfaceWrapper *previewingItem)
}
class SurfaceWrapper {
+ shellSurface()
+ setOpacity(double)
+ setHideByWorkspace(bool)
}
Workspace --> SurfaceWrapper : QPointer
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @wineee - I've reviewed your changes - here's some feedback:
- Remove the obsolete TODO and consolidate SurfaceWrapper invalidation logic instead of relying on ad-hoc shellSurface() checks in callers.
- Encapsulate the shellSurface() validity check into a small helper method to improve readability and avoid duplicating the check.
- Consider resetting m_previewingItem in SurfaceWrapper::aboutToBeInvalidated so QPointer is nullified automatically and caller code can be simplified.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove the obsolete TODO and consolidate SurfaceWrapper invalidation logic instead of relying on ad-hoc shellSurface() checks in callers.
- Encapsulate the shellSurface() validity check into a small helper method to improve readability and avoid duplicating the check.
- Consider resetting m_previewingItem in SurfaceWrapper::aboutToBeInvalidated so QPointer is nullified automatically and caller code can be simplified.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zccrs
approved these changes
Aug 14, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wineee, zccrs 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
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.
The changes prevent a crash that occurred when accessing a SurfaceWrapper object after it had been invalidated. Using QPointer with an explicit shellSurface() validity check ensures we don't access dangling pointers, as the SurfaceWrapper::aboutToBeInvalidated signal doesn't nullify WWrapPointer in time.
fix: 修复预览项处理时的崩溃问题
这些修改解决了在 SurfaceWrapper 对象被销毁后仍然尝试访问其属性导致的崩溃
问题。使用 QPointer 并配合显式的 shellSurface() 有效性检查,可以确保不会
访问到悬空指针,因为 SurfaceWrapper::aboutToBeInvalidated 信号无法及时将 WWrapPointer 置空。
Summary by Sourcery
Prevent crashes during workspace preview by replacing WWrapPointer with QPointer and adding explicit shellSurface() validity checks
Bug Fixes: