Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Canvas not marked as closed when its parent Surface is collected #66

Closed
ericek111 opened this issue Apr 8, 2024 · 3 comments
Closed

Comments

@ericek111
Copy link

While both Canvas and Surface implement AutoCloseable, when the GC hits, the Canvas is freed by its parent Surface (in the native lib) without marking the Canvas as closed. This causes a crash when doing anything with the orphaned Canvas object. It's most noticeable when resizing the window, since the whole thing gets thrown away (surface.close(); renderTarget.close()... unless I'm doing it wrong lol).

I believe when calling getCanvas() in Surface, the resulting Java object should be stored (cached) in the Surface object and returned with every subsequent call, and of course explicitly closed on cleanup.

public Canvas getCanvas() {
try {
Stats.onNativeCall();
long ptr = _nGetCanvas(_ptr);
return ptr == 0 ? null : new Canvas(ptr, false, this);
} finally {
ReferenceUtil.reachabilityFence(this);
}
}

and possibly the opposite in Canvas where it should return its parent Surface (same as _owner).

public Surface getSurface() {
try {
Stats.onNativeCall();
long ptr = _nGetSurface(_ptr);
return ptr == 0 ? null : new Surface(ptr);
} finally {
ReferenceUtil.reachabilityFence(this);
}
}

@tonsky
Copy link
Contributor

tonsky commented Apr 10, 2024

This will also cause an exception, because closing canvas from surface or any other way will set its ptr to 0 and you won’t be able to use it.

Are you saying that this is still preferable?

@ericek111
Copy link
Author

I'd say that yes, that would be better, because there will be only one Canvas Java object for each Surface (more efficient -- memory, less native calls), and if it's freed by the parent Surface being collected, isClosed() will properly reflect that, instead of the current situation where only the parent Managed object reports "closed".

So all in all, more straightforward, efficient and obvious (_ptr set to 0 instead of some weird undefined behaviour with a dangling pointer).

@tonsky tonsky closed this as completed in 33d80f1 Apr 15, 2024
@tonsky
Copy link
Contributor

tonsky commented Apr 15, 2024

Implemented, I’ll do a release soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants