-
Notifications
You must be signed in to change notification settings - Fork 625
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[CameraServer] destroy VideoListener first #7686
base: main
Are you sure you want to change the base?
Conversation
Have you been able to confirm that this fixes the issue? |
I haven't yet. @Sam948-byte had that in work |
I wasn't able to get to that last night. I think the earliest I can test is probably tomorrow night. |
A test that reproduces the issue with ASan + TSan would be good. |
I'm not sure what Photon does that wpilib's ci doesn't that would trigger this bug in the first place. Allwpilib already has tests run under sanitizers. |
I don't see any tests for CameraServer 😅 |
i suppose that would do it, eh? lol. we should upstream some tests, then |
d5c6140
to
1846af7
Compare
I'm sometimes seeing new one this locally, and even with this patch, I'm still seeing use-after-frees. Could we be removing the listener after the callback has been invoked by the callback thread? I don't see any synchronization happen in CallbackManager::Remove at all.
|
[potentially] fixes #7685