fix Mac teardown crashes by correcting QObject ownership hierarchy - #526
Merged
Conversation
Reviewer's GuideAdjusts QObject ownership for group and timer views on macOS by reparenting models and delegates to their views, reinstating heap allocation for GroupModel, and updating usage sites to work with pointer-based models and Qt-managed lifetimes to avoid shutdown crashes. Sequence diagram for Qt teardown with corrected ownership hierarchysequenceDiagram
participant App
participant GroupWidget
participant QTableView as m_table
participant GroupModel as m_model
participant GroupProxyModel as m_proxyModel
participant GroupDelegate as m_delegate
App->>GroupWidget: destroy GroupWidget
activate GroupWidget
GroupWidget->>GroupWidget: ~GroupWidget()
GroupWidget->>QTimer: stop()
deactivate GroupWidget
note over GroupWidget,QTableView: Qt QObject destruction via parent-child
GroupWidget->>QTableView: ~QTableView() (child deleted by parent)
activate QTableView
QTableView->>GroupProxyModel: ~GroupProxyModel()
QTableView->>GroupModel: ~GroupModel()
QTableView->>GroupDelegate: ~GroupDelegate()
deactivate QTableView
note over QTableView,GroupModel: View and its models/delegate are destroyed together
Class diagram for updated Qt ownership in GroupWidgetclassDiagram
class GroupWidget {
+Mmapper2Group* m_group
+MapData* m_map
+GroupProxyModel* m_proxyModel
+GroupModel* m_model
+QTimer* m_pulseTimer
+QTableView* m_table
+void slot_onCharacterAdded(SharedGroupChar character)
+void slot_onCharacterRemoved(GroupId characterId)
+void slot_onCharacterUpdated(SharedGroupChar character)
+void slot_onGroupReset(GroupVector newCharacterList)
+void slot_mapLoaded()
+void slot_mapUnloaded()
-void updateColumnVisibility()
-void updatePulseTimer()
}
class GroupModel {
+void setCharacters(GroupVector characters)
+void insertCharacter(SharedGroupChar character)
+void removeCharacterById(GroupId characterId)
+void updateCharacter(SharedGroupChar character)
+GroupVector getCharacters()
+SharedGroupChar getCharacter(int row)
+void setMapLoaded(bool loaded)
}
class GroupProxyModel {
+void setSourceModel(GroupModel* model)
}
class GroupDelegate {
}
class QTableView {
+void setModel(GroupProxyModel* model)
+void setItemDelegate(GroupDelegate* delegate)
}
class QTimer {
}
GroupWidget *-- QTableView : owns m_table
GroupWidget *-- QTimer : owns m_pulseTimer
QTableView *-- GroupModel : parent of m_model
QTableView *-- GroupProxyModel : parent of m_proxyModel
QTableView *-- GroupDelegate : item delegate parent
GroupProxyModel o-- GroupModel : wraps
Class diagram for updated Qt ownership in TimerWidgetclassDiagram
class TimerWidget {
+CTimers& m_timers
+TimerModel* m_model
+QTableView* m_view
}
class TimerModel {
}
class TimerDelegate {
}
class QTableView {
+void setModel(TimerModel* model)
+void setItemDelegateForColumn(int column, TimerDelegate* delegate)
}
TimerWidget *-- QTableView : owns m_view
QTableView *-- TimerModel : parent of m_model
QTableView *-- TimerDelegate : shared delegate parent
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:
- All call sites now using
deref(m_model)assumem_modelis non-null; it would be safer either to assert/check for null insidederefor to guard the slots (e.g.slot_mapLoaded/Unloaded) in case they can fire before the view/model is fully initialized or after it has been torn down. - Since
GroupModelandGroupProxyModellifetimes are now owned bym_table, consider documenting or encapsulating that ownership (e.g. via a small helper or comment near their creation) to make it clear to future maintainers thatGroupWidgetmust not manually delete them and that the raw pointers are non-owning.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- All call sites now using `deref(m_model)` assume `m_model` is non-null; it would be safer either to assert/check for null inside `deref` or to guard the slots (e.g. `slot_mapLoaded/Unloaded`) in case they can fire before the view/model is fully initialized or after it has been torn down.
- Since `GroupModel` and `GroupProxyModel` lifetimes are now owned by `m_table`, consider documenting or encapsulating that ownership (e.g. via a small helper or comment near their creation) to make it clear to future maintainers that `GroupWidget` must not manually delete them and that the raw pointers are non-owning.
## Individual Comments
### Comment 1
<location path="src/group/groupwidget.h" line_range="154" />
<code_context>
MapData *m_map = nullptr;
GroupProxyModel *m_proxyModel = nullptr;
- GroupModel m_model;
+ GroupModel *m_model = nullptr;
QTimer *m_pulseTimer = nullptr;
</code_context>
<issue_to_address>
**suggestion:** Centralize access to m_model instead of using deref(m_model) in multiple places.
Since m_model is now a pointer, several `deref(m_model)` calls are scattered across slots and helpers. Please add a small private accessor like `GroupModel &model() { return deref(m_model); }` and use it instead. That centralizes the invariant/non-null assertion for m_model and improves readability.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| MapData *m_map = nullptr; | ||
| GroupProxyModel *m_proxyModel = nullptr; | ||
| GroupModel m_model; | ||
| GroupModel *m_model = nullptr; |
There was a problem hiding this comment.
suggestion: Centralize access to m_model instead of using deref(m_model) in multiple places.
Since m_model is now a pointer, several deref(m_model) calls are scattered across slots and helpers. Please add a small private accessor like GroupModel &model() { return deref(m_model); } and use it instead. That centralizes the invariant/non-null assertion for m_model and improves readability.
The application was crashing on macOS during shutdown due to improper parenting of UI components. When objects like delegates or models are parented to the main Widget instead of the View, they can be destroyed out of order, leading to use-after-free errors when the View attempts to access them during its own destruction. - Parent GroupModel and GroupProxyModel to m_table instead of 'this'. - Parent GroupDelegate and TimerDelegate to their respective views. - Ensure the lifecycle of the delegate matches the lifecycle of the view to prevent segmentation faults during signal disconnection. - Restore heap allocation for GroupModel to ensure Qt's ownership system manages its cleanup correctly.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #526 +/- ##
=======================================
Coverage 25.40% 25.40%
=======================================
Files 519 519
Lines 43109 43107 -2
Branches 4699 4699
=======================================
Hits 10952 10952
+ Misses 32157 32155 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
1 participant
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 application was crashing on macOS during shutdown due to improper parenting of UI components. When objects like delegates or models are parented to the main Widget instead of the View, they can be destroyed out of order, leading to use-after-free errors when the View attempts to access them during its own destruction.
Summary by Sourcery
Fix macOS shutdown crashes by aligning Qt object ownership with their views to ensure correct destruction order of models, views, and delegates.
Bug Fixes:
Enhancements: