lp1865957: Fetch multiple tracks from MusicBrainz #2534
Conversation
|
CI build failure is unrelated |
uklotzde
changed the title
|
If I don't close the music brains window, switch to the Mixxx window in full screen. There is no chance to show the music brains window again. I think it should be moved to the top if you select "fetch Metadata from Musikbrainz" again. |
|
Unfortunately it still segfaults, if you keep the window open: I need to check if QMetaObject::invokeMethod() for Qt < 5.10 works differently. Update: Invocation of method by name (old version) works as expected in Qt 5.13.2, no issues. |
I cannot confirm this. Switching into fullscreen mode works even while the window is open. The window handling has not been changed. |
I read about (and experienced myself) a few window management issues, like the mapping wizard window being hidden behind Mixxx during mapping (Ubuntu 18.04). If Uwe can't reproduce with fedora this seems to be OS specific. |
No, that's why I asked for detailed instructions. Maybe there is some subtle special case that I didn't account for? |
|
I can randomly click on the Previous and Next buttons to abort running requests and trigger new request in DlgTagFetcher. Not any issues independent of the frequency of abusing those buttons, everything works perfectly and stable. |
|
Can anyone else reproduce this crash??? The affected pointers are managed and checked by our |
|
@daschuer Are your sure that you tested a consistent build? |
There was a problem hiding this comment.
Code looks good, the branch builds fine and tests still pass. I cannot reproduce any crashes that @daschuer mentioned.
Maybe that window should be modal to avoid confusion when another track is selected and the user does not know which track the fetched metadata applies to, but that's not in scope of this PR.
LGTM, thank you!
|
@daschuer Ping |
|
@daschuer Last attempt to get feedback. I installed a VM with Ubuntu 18.04 LTS, compiled this branch and manually tested. Not a single crash. |
|
Sorry for the delay, I hope I will find time to look into this tonight. |
|
I was able to reproduce the crasher. It happens when hammering the next button. |
|
A fix is in progress |
|
I'm curious for the results and would like to know for which Qt version the crash occurs. No matter how fast or often I hammer the Previous/Next buttons, not a single crash yet. |
|
This fixes the issue: |
|
I will also do a review. I am afraid there are some issues left. |
|
I just tested and also spammed the previous/next button. All requests seems to work nicely, I didn't encounter any crashes or debug asserts. However, when I actually apply the metadata there seems to be a problem: After applying the new metadata it does show up in the library view. When I then select another track and then select the track I just applied the metadata to, the metadata gets reset to the state before the musicbrainz import (at least the title/artist fields in the library view). Might be unrelated to this branch and also be caused by the caching issue, I don't know. |
|
SIGSEGV in another context, not sure if related and how: This is the first time this ever happened. |
src/musicbrainz/tagfetcher.cpp
Outdated
| @@ -38,15 +38,15 @@ void TagFetcher::startFetch( | |||
|
|
|||
| void TagFetcher::abortAcoustIdTask() { | |||
| if (m_pAcoustIdTask) { | |||
| disconnect(m_pAcoustIdTask.get()); | |||
| disconnect(m_pAcoustIdTask); | |||
| m_pAcoustIdTask->deleteBeforeFinished(); | |||
There was a problem hiding this comment.
The crash happens here. The operator-> returns the plain pointer here in the GUI thread. If the thread is suspended and the delete from the worker thread happens in between, than the pointer becomes dangling an the crash occurs.
https://github.com/uklotzde/mixxx/pull/11 fixes the issue.
I strongly recommend to remove all auto deletion code as done in the PR.
If you need fire and forget nature in your other branches, inhered from the classes and add the desired nature.
It is kind of unpredictable and hard to review to have bot concepts mixed.
There was a problem hiding this comment.
The self-destruction code is now enclosed in VERFIY_AND_DEBUG_ASSERT to ensure that no unhandled signals slip through. It only serves as a fallback to prevent memory leaks.
I still don't understand how a crash could happen when using deleteLater(). This should ensure that objects remain alive until all signal handling has finished and control has been returned to the event loop.
There was a problem hiding this comment.
I hope I can explain:
DeleteLater sends a signal to the object that calls a slot that deletes the object from the thread the object lives in. This ensures that all signals that have been issued before and might be still in the queue are executed before deletion, which would lead to a crash in the best case.
Since Qt5 Qt uses it's QWeakPointer and QSharedPointer internally to maintain the object tree. This works with less locking than the Qt4 approach, that utilizes signals and slots.
A QPointer is essential a wrapper around Qt's weak pointer. It becomes a nullptr at any time once the last reference to the object was removed.
The code of question can be also written like this:
if (m_pAcoustIdTask) {
disconnect(m_pAcoustIdTask);
m_pAcoustIdTask->deleteBeforeFinished();
Can be also written inlined as:
if (m_pAcoustIdTask) {
disconnect(m_pAcoustIdTask);
mixxx::AcoustIdLookupTask pTask = m_pAcoustIdTask.data();
QMetaObject::invokeMethod([] {pTask->slotAbort(); })
The crash happens if the object is deleted from the other thread just between the last two lines. m_pAcoustIdTask.data() returns the still valid pointer 0x120697a0 but it becomes dangling before pTask->slotAbort() is called.
The DeleteLater trick dos not help here, because the object is already gone.
There was a problem hiding this comment.
All these signal and slots are hard to follow.
This is IMHO the major disadvantage of using a QT event loop for a worker thread.
Please ensure that every code path ends with either a success or a failure signal.
This also includes the early exists with VERIFY_OR_DEBUG_ASSERT.
uklotzde
changed the title
uklotzde
changed the title
|
Done. I have added lots of assertions to check for spurious, unexpected signals. This hopefully prevents future regressions if anyone tries to adjust the inner workings of TagFetcher. Experience has shown that this could be tricky including unforeseeable interactions. |
|
Great. The code looks good and more important it works really good. |
|
@daschuer Thank you for your review patience! I was struggling really hard to find a solution that is acceptable for both of us. Glad that it worked out in the end. |
https://bugs.launchpad.net/mixxx/+bug/1865957
Fixed: Fetching multiple tracks from MusicBrainz while leaving the dialog open.
The
WebTaskobject didn't support the use case to be restarted with a different URL.