Skip to content

Commit

Permalink
Guard disposal of WindowClass
Browse files Browse the repository at this point in the history
Saw it fail once in tests with ERROR_CLASS_HAS_WINDOWS (1412): Class still has open windows.

- Ensure it doesn't throw on Finalizer thread.
- Ensure it doesn't double dispose
- Ensure we keep `this` alive in Window and WindowClass while responding to messages
  • Loading branch information
JeremyKuhne committed Jan 22, 2024
1 parent 6d94953 commit 3d137c0
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 26 deletions.
8 changes: 6 additions & 2 deletions src/thirtytwo/Window.cs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ private LRESULT WindowProcedureInternal(HWND window, uint message, WPARAM wParam
{
foreach (var handler in handlers.GetInvocationList().OfType<WindowsMessageEvent>())
{
var result = handler(this, window, (MessageType)message, wParam, lParam);
LRESULT? result = handler(this, window, (MessageType)message, wParam, lParam);
if (result.HasValue)
{
return result.Value;
Expand All @@ -157,7 +157,11 @@ private LRESULT WindowProcedureInternal(HWND window, uint message, WPARAM wParam
}
}

return WindowProcedure(window, (MessageType)message, wParam, lParam);
LRESULT windProcResult = WindowProcedure(window, (MessageType)message, wParam, lParam);

// Ensure we're not collected while we're processing a message.
GC.KeepAlive(this);
return windProcResult;
}

protected virtual LRESULT WindowProcedure(HWND window, MessageType message, WPARAM wParam, LPARAM lParam)
Expand Down
63 changes: 39 additions & 24 deletions src/thirtytwo/WindowClass.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@ public unsafe partial class WindowClass : IDisposable
private readonly WindowProcedure _windowProcedure;
private readonly string _className;
private readonly WindowClassInfo? _windowClass;
private bool _disposedValue;
private int _disposedValue;
private readonly object _lock = new();

public ATOM Atom { get; private set; }
public HMODULE ModuleInstance { get; }
private bool Disposed => _disposedValue != 0;

public unsafe WindowClass(
string? className = default,
Expand Down Expand Up @@ -105,10 +106,7 @@ public WindowClass(string registeredClassName)
/// </summary>
public unsafe WindowClass Register()
{
if (_disposedValue)
{
throw new ObjectDisposedException(GetType().Name);
}
ObjectDisposedException.ThrowIf(Disposed, this);

if (_windowClass is not null && !IsRegistered)
{
Expand Down Expand Up @@ -143,10 +141,7 @@ public virtual HWND CreateWindow(
nint parameters = default,
HMENU menuHandle = default)
{
if (_disposedValue)
{
throw new ObjectDisposedException(GetType().Name);
}
ObjectDisposedException.ThrowIf(Disposed, this);

if (!IsRegistered)
throw new InvalidOperationException("Window class must be registered before using.");
Expand Down Expand Up @@ -191,40 +186,60 @@ public virtual HWND CreateWindow(
}

private LRESULT WindowProcedureInternal(HWND window, uint message, WPARAM wParam, LPARAM lParam)
=> WindowProcedure(window, (MessageType)message, wParam, lParam);

protected virtual LRESULT WindowProcedure(HWND window, MessageType message, WPARAM wParam, LPARAM lParam)
{
return Interop.DefWindowProc(window, (uint)message, wParam, lParam);
if (Disposed)
{
// In the middle of disposing, we've flipped the flag, but haven't unregistered the class yet.
return Interop.DefWindowProc(window, message, wParam, lParam);
}

LRESULT result = WindowProcedure(window, (MessageType)message, wParam, lParam);

// We don't want to be finalized while we're responding to a message.
GC.KeepAlive(this);
return result;
}

protected virtual LRESULT WindowProcedure(HWND window, MessageType message, WPARAM wParam, LPARAM lParam) =>
Interop.DefWindowProc(window, (uint)message, wParam, lParam);

protected virtual void Dispose(bool disposing)
{
}

private void InternalDispose(bool disposing)
{
if (!_disposedValue)
if (Interlocked.Exchange(ref _disposedValue, 1) == 1)
{
_disposedValue = true;
Debug.Fail("Double disposing");
return;
}

if (Atom.IsValid)
if (Atom.IsValid)
{
// Free the memory for the window class and prevent further callbacks.
// (Presuming that we don't have to set the default WNDPROC back via SetClassLong, if we do
// we can follow along with what Window does.)
if (Interop.UnregisterClass((char*)Atom.Value, ModuleInstance))
{
// Free the memory for the window class and prevent further callbacks.
// (Presuming that we don't have to set the default WNDPROC back via SetClassLong, if we do
// we can follow along with what Window does.)
if (Interop.UnregisterClass((char*)Atom.Value, ModuleInstance))
Atom = default;
}
else
{
WIN32_ERROR error = Error.GetLastError();
if (disposing)
{
Atom = default;
error.Throw();
}
else
{
Error.ThrowLastError();
// Don't want to throw on the finalizer thread.
Debug.Fail($"Failed to unregister window class: {error}");
}
}

Dispose(disposing);
}

Dispose(disposing);
}

~WindowClass() => InternalDispose(disposing: false);
Expand Down

0 comments on commit 3d137c0

Please sign in to comment.