Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refectoring that aims to fix Rekordbox crash on macOS #10608 #11230

Merged
merged 27 commits into from
Feb 12, 2023
Merged

Refectoring that aims to fix Rekordbox crash on macOS #10608 #11230

merged 27 commits into from
Feb 12, 2023

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Jan 29, 2023 •
edited
Loading

We have a confirmed crasher on macOS regarding Reckordbox #10608.
Because it is not reproducible a did a tight review of the memory management in the surrounding code.

I have discovered a suspicious pointer handling that has been introduced because of the dynamic nature of the tree view in case of the Reckordbox feature. Unprotected raw pointers are carried by the QModelIndex and may become dangling after a tree rebuild. I have introduces smart pointers as best as possible without a complete rewrite and deleted the possible dangling pointers in 3260e9c and 83973e1

In addition I have cleand up the code around the search columns that has suffered some rotting.

Even though I have still no evidence that this PR fixes any crash there is a chance. If not this is IMHO a useful refactoring.
That's why I have target this to 2.3. If we think it is to big for a bug fix we can merge it to 2.4.

@daschuer
Make use of unique_ptr and std::move
009dba1 
This avoids memory leaks
@daschuer
Use std::move in BaseSqlTableModel::setTable
c6ea8ab
@daschuer
Inmprove initialization of library columns
b6c82af
@daschuer
Move BaseTrackCache::setSearchColumns to constructor, to create the o…
adcb323 
…bject at once.
@daschuer
Use QStringLiteral for Library columns
734d410
@daschuer
Use std::unique_ptr when creating the library tree
6cfdc9c
@daschuer
Reset m_pLastRightClickedItem before removing treeItems.
3260e9c 
This way it cannot become a dangling pointer
@daschuer
Clear LastRightClickIndex when removing tree items.
83973e1 
It can become a dangling pointer
@daschuer
Clean up search columns of the Serato and Rekordbox feature to match …
01d6667 
…the behavior of the native Mixxx library
@daschuer
remove unused m_searchColumnIndices
9257b80
@daschuer
Remove unused include
849215e
@daschuer
Browsetablemodel: Simplify adding search columns. Not yet enabled.
e79baef
@ronso0 ronso0 changed the title Refectoring that aims to fix crash #10608 Refectoring that aims to fix Rekordbox crash on macOS #10608 Jan 29, 2023
This was referenced Jan 30, 2023
Closed
Merged
@Swiftb0y
Copy link
Member

I took a quick look at this PR and I agree that this likely won't really fix any crash but it highlights the unsafe pointer handling by use of unsafe unique_ptr APIs. I will conduct a full review asap.

Swiftb0y
Swiftb0y requested changes Jan 30, 2023
View reviewed changes
src/library/treeitem.cpp Show resolved Hide resolved
src/library/treeitem.cpp Show resolved Hide resolved
src/library/banshee/bansheeplaylistmodel.cpp Outdated Show resolved Hide resolved
src/library/banshee/bansheeplaylistmodel.cpp Outdated Show resolved Hide resolved
src/library/basetrackcache.cpp Outdated Show resolved Hide resolved
src/library/baseplaylistfeature.cpp Show resolved Hide resolved
src/library/baseexternallibraryfeature.h Show resolved Hide resolved
src/library/rekordbox/rekordboxfeature.cpp Outdated Show resolved Hide resolved
src/library/serato/seratofeature.cpp Outdated Show resolved Hide resolved
src/library/treeitem.cpp Outdated Show resolved Hide resolved
daschuer and others added 2 commits January 31, 2023 00:12
@daschuer @Swiftb0y
Remove pointless comments
3fcfa16 
Co-authored-by: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com>
@daschuer
Make use of SharedPointer::create to avoid a literally new,
5805b17
@daschuer
Copy link
Member Author

I think I have addressed all comments. @Swiftb0y some of your ideas need to go to 2.4.
Do you have an opinion for the target branch of this PR? At least 3260e9c and 83973e1 should go to 2.3.

Swiftb0y
Swiftb0y requested changes Jan 31, 2023
View reviewed changes
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I'll still have to re-review a couple other parts though.

src/library/rekordbox/rekordboxfeature.cpp Outdated Show resolved Hide resolved
src/library/serato/seratofeature.cpp Outdated Show resolved Hide resolved
@Swiftb0y
Copy link
Member

Do you have an opinion for the target branch of this PR?

2.3 is fine if we leave comments in the code for the stuff that has to be done in 2.4.

@daschuer
Copy link
Member Author

daschuer commented Feb 1, 2023

I think I have addressed everything.

@Swiftb0y
Copy link
Member

Swiftb0y commented Feb 2, 2023

There are still a couple questions from me and ronso0 which have yet to be answered. I have double checked every comment and IMO everything that is not marked resolved yet is still an unfinished discussion.

@daschuer
Copy link
Member Author

daschuer commented Feb 2, 2023

I think I have added now all missing answers.

@Swiftb0y
Copy link
Member

Swiftb0y commented Feb 2, 2023

Thank you, I replied in their respective threads.

@daschuer
Copy link
Member Author

daschuer commented Feb 2, 2023

Done

@Swiftb0y
Copy link
Member

Swiftb0y commented Feb 2, 2023

Thank you, there is still another open comment where I requested to document the lifetime of a raw pointer.

@daschuer
Copy link
Member Author

daschuer commented Feb 3, 2023

Done

@Swiftb0y
Copy link
Member

Swiftb0y commented Feb 3, 2023

Thank you, can you take care of the failing pre-commit clang-format?

@daschuer
Copy link
Member Author

daschuer commented Feb 3, 2023

Ups, done.

Swiftb0y
Swiftb0y approved these changes Feb 12, 2023
View reviewed changes
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, sorry for not merging earlier. Lost track of this PR. Busy week.

@Swiftb0y Swiftb0y merged commit f3ee178 into mixxxdj:2.3 Feb 12, 2023
@uklotzde uklotzde mentioned this pull request Mar 31, 2023
Closed
@daschuer daschuer deleted the gh10608 branch May 4, 2023 11:09
@daschuer daschuer mentioned this pull request Jun 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Holzhaus Holzhaus Holzhaus left review comments

@ronso0 ronso0 ronso0 left review comments

@Swiftb0y Swiftb0y Swiftb0y approved these changes

Assignees
No one assigned
Labels
code quality library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daschuer @Swiftb0y @Holzhaus @ronso0