fix: resolve multiple critical crashes in Qt Quick rendering#665
Merged
zccrs merged 1 commit intolinuxdeepin:masterfrom Dec 25, 2025
Merged
fix: resolve multiple critical crashes in Qt Quick rendering#665zccrs merged 1 commit intolinuxdeepin:masterfrom
zccrs merged 1 commit intolinuxdeepin:masterfrom
Conversation
Reviewer's GuideFixes three crash scenarios in Qt Quick rendering by guarding buffer resets against same-pointer cases to avoid double-unlock, deferring DataManager item destruction until after iteration to avoid use-after-free, and only marking items dirty when a valid window exists during resource release to avoid accessing destroyed scene graph nodes. Sequence diagram for buffer commit handling in WSurfaceItemContentsequenceDiagram
participant WaylandSurface as WaylandSurface
participant WSurfaceItemContent as WSurfaceItemContent
participant WSurfaceItemContentPrivate as WSurfaceItemContentPrivate
participant PendingBuffer as pendingBuffer
participant Buffer as buffer
participant WlrBuffer as wlr_buffer
WaylandSurface->>WSurfaceItemContent: commit()
WSurfaceItemContent->>WSurfaceItemContentPrivate: handleSurfaceCommit()
WSurfaceItemContentPrivate->>WaylandSurface: buffer()
WaylandSurface-->>WSurfaceItemContentPrivate: newBuffer
alt non_live_mode
WSurfaceItemContentPrivate->>PendingBuffer: get()
PendingBuffer-->>WSurfaceItemContentPrivate: oldPtr
WSurfaceItemContentPrivate->>WSurfaceItemContentPrivate: compare oldPtr != newBuffer
alt pointer_changed
WSurfaceItemContentPrivate->>PendingBuffer: reset(newBuffer)
PendingBuffer-->>WlrBuffer: unlock(oldPtr)
PendingBuffer->>WlrBuffer: lock(newBuffer)
else pointer_unchanged
WSurfaceItemContentPrivate->>WSurfaceItemContentPrivate: skip reset and lock
end
else live_mode
WSurfaceItemContentPrivate->>Buffer: get()
Buffer-->>WSurfaceItemContentPrivate: oldPtr
WSurfaceItemContentPrivate->>WSurfaceItemContentPrivate: compare oldPtr != newBuffer
alt pointer_changed
WSurfaceItemContentPrivate->>Buffer: reset(newBuffer)
Buffer-->>WlrBuffer: unlock(oldPtr)
Buffer->>WlrBuffer: lock(newBuffer)
WSurfaceItemContentPrivate->>WSurfaceItemContent: update()
else pointer_unchanged
WSurfaceItemContentPrivate->>WSurfaceItemContentPrivate: skip reset and lock
end
end
Sequence diagram for DataManager CleanJob deferred destructionsequenceDiagram
participant RenderThread as RenderThread
participant CleanJob as CleanJob
participant DataManager as DataManager
participant Data as Data
participant DataType as DataType
RenderThread->>CleanJob: run()
CleanJob->>DataManager: manager pointer
CleanJob->>DataManager: cleanJob = nullptr
CleanJob->>DataManager: tmp.swap(dataList)
CleanJob->>DataManager: dataList.reserve(tmp.size())
loop for each shared Data in tmp
CleanJob->>Data: check released
alt released > 2
CleanJob->>CleanJob: itemsToDestroy.append(data.data)
else released <= 2
CleanJob->>DataManager: dataList << data
CleanJob->>Data: update released counter
end
end
CleanJob->>CleanJob: check manager is not null
alt manager not null
loop for each item in itemsToDestroy
CleanJob->>DataManager: get()
DataManager-->>CleanJob: resourceManager
CleanJob->>DataType: destroy(item)
end
else manager null
CleanJob->>CleanJob: skip destruction to avoid use-after-free
end
Class diagram for updated Qt Quick rendering componentsclassDiagram
class QQuickItem {
}
class QQuickItemPrivate {
+void dirty(int flags)
}
class WSurfaceItemContent {
+void releaseResources()
+QWindow window()
}
class WSurfaceItemContentPrivate {
-std_unique_ptr~WlrBuffer~ pendingBuffer
-std_unique_ptr~WlrBuffer~ buffer
+void handleSurfaceCommit()
}
class WBufferItem {
+void releaseResources()
+QWindow window()
}
class DataManagerBase {
}
class DataManager {
+QPointer~DataManager~ manager
+QList~shared_ptr_Data~~ dataList
+QPointer~CleanJob~ cleanJob
+ResourceManager get()
}
class CleanJob {
+void run()
-QPointer~DataManager~ manager
}
class Data {
+int released
+DataType* data
}
class DataType {
}
WSurfaceItemContent --|> QQuickItem
WBufferItem --|> QQuickItem
WSurfaceItemContent ..> QQuickItemPrivate : uses
WBufferItem ..> QQuickItemPrivate : uses
DataManager --|> DataManagerBase
DataManager o-- CleanJob
DataManager o-- Data
Data --> DataType
WSurfaceItemContent *-- WSurfaceItemContentPrivate
WSurfaceItemContentPrivate o-- WlrBuffer
class WlrBuffer {
+void lock()
+void unlock()
}
File-Level Changes
Possibly linked issues
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
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `waylib/src/server/qtquick/private/wrenderbuffernode.cpp:234-236` </location>
<code_context>
+
+ // Destroy items after we're done accessing manager
+ // This prevents crashes if manager gets deleted during destruction
+ if (manager) {
+ for (auto item : std::as_const(itemsToDestroy)) {
+ manager->get()->destroy(item);
+ }
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against manager being deleted reentrantly during item destruction loop.
Deferring `destroy()` avoids reentrancy during the `dataList` iteration, but there’s still a lifetime hazard: `manager` is a `QPointer`, and `manager->get()->destroy(item)` may delete the manager (or its target) mid-loop. If that happens, `manager` becomes null, yet subsequent iterations still call `manager->get()` because the `if (manager)` is only checked once before the loop, leading to a crash.
To make this safe:
- Re-check `manager` on every iteration and break if it’s null:
```cpp
for (auto item : std::as_const(itemsToDestroy)) {
if (!manager)
break;
manager->get()->destroy(item);
}
```
- Or, if valid in this context, capture a raw `DataManager*` whose lifetime is guaranteed for the whole cleanup, and use that instead of the `QPointer` in the loop.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes three critical crash scenarios in Qt Quick Wayland rendering related to buffer management, resource cleanup, and scene graph node lifecycle:
- Fixes a use-after-free crash in DataManager's CleanJob by deferring item destruction until after manager data structures are no longer being accessed
- Resolves a buffer double-unlock assertion failure in wlroots by only calling
unique_ptr::reset()when the buffer pointer actually changes - Prevents crashes during window destruction by checking if the window is still valid before marking scene graph nodes as dirty
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| waylib/src/server/qtquick/wsurfaceitem.cpp | Adds buffer pointer comparison checks before reset operations to prevent double-unlock; adds window validity check in releaseResources() to prevent scene graph access during destruction |
| waylib/src/server/qtquick/wbufferitem.cpp | Adds window validity check in releaseResources() to prevent scene graph access during window destruction |
| waylib/src/server/qtquick/private/wrenderbuffernode.cpp | Refactors CleanJob to collect items for destruction in a separate list before destroying them, preventing use-after-free when manager gets deleted during item destruction |
This commit fixes three critical crash scenarios reported in issue linuxdeepin#659: 1. DataManager CleanJob use-after-free crash (linuxdeepin#659 comment 1) Problem: - In CleanJob::run(), calling manager->get()->destroy() could trigger shared_ptr<Data> destruction, which might delete the DataManager itself - Subsequent loop iterations would access the freed manager pointer Solution: - Collect items to destroy in a separate list first - Only destroy items after the loop completes and we're done accessing manager - Add null check before destruction to handle edge cases 2. Buffer double-unlock crash (linuxdeepin#659 comment 5) Problem: - When Wayland surface commits the same buffer repeatedly (pointer unchanged), the code would call pendingBuffer.reset(newBuffer) where newBuffer points to the same object as pendingBuffer.get() - std::unique_ptr::reset() ALWAYS calls the deleter (unlocker) even when old_ptr == new_ptr, causing: * Step 1: deleter(old_ptr) → wlr_buffer_unlock(A) → n_locks: 1 → 0 * Step 2: ptr = new_ptr (same object A) * Step 3: lock() → wlr_buffer_lock(A) → n_locks: 0 → 1 - The unlock in step 1 triggers assert(buffer->n_locks > 0) in wlroots Why not lock before reset? - Scenario 1 (same pointer): Creates temporary incorrect ref count * lock() → n_locks: 1 → 2 (but only 1 actual holder!) * reset() → unlock() → n_locks: 2 → 1 * Result: Temporarily inaccurate ref count, potential race conditions - Scenario 2 (different pointer): Locks wrong buffer! * pendingBuffer → A, newBuffer → B * lock() → A.n_locks: 1 → 2 (locking old buffer A!) * reset(B) → unlock(A) → A.n_locks: 2 → 1 * Result: B has n_locks=0 (not locked!), A has leaked reference Solution: - Check if pointer actually changed before calling reset() - Only perform reset + lock sequence when pointers differ - When pointers are same, skip the operation entirely 3. QSGNode markDirty crash during window destruction (linuxdeepin#659 comment 6) Problem: - releaseResources() is called during window destruction - Calling QQuickItemPrivate::dirty() tries to mark scene graph nodes dirty - But window and scene graph may already be destroyed at this point - Accessing destroyed nodes causes crash Solution: - Only call dirty() if window() returns non-null - This ensures scene graph is still valid before marking nodes dirty
zccrs
approved these changes
Dec 25, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Dami-star, 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.
This commit fixes three critical crash scenarios reported in issue #659:
DataManager CleanJob use-after-free crash ([Bug]: coredump--关闭窗口崩溃 #659 comment 1)
Problem:
Solution:
Buffer double-unlock crash ([Bug]: coredump--关闭窗口崩溃 #659 comment 5) Problem:
Why not lock before reset?
Scenario 1 (same pointer): Creates temporary incorrect ref count
Scenario 2 (different pointer): Locks wrong buffer! * pendingBuffer → A, newBuffer → B * lock() → A.n_locks: 1 → 2 (locking old buffer A!) * reset(B) → unlock(A) → A.n_locks: 2 → 1 * Result: B has n_locks=0 (not locked!), A has leaked reference
Solution:
QSGNode markDirty crash during window destruction ([Bug]: coredump--关闭窗口崩溃 #659 comment 6)
Problem:
Solution:
Summary by Sourcery
Fix multiple crash scenarios in Qt Quick Wayland rendering by hardening buffer management, deferred destruction, and scene graph updates during window teardown.
Bug Fixes: