RenderingDevice: Add null checks when retrieving uniform sets#114073
Conversation
This may mitigate a crash seen in the wild in Rift Riff on Android, most likely trading it for a single-frame rendering bug (which is better than crashing on user devices). It doesn't solve the underlying issue which seems to be a race condition where a uniform set RID gets has been freed while still being reported as owned by the RID_Owner.
78f2c4e to
adb7774
Compare
| const String us_info = us ? vformat("(%d):\n%s\n", i, _shader_uniform_debug(us->shader_id, us->shader_set)) : vformat("(%d, which was just freed) ", i); | ||
| ERR_FAIL_MSG(vformat("Uniforms supplied for set %sare not the same format as required by the pipeline shader. Pipeline shader requires the following bindings:\n%s", us_info, _shader_uniform_debug(draw_list.state.pipeline_shader))); |
There was a problem hiding this comment.
This looks a bit convoluted with the nested vformats, but it's basically switching between these two messages depending on whether us is null:
Uniforms supplied for set (<i>, which was just freed) are not the same format as required by the pipeline shader. Pipeline shader requires the following bindings:
<pipeline shader debug info>
(same as the else branch)
and:
Uniforms supplied for set (<i>):
<uniform set debug info>
are not the same format as required by the pipeline shader. Pipeline shader requires the following bindings:
<pipeline shader debug info>
clayjohn
left a comment
There was a problem hiding this comment.
Looks good to me. I tested this with a custom build that deletes one uniform set one time right after calling draw_list_bind_uniform_set. The result was an error message and the sprite failed to draw, but that was it, Godot continued to run after that and did not crash.
So at the very least this will make Godot more resilient and less likely to crash when a uniform set is freed after being bound, but before the draw call is issued.
Now we just need to figure out how the uniform set is getting freed in these cases. Since it looks like some sort of race condition despite all the relevant code being single threaded
|
Thanks! |
|
Cherry-picked for 4.5.2. |
This may mitigate a crash seen in the wild in Rift Riff on Android, most likely trading it for a single-frame rendering bug (which is better than crashing on user devices).
It doesn't solve the underlying issue which seems to be a race condition where a uniform set RID gets has been freed while still being reported as owned by the RID_Owner.
Stack trace from Rift Riff:
This was partially mentioned in #112157 but it seems conflated with other rendering related crashes which may not have the same cause there, so it's not fixing that issue per se.
Some more context on devices where this issue was reproduced in Rift Riff, mainly Mediatek, Google Tensor, Spreadtrum and Samsung s5e8835 SoCs.
Details