fix: synchronize close state in glfw window (#3874)

[partyzanex]

Description:

The goroutine and main thread both touched same fields (closing, viewport) without any lock, and viewport.Destroy() left pointer in place ? so next frame called methods on an already destroyed window.

What changed:

• Added a sync.RWMutex to the window that guards closing and viewport.
• isClosing(), view() and Close() now read and write those fields under the lock.
• RunWithContext takes a single snapshot of viewport and skips frame if the window is gone.
• A new helper destroyViewport() clears pointer before destroying window; close path uses it.

Tests added in internal/driver/glfw/window_test.go:

• One test runs Close() while another goroutine reads window state. Without fix it fails under -race.

Fixes #3874

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[andydotxyz]

Can you confirm which goroutine you are referring to? All Fyne calls should operate on the same thread...

[partyzanex]

Can you confirm which goroutine you are referring to? All Fyne calls should operate on the same thread...

Even though Fyne pins UI work to a single thread, application code still routinely can calls Window methods from background goroutines: timers, network handlers, channel consumers, worker pools, and so on. Close() already acknowledges this contract by self-dispatching through async.EnsureMain, because callers can invoke it from arbitrary goroutines.

Several other Window methods, however, read window state before dispatching to the main thread. That is where the race occurs.

Close() already establishes the contract: it self-dispatches through async.EnsureMain, so callers are implicitly allowed to invoke it from arbitrary goroutines. Several other Window methods (SetFullScreen, RequestFocus, RequestPosition, RequestFullScreenSecondary) still read w.viewport and w.closing directly on the caller goroutine before dispatching to the main thread.

That creates a race where:

• goroutine A calls Close()
• goroutine B calls SetFullScreen()
• both concurrently access w.viewport / w.closing

The -race test reproduces this scenario directly.

The second test exposes a separate lifetime issue unrelated to threading. destroyWindow() calls viewport.Destroy() but does not clear w.viewport, so a later view() can still return a destroyed handle. That matches the glfwMakeContextCurrent(0x0) path from issue #3874.

destroyViewport() fixes this because it performs both destruction and w.viewport = nil under closeLock, ensuring the window state stays consistent during teardown.

[redawl]

Can you confirm which goroutine you are referring to? All Fyne calls should operate on the same thread...

Even though Fyne pins UI work to a single thread, application code still routinely can calls Window methods from background goroutines: timers, network handlers, channel consumers, worker pools, and so on. Close() already acknowledges this contract by self-dispatching through async.EnsureMain, because callers can invoke it from arbitrary goroutines.

Several other Window methods, however, read window state before dispatching to the main thread. That is where the race occurs.

Close() already establishes the contract: it self-dispatches through async.EnsureMain, so callers are implicitly allowed to invoke it from arbitrary goroutines. Several other Window methods (SetFullScreen, RequestFocus, RequestPosition, RequestFullScreenSecondary) still read w.viewport and w.closing directly on the caller goroutine before dispatching to the main thread.

That creates a race where:

* goroutine A calls `Close()`

* goroutine B calls `SetFullScreen()`

* both concurrently access `w.viewport` / `w.closing`

The -race test reproduces this scenario directly.

The second test exposes a separate lifetime issue unrelated to threading. destroyWindow() calls viewport.Destroy() but does not clear w.viewport, so a later view() can still return a destroyed handle. That matches the glfwMakeContextCurrent(0x0) path from issue #3874.

destroyViewport() fixes this because it performs both destruction and w.viewport = nil under closeLock, ensuring the window state stays consistent during teardown.

Application code should be wrapped in fyne.Do or fyne.DoAndWait if they are calling fyne apis from goroutines. async.EnsureMain was not added to all methods because the performance penalty of the thread check was deamed too great.

[andydotxyz]

Close() already establishes the contract: it self-dispatches through async.EnsureMain, so callers are implicitly allowed to invoke it from arbitrary goroutines.

Currently, but it will not be supported forever. The locks in place currently are for the transition period only. Apps using 2.8 (after release) that have not migrated will see the following message:

*** This application has not been migrated to the fyne.Do threading model ***
*** The next major Fyne release will remove this safety! ***
*** Read more at https://docs.fyne.io/started/goroutines ***

We started this migration in 2.6 and has been mentioned in the release notes.
If you are interested in more details check out https://github.com/fyne-io/fyne/wiki/Threading-migration.

[coveralls]

coverage: 60.216% (+0.02%) from 60.194% — partyzanex:bugfix/3874-runwithcontext-close-race into fyne-io:develop

[partyzanex]

Thanks, that made me realize I misunderstood fyne's threading and window-lifecycle guarantees. The mutex was based on a wrong assumption, so I've removed it.

I'd still like to keep the stale-pointer fix: after viewport.Destroy(), w.viewport still points to a destroyed window. Nilling it before destroying ensures view() returns nil instead of handing out a freed pointer — which is what caused the crash in #3874.

[andydotxyz]

Thanks, I agree it makes sense to keep in this safety.

But can you reduce the comments to the essentials? Otherwise they rot and/or get misleading.

[andydotxyz]

Can you make this more succinct?
It reads AI generated because it refers to behaviour in other methods as well.
The docs on a method should describe what it does only - not how others respond to it.
Also no need to mention it has the same threading semantics as the rest of the code in the same file.