Race condition in renderer cache can lead to widget being rendered with destroyed renderer

[dweymouth]

Checklist

• I have searched the issue tracker for open issues that relate to the same problem, before opening a new one.
• This issue only relates to a single bug. I will open new issues for any other problems.

Describe the bug

Thanks to @knusbaum for discovering this bug

The implementation of the widget renderer cache cleaning task acquires a lock on the renderer cache map, identifies the expired renderers and calls Destroy() on each of them, and then releases the lock before it grabs it again to actually remove the expired renderers from the map. This leads to a race condition where the renderer for a widget could be looked up after Destroy has been called but before it was removed from the map.

A possible fix would be to make the clean task atomic, so it holds a lock for the entirety of the clean task, but we need to make sure this can't introduce deadlocks.

How to reproduce

it's a race condition, hard to reproduce, but it is clear from looking at the code

Screenshots

No response

Example code

see implementation linked in the bug description

Fyne version

develop

Go compiler version

n/a

Operating system and version

n/a

Additional Information

No response

[knusbaum]

Having looked at the code, I think the easiest fix is to move the Destroy call out of the RLock()'d loop and into the Lock()'d loop where we actually delete the objects, after the delete() call.

There may be other places where it's necessary to fix this, since IIRC this isn't the only place deleting items from the cache.
Ideally this would be put into one function so there aren't multiple places deleting things that need to be kept in sync.

[dweymouth]

I actually don't think that fixes it, since if I understand the code correctly, in between the RLocked loop and the Locked loop, a renderer that was marked for deletion could then be looked up and returned, marked alive, and then deleted in the second loop. I think we either need a flag on the renderer to track if a renderer is "marked" for deletion, or maybe a larger-scale reorganization of the entire render cache setup

[pierrec]

It looks like this is not the only function affected by this kind of race:

• destroyExpiredCanvases
• destroyExpiredFontMetrics
• destroyExpiredSvgs

[andydotxyz]

Should this be marked as resolved with the fix landed? I lost track sorry.

[pierrec]

Should this be marked as resolved with the fix landed? I lost track sorry.

The intent of the PR is to fix this issue, but it is unsure if there is a reproducer.
@knusbaum maybe you have one?

[dweymouth]

I think this issue is closed by the PR, but there is a related issue #5131 which is still unresolved. (Actually 5131 is an issue that is not happening right now only because of issue #4903)