HumbleUI/Skija

Canvas not marked as closed when its parent Surface is collected

Closed this issue · 3 comments

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);
}
}

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?

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).

Implemented, I’ll do a release soon