PopupWindow : Possible fix for obscure segfault#6905
Merged
murraystevenson merged 1 commit intoGafferHQ:1.6_maintenancefrom Apr 24, 2026
Merged
PopupWindow : Possible fix for obscure segfault#6905murraystevenson merged 1 commit intoGafferHQ:1.6_maintenancefrom
murraystevenson merged 1 commit intoGafferHQ:1.6_maintenancefrom
Conversation
danieldresser-ie
force-pushed
the
maybeUiCrashFix
branch
from
ff26cb1 to
45d9283
Compare
As the new unit test demonstrates, this can lead to PySide crashes if the widget is later garbage collected on a background thread. We believe this test models crashes that we have seen both at Image Engine and Cinesite. In Image Engine's case the popup was commonly in a Spreadsheet, and in Cinesite's case the common trigger seemed to have been a custom render pass name widget that the RenderPassEditor pops up. The circular reference was caused by a13abf4, where I claimed to be "using a regular function rather than a method". But what I failed to understand was that `__get__` was actually creating a bound method, and hence a circular reference. The solution is to revert to the original code, using WeakMethod to avoid the circular reference. This has the problem that motivated the change in the first place though - warnings about an expired WeakMethod when closing a popup editor in VectorDataWidget. This time we fix them by passing `fallbackResult = None` to WeakMethod, so it doesn't complain about expiration.
johnhaddon
force-pushed
the
maybeUiCrashFix
branch
from
45d9283 to
4bac9ef
Compare
Member
|
@danieldresser-ie, @ivanimanishi, @mortimerb thank you so much for tracking this one down. I have been hunting it for months at Cinesite but never got a reliable repro. I'm pretty sure it's the same thing though - same stack trace, and we recently started to suspect that a PopupWindow was involved. @danieldresser-ie, I think your fix is totally legit, but I've pushed an alternative version that does a fuller revert of a13abf4, includes a unit test that proves the crash is fixed, and has more discussion in the commit message. If that looks OK to you then let's get this into a release today. |
Contributor
Author
|
LGTM |
github-project-automation
Bot
moved this from Pending Review
to Pending release
in Work in Progress
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ImageEngine has found a weird crash in a specific UI, which has been fairly frequently causing problems. It involves using the Spreadsheet editor to create a popup window containing a VectorDataWidget, closing that, and then pressing a button that reloads the contents of a complicated dynamic box, triggering UI refreshes that take quite a while to complete.
I've emailed John some crash logs to look at.
Martin and Ivan were able to track this down to having started happening in 1.6.4, and more specifically, this commit:
a13abf4
Looking at that without much context, and not being that familiar with this use of
__get__, it looks to me like it is creating a circular reference to _qtWidget() that would result in being kept alive until garbage collection. I'm not sure whether that should be a problem, but it seems plausible that it could be at least the trigger for this crash, even if it isn't wrong.The result of a
__get__seems a lot like a method, so I suggested wrapping it in a WeakMethod to see what happens. In Ivan's testing, that seems to have stopped him seeing the crash. I have no idea if this is actually correct, or if it could cause other problems, but I figured I'd PR it in case it seems reasonable. If we can find a fix for this issue, it would be great to release it quickly.If we're completely barking up the wrong tree here, and John doesn't spot anything likely in the crash logs, then we might need to try and reproduce this outside the IE env, but that could be quite tricky, given that it seems to be dependent on the specific timing of loading a complex setup using a custom UI.