#1644 Crash in notation editor when undoing create segment, incl. 22.06
Seems to have been introduced somewhere between [8c1c6b] (OK) and [6a0662] (crash). Yes, many (22) commits between the two but those are the closest saved builds I have.
Steps to reproduce (bug is somewhat sensitive to exact details):
1. New document
2. Create two 4-bar segments, each from measure 1 through measure 4, one on track 1 and one on track 2, in that order.
3. Create one 2-bar segment from measure 1 through measure 2, on track 3.
4. Ctrl+A to select all segments.
5. "N" to bring up notation editor.
6. Ctrl+Z to undo -- some builds crash here.
7. Ctrl+Z to undo second time -- crash here if not on first undo.
Typical stack trace:
(gdb) where 12
#0 0x000000000082ccd0 in Rosegarden::Segment::getStartTime() const ()
#1 0x00000000009d6d51 in Rosegarden::NotationHLayout::getXForTimeByEvent(long) const ()
#2 0x000000000064fd94 in Rosegarden::NotationScene::getCursorCoordinates(long) const ()
#3 0x000000000061d5a0 in Rosegarden::NotationWidget::slotPointerPositionChanged(long) ()
#4 0x00007ffff57b1d1f in QMetaObject::activate(QObject*, int, int, void**) ()
at /usr/lib64/libQt5Core.so.5
#5 0x00000000004a59d2 in Rosegarden::RosegardenDocument::pointerPositionChanged(long) ()
#6 0x00007ffff57b1d1f in QMetaObject::activate(QObject*, int, int, void**) ()
at /usr/lib64/libQt5Core.so.5
#7 0x00000000004ec1c2 in Rosegarden::CommandHistory::undo() ()
#8 0x00007ffff57b1d1f in QMetaObject::activate(QObject*, int, int, void**) ()
at /usr/lib64/libQt5Core.so.5
#9 0x00007ffff6aedcc2 in QAction::triggered(bool) ()
at /usr/lib64/libQt5Widgets.so.5
#10 0x00007ffff6af03ac in QAction::activate(QAction::ActionEvent) ()
at /usr/lib64/libQt5Widgets.so.5
#11 0x00007ffff6af0c75 in QAction::event(QEvent*) ()
at /usr/lib64/libQt5Widgets.so.5
(More stack frames follow...)
Crash still happens if undo is from "Edit -> Undo create segment" menu instead of Ctrl+Z shortcut. No crash if done in matrix editor or directly in main window / track editor.
I can't reproduce this.
We had something similar which was (hopefully) fixed back in April - commit [cf0a28] (cf0a28d76b91266cd07b429627c987f1ea35e7b2).
Related
Commit: [cf0a28]
Last edit: Ted Felix 2022-07-10
Yes, this is one is very hard to track down.
After looking at the code and comments in [cf0a28] it seemed possible that a double free was happening, so I added debug output (in my current working tree). While the crash is 100% reproducible on my system (in commits ranging across my own feature branch, master 22.06, and others), with debug sometimes it crashes, although not with any double free that I can find, and sometimes not. I haven't looked into the subject for many years, but always assumed that with the same code/build/usage
malloc, etc., were deterministic and would produce the same results. That's not the case here, shown both because crash-vs-no-crash and because the debug shows different addresses, even between two non-crash runs.Here's a run that didn't crash.:
Here's another run that crashes with the same code, same debug, same "steps to reproduce". It's suspicious that
segmentRemoved()is deleting the 2nd, not the 3d/last, segment created bysetStaffs()as was the case in the successful run.Just for fun, I removed the
deleteanderaseinsegmentRemoved(). After that I couldn't get it to crash, and it doesn't seem to leak memory:In the unlikely chance that I have spare time to look further I might try
valgrindbecause something's getting corrupted, somewhere. In addition, I'm not happy about the queued connection timing issues mentioned in the code comment, and the fundamental design where one scene is destructed and a new one constructed merely because a staff is being deleted. But there's a lot of that kind of thing in Rosegarden, and that I don't like it is my problem, not yours.Basically the bug doesn't affect me, and if it's not reproducible by others I guess it can be ignored for now.
Related
Commit: [cf0a28]
I did a valgrind run of this. Always difficult analyzing valgrind output !!!
I get some messages about the m_preview in NotationStaff.cpp.
Can you try to see if the preview note has something to do with the crash:
1. Make sure a preview note is shown - keep the mouse in the notation editor with the note insert tool active. Try to reproduce the crash.
2. Make sure no preview is shown - switch to select tool before trying to reproduce the crash.
Thanks
I don't see much difference between being in "Draw Notes and Rests(F3)" vs "Select and Edit(F2)" mode when the undo of the segments (Ctrl+Z) is done. I do see a difference -- I don't think it ever crashes, although I'm not 100% sure -- if the mouse is not in the staff view area during undo.
I added more debug and don't see any double frees of
NotationStaff::m_previewItem, although they're definitely being leaked:This is when moving the mouse around and the preview note is being created and moved (new ones being made, not the original one being moved). Note that these aren't "permanent" leaks because when the notation editor is closed the
QMainWindowwill clean up anyQGraphicsItems it still contains. But once again this is the kind of thing in Rosegarden I complain about -- it would be much cleaner, more efficient, safer, etc. if the preview note was created once andsetPos()used to move it as required.I did a quick valgrind run and didn't see anything obvious. I'm attaching a valgrind suppression file that I wrote for Rosegarden a few months ago. It's a total hack -- I just kept adding suppressions for excess valgrind noise in that long-ago commit until only what I was looking for remained. It almost certainly suppresses too much and could miss what's really happening here, but it might be of some use if you want to try it and modify it as needed.
I don't think there is a leak. m_previewItem is deleted before the call to makeNote.
Initially or after a clear this will delete a null pointer. This worried me but mister google assures me that this is OK. It seems to be good style NOT to do a null check before deleting.
Yes, you're correct. My simplistic check of "news" vs "deletes" wasn't taking into account the delete before makeNote(). As I said, it doesn't matter anyway due to the fact that Qt cleans up orphaned QGraphicsItems. And also yes, doing a delete of a null pointer has been safe in C++, either since the beginning or at least for a very long time.
Confirmed. Did the procedure once, didn't crash. Continued with the procedure a second time, crashed on the second Ctrl+Z. I'm at [1979b2].
Related
Commit: [1979b2]
Diff:
Related
Commit: [6a0662]
Commit: [8c1c6b]
Think I have a fix for this. Try the upstream/bug1644 branch. Specifically [e67b14].
https://sourceforge.net/p/rosegarden/git/ci/bug1644/tree/
It appears as if NotationHLayout doesn't get the memo that the segment was deleted. I added a call to redo the layouts at the end of
NotationScene::segmentRemoved(). Seems to work for me.Please review and test and let me know if this looks like the right solution.
Related
Commit: [e67b14]
Looks good to me. Using the updated procedure (same as original but make sure mouse is not in NotationView area), get repeatable crash with master/[1979b2], none with bug1644/[e67b14].
Not that it matters, but I'm speculating that this wasn't reproducible for Philip because he's on a Qt6 (??) vs my (and Ted's?) Qt5. And that the bug was real but an edge case, and 6 is either insensitive to it or specifically checks for and avoids it.
Related
Commit: [1979b2]
Commit: [e67b14]
Actually I was working with Qt 5.12.7. I also have a Qt 5.9.7 - still no crash !!
Maybe the compiler version ?
gcc 7.5.0
Very strange !!
This is a case of reading from freed memory. Anything can happen. Most likely it is the memory manager that is making the problem more or less apparent. valgrind should have found this. It reports them as "Invalid read of size x". They are really hard to find in all the valgrind output, though.
I'm going to do a little regression testing of the horizontal layout to make sure everything looks ok before pushing this to master.
I was getting "Invalid read of size x" messages - from the clearPreview call !!
It seems clearPreview was being called on a deleted Staff.
I have made a merge request to fix this
Pushed all changes as [bb6e7d]. Please test latest git.
Related
Commit: [bb6e7d]