HumbleUI / Skija

Java bindings for Skia
Apache License 2.0
498 stars 34 forks source link

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

Closed ericek111 closed 4 months ago

ericek111 commented 4 months ago

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.

https://github.com/HumbleUI/Skija/blob/a63330f37f24d57b457e625d861b121689c8d182/shared/java/Surface.java#L661-L669

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

https://github.com/HumbleUI/Skija/blob/a63330f37f24d57b457e625d861b121689c8d182/shared/java/Canvas.java#L91-L99

tonsky commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

Implemented, I’ll do a release soon