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.

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

MixxxApplication: Only log opaque pointer after notify #13910

Open
wants to merge 1 commit into
base: 2.5
Choose a base branch
from

Conversation

fwcd
Copy link
Member

@fwcd fwcd commented Nov 19, 2024

As an addendum to #13900, QEvent::Timer seems to be another event type where a dangling pointer could come up (the screenshot includes some more logging which I've removed again):

Screenshot 2024-11-19 at 18 46 45

Since the target pointer could be dangling for any event point, this PR takes the safer road by only logging the opaque pointer after the notify call.

@JoergAtGithub
Copy link
Member

JoergAtGithub commented Nov 19, 2024

I think that this could happen with any event type - but QEvent::DeferredDelete has the speciality, that it always happen (if we hit the time limit).
As I expressed already in #13889 (comment) I see only two proper fixes:

  1. Cache the object informantion before calling notify, which will affect the general performance in developer mode
  2. Remove the printout of the target object information and just print the information about the event type

@fwcd
Copy link
Member Author

fwcd commented Nov 19, 2024

IMO not printing the target (or only the hex pointer) would be the safe option for now if we don't know whether the pointer might be dangling. Maybe we could add another verbosity or log level for caching the object info and logging it?

@fwcd fwcd changed the title MixxxApplication: Log pointer only on timer events MixxxApplication: Only log opaque pointer after notify Nov 19, 2024
@JoergAtGithub
Copy link
Member

@ronso0 has already opened a related PR #13909, which removes the only known case to use --developer for non-development.

@fwcd
Copy link
Member Author

fwcd commented Nov 19, 2024

Yeah, I believe we should still fix this though, since non-deterministic crashes shouldn't happen, even in developer mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants