mixxxdj / mixxx

Mixxx is Free DJ software that gives you everything you need to perform live mixes.
http://mixxx.org
Other
4.53k stars 1.28k forks source link

MixxxApplication: Handle nullptr targets in notify #13903

Closed fwcd closed 2 days ago

fwcd commented 4 days ago

This handles null pTargets in MixxxApplication::notify's debug logging.

I ran into some hard-to-debug corner cases where the target pointer was null and the event was already optimized out, so I believe handling this case should at least provide valuable information about the event through the log. Maybe it would even make sense to log this case via qWarning() or qCritical().

ronso0 commented 4 days ago

@fwcd did this fix it for the cases you encountered? I also did some debugging and I couldn't avoid the crash with a target nullptr check, the crash happened with target->metaObject(). Dunno if it would be the same for pTarget->thread(). See #13885

ronso0 commented 4 days ago

I have concerns to keep the logging in the stable release for reasons I pointed out in https://github.com/mixxxdj/mixxx/pull/13889#issuecomment-2480628292. Unless of course someone can verify that QEvent::DeferredDelete, QEvent::ChildRemovedand nullptr target are the only cases we need to worry about.

fwcd commented 4 days ago

did this fix it for the cases you encountered?

Sadly I couldn't reproduce my bug after adding the logging anymore. Maybe some optimization or race condition that no longer happens when the program actually has to check that pointer. My hope was that the null check at least would help with future debugging.

By the way, is the object guaranteed to be alive before the notify? Maybe we could just stringify the target object beforehand or would that incur too much of a performance cost? If so we could gate that behind the developer flag too.

I'm a bit worried that we might not catch all cases here and get spurious crashes all over the place because we missed handling some event (we're already in undefined behavior territory if we can get a null or dangling pointer on some code paths)

fwcd commented 2 days ago

I think we should either have a debug assert or check for the null pointer in some other form to make sure we never dereference null. #13910 would work too.