(fix) avoid crash after removing Quick Link by ronso0 · Pull Request #14556 · mixxxdj/mixxx

ronso0 · 2025-03-27T02:13:54Z

workaround for #8270, happening with Qt 6.2.3

(... and other minor fixes for BrowseFeature, please see individual commits)

Apparently, the checks for item != nullptr and item->getData().isValid() don't suffice.
The crash happens afterwards with qDebug() << item->getLabel() << item->getData().toString(), so apparently the item has been removed in the meantime.

Simply don't call onLazyChildExpandation() after the context menu was closed and a Quick Link might have been removed.
Tbh I don't understand reasoning to expand the item after the menu closed in the first place. Regardless if the menu is used or closed right away, this is basically expand on right-click.

I removed those calls entirely, simply to avoid any further surprises.

Another bug I noticed while working on #14514 is that "Refresh directory tree" doesn't work for items other than first-level child items (and probably never did 😬 since #12908).
Fixed by using the last right-clicked index, too, as we do in other library features.

ronso0 · 2025-03-27T02:16:13Z

I can split the commit into the actual workaround and other tweaks if desired.

edit Done.

ronso0 · 2025-03-27T13:47:29Z

-    TreeItem *item = static_cast<TreeItem*>(index.internalPointer());
-    if (!(item && item->getData().isValid())) {
+    TreeItem* pItem = static_cast<TreeItem*>(index.internalPointer());
+    if (!(pItem && pItem->getData().isValid())) {


Interestingly, if I add various qDebug() calls (log item data and label etc.), the item pointer seems to remain intact and the existing qDebug() call below doesn't crash anymore 🤷

Would the delete management of std::shared_ptr/unique_ptr instead of plain * keep the item intact?

I have no clue what could be the reason. The internalPointer() is however a void* only without smart pointer capabilities. You get here only the address where the object has been when stored. There is no smart grantee, that the object TreeItem is still there. The pointer is only "borrowed", someone else need to take care that the object is kept valid.

Okay.
I removed the call as it was unnecessary in the first place, so I think this is ready.
We just need to make sure not to call it after/while modifying the tree.

…ick Link removal

… items

… have been removed

ronso0 · 2025-04-23T15:03:15Z

Can someone reprodue this bug or does it only happen with my outdated Qt 6.2.3?

If the former, shall I move the other (non-critical) fixes to another PR so this one has a better chance to get into 2.5.1?

daschuer · 2025-04-23T15:29:08Z

I have already a review in progress.

daschuer

For my understanding is that the root cause if the crash is the borrowed InternalPointer() in QModelIndex.

All later checks are pointless, because they succeed either (the pointer will never be null) or are also crashing because of call to the internal printer hidden behind the QT API.

The solution presented here to clear the stored value after using works only if it is actually used.
But there is no guarantee that such an action happens.

It would be better, if we effectively clear the pointer whenever the side bar tree structure is edited and there's a chance for a dangling pointer.

This seems to be more safe than all other not working checks.

I have not looked into a possible implementation but if you like, I can do a PR to your branch.

Use only m_lastRightClickIndex
Listen to a model changes we signal or such and reset the value
Temove misleading checks (make them asserts)

What do you think?

daschuer · 2025-04-23T05:46:15Z

@@ -77,6 +77,7 @@ class BrowseFeature : public LibraryFeature {
    // Caution: Make sure this is reset whenever the library tree is updated,
    // so that the internalPointer() does not become dangling
    TreeItem* m_pLastRightClickedItem;


This is a "borrowed" pointer that becomes dangling if the tree item is removed.

daschuer · 2025-04-23T05:50:39Z

    // Caution: Make sure this is reset whenever the library tree is updated,
    // so that the internalPointer() does not become dangling
    TreeItem* m_pLastRightClickedItem;
+    QModelIndex m_pLastRightClickedIndex;


This is the index, containing the borrowed pointer above as InternalPointer().

I think the name "InternalPointer is already a hint, that the it shall not be used externally.

But anyway with a bit of control we can.

ronso0 · 2025-04-23T16:33:40Z

Thanks for taking a look!

I have not looked into a possible implementation but if you like, I can do a PR to your branch.

Yes, please. I think I get the idea, but I'm not sure about the QModelIndex <--> TreeItem relation.

IIUC the issue would be fixed if we'd listen to the tree model's signals rowsAboutToBe[Removed|Inserted|Moved] and clear the lastRightClickedItem/Index. Only if it is affected? (it is not if rows are added below, but that's currently cumbersome to figure out (it's a bit easier with TreeItem::childLevel() from #14508 but still..)

daschuer · 2025-04-24T21:15:24Z

Here it is:
ronso0#92

…lickedPath()

This avoids using an no longer existing QMdodelndex() with a danging pointer.

daschuer

LGTM, Thank you.

ronso0 · 2025-09-03T22:24:43Z

+    connect(m_pSidebarModel,
+            &QAbstractItemModel::modelAboutToBeReset,
+            this,
+            &BrowseFeature::invalidateRightClickIndex);


This breaks slotRefreshDirectoryTree() where we call
onLazyChildExpandation(m_lastRightClickedIndex);
TreeItemModel::insertTreeItemRows() has now an invalid index (invalid parent TreeItem) and inserts the new rows at row 0.

Apparently it's not required to reset the QModelIndex in that case since onLazyChildExpandation(index) works flawlessly even tough this also alters the model (below the index).

Possible fixes:

revert this commit -- and test all cases that caused crashes earlier
(see 1.12 removing a quick link can lead to crash #8270 / crash when deleting directory from Computer -> Quick Links #12817)

in slotRefreshDirectoryTree(), copy m_lastRightClickedIndex and call onLazyChildExpandation(copy) -- works, no crash

@daschuer What do you think?

github-actions Bot added library browse labels Mar 27, 2025

ronso0 added crash library browse and removed library browse labels Mar 27, 2025

ronso0 added this to the 2.5.1 milestone Mar 27, 2025

ronso0 mentioned this pull request Mar 27, 2025

1.12 removing a quick link can lead to crash #8270

Open

ronso0 linked an issue Mar 27, 2025 that may be closed by this pull request

1.12 removing a quick link can lead to crash #8270

Open

ronso0 added 2 commits March 27, 2025 12:27

BrowseFeature: p prefix for TreeItem pointers

13d5bb1

BrowseFeature: reset m_pLastRightClickedItem after use

b874eaf

ronso0 force-pushed the quick-links-remove-crash-fix branch from 3ed110b to 2107c57 Compare March 27, 2025 11:37

ronso0 commented Mar 27, 2025

View reviewed changes

ronso0 force-pushed the quick-links-remove-crash-fix branch from 0feb1f4 to 0c42129 Compare April 2, 2025 05:26

BrowseFeature: more checks for valid data

b6f20ea

ronso0 force-pushed the quick-links-remove-crash-fix branch from 0c42129 to 2abde9b Compare April 2, 2025 08:57

ronso0 mentioned this pull request Apr 2, 2025

2.5.1 release #14501

Merged

ronso0 added 6 commits April 3, 2025 13:45

FolderTreeModel: use less costly entryList() for directoryHasChildren()

41d1fe1

BrowseFeature: rework context menu creation

d1d6403

(fix) BrowseFeature: avoid crash in onLazyChildExpandation() after Qu…

a05c1d6

…ick Link removal

(fix) BrowseFeature: make "Refresh directory tree" work for all child…

c6db71e

… items

(fix) BrowseFeature: don't restore default Quick Links when all links…

b53961d

… have been removed

(fix) BrowseFeature: don't cache hasChildren() for Quick Links node

848e0b8

ronso0 force-pushed the quick-links-remove-crash-fix branch from 2abde9b to 848e0b8 Compare April 3, 2025 11:45

ronso0 mentioned this pull request Apr 3, 2025

(feat) add Library sidebar bookmarks #14508

Open

9 tasks

daschuer requested changes Apr 23, 2025

View reviewed changes

daschuer added 4 commits April 25, 2025 20:41

Use p Prefix

f5e76db

Dispose redundant m_pLastRightClickedItem and introduce getLastRightC…

d001aec

…lickedPath()

Invalidate m_lastRightClickedIndex when the tree is about to be changed.

dfb2328

This avoids using an no longer existing QMdodelndex() with a danging pointer.

Refine the comments about the dangling pointer case

a21546f

ronso0 mentioned this pull request Apr 25, 2025

Quick links remove crash fix ronso0/mixxx#92

Closed

daschuer approved these changes Apr 25, 2025

View reviewed changes

daschuer merged commit bb2767a into mixxxdj:2.5 Apr 25, 2025
3 checks passed

ronso0 deleted the quick-links-remove-crash-fix branch April 26, 2025 00:45

ronso0 mentioned this pull request Sep 3, 2025

Rescan only a given subfolder or new files #13989

Open

ronso0 commented Sep 3, 2025

View reviewed changes

Uh oh!

Conversation

ronso0 commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronso0 commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronso0 Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

daschuer Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

ronso0 Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

ronso0 commented Apr 23, 2025

Uh oh!

daschuer commented Apr 23, 2025

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

daschuer Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

daschuer Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

ronso0 commented Apr 23, 2025

Uh oh!

daschuer commented Apr 24, 2025

Uh oh!

daschuer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ronso0 Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ronso0 commented Mar 27, 2025 •

edited

Loading

ronso0 commented Mar 27, 2025 •

edited

Loading

ronso0 Sep 3, 2025 •

edited

Loading