Illegal glfwDestroyWindow in window close handler

[Radbuglet]

On MacOS, the following code will cause a segfault when the user quits the application using command+Q:

using Glfw;

// ...

public static void Main()
{
	using var window = new NativeWindow(1920, 1080, "My App");
	while (!window.IsClosed)
	{
		Glfw.PollEvents();
	}
	Console.WriteLine("Goodbye!");
}

The access violation happens after the NativeWindow.Closed event is finished dispatching, where the window close request callback returns control back to GLFW. Looking into the implementation of the protected NativeWindow.OnClosing handler, this seems to happen because underlying window is destroyed in the middle of the callback (see this line, which is responsible for destroying the window handle). This is illegal, with the GLFW reference documentation for glfwDestroyWindow stating that:

Reentrancy
This function must not be called from a callback.

This is a logical error, not an error in C#'s FFI. Here is the equivalent C code which exhibits the same faulty behavior:

#include <stdio.h>
#include <GLFW/glfw3.h>

static void onClosing(GLFWwindow* window)
{
	printf("Destroying %p\n", window);
	// ...
	glfwDestroyWindow(window);
}

int main(void)
{
	// Setup GLFW
	if (!glfwInit())
		return -1;

	// Setup window
	GLFWwindow* window = glfwCreateWindow(1920, 1080, "My App", NULL, NULL);

	if (!window)
		goto stop;

	glfwSetWindowCloseCallback(window, onClosing);

	// Main loop
	while (!glfwWindowShouldClose(window))
	{
		glfwSwapBuffers(window);
		glfwPollEvents();
	}

	// Cleanup
	puts("Goodbye!");
stop:
	glfwTerminate();
	return 0;
}

There's no easy way to fix this without changing the API. A non-breaking (but potentially confusing) fix could be to invisibly handle window destruction at the end of the Glfw.PollEvents method call by destroying all windows that still have the shouldClose flag, but that breaks the user expectation that the Glfw class is just raw FFI. A better (but breaking) approach would be to introduce a dedicated wrapper around Glfw.PollEvents that "handles higher-level behavior for the GLFW-NET-specific wrapper objects" i.e. implements the behavior of the previous proposal but in a different method.

[ForeverZer0]

Looking at this right now, perhaps I am missing something, but I do not even see why I chose to call base.Close() or what purpose it is even serving, other than propagate the call down to the parent class. The issue with that is I don't even believe the SafeHandle parent class even utilizes the method other than calling Dispose.

Shortly after the new year, I will have some proper time to invest back into this project. As you stated, I wish to keep priority on keeping the raw FFI class as close to being a 1:1 analog as I can. I already had some plans on redoing that portion to take advantage of unmanaged delegates over the DllImport paradigm, which would also allow for me to create my own loader and get rid of the whole library renaming debacle (hindsight 20/20).

I will definitely take a look at this oversight of mine at that time. I have a vague memory of something like this occurring at some point when testing, but I was unable to reproduce it or figure out what the issue was at the time, so it got forgotten about.

[Radbuglet]

but I do not even see why I chose to call base.Close() or what purpose it is even serving, other than propagate the call down to the parent class

base.Close() calls SafeHandle.Close, which is an alias to SafeHandle.Dispose, which—after some use count shenanigans to make multi-threaded finalization with PInvoke safe—calls NativeWindow.ReleaseHandle and destroys the window.

So the base.Close() call was presumably to destroy the window, which is necessary because the window won't be properly closed in multi-window applications otherwise. Most people either don't write multi-window applications or handle window destruction after glfwPollEvents so they don't typically run into this issue.

---

Unrelated:

I already had some plans on redoing that portion to take advantage of unmanaged delegates over the DllImport paradigm, which would also allow for me to create my own loader and get rid of the whole library renaming debacle (hindsight 20/20).

Yeah, from what I can tell, the compiler doesn't even choose the proper library on OSX and defaults to "glfw", which only matches libglfw without the .dylib extension. Not sure why DllImport properly prefixes the file with lib but then omits the extension but O.K.