RFR: 8341418: Prism/es2 DrawableInfo is never freed (leak)

[Michael Strauß]

On Tue, 1 Oct 2024 17:37:15 GMT, Thiago Milczarek Sayao <tsayao at openjdk.org> wrote:

> When creating a Scene, a `DrawableInfo` is allocated with `malloc`. When scene changes, this is called on `WindowStage.java`:
> 
> `QuantumRenderer.getInstance().disposePresentable(painter.presentable);   // latched on RT`
> 
> But the underlying `DrawableInfo` is never freed.
> 
> I also think this should be done when the Stage is closed.
> 
> To test:
> 
> import javafx.animation.Animation;
> import javafx.animation.KeyFrame;
> import javafx.animation.KeyValue;
> import javafx.animation.Timeline;
> import javafx.application.Application;
> import javafx.scene.Scene;
> import javafx.scene.control.TextField;
> import javafx.scene.control.Label;
> import javafx.scene.layout.Pane;
> import javafx.scene.layout.StackPane;
> import javafx.scene.layout.VBox;
> import javafx.scene.paint.Color;
> import javafx.stage.Stage;
> import javafx.util.Duration;
> 
> public class TestScenes extends Application {
> 
>     @Override
>     public void start(Stage stage) {
>         Timeline timeline = new Timeline(
>                 new KeyFrame(Duration.millis(100), e -> stage.setScene(createScene("Scene 1", Color.RED))),
>                 new KeyFrame(Duration.millis(200), e -> stage.setScene(createScene("Scene 2", Color.BLUE))),
>                 new KeyFrame(Duration.millis(300), e -> stage.setScene(createScene("Scene 3", Color.GREEN)))
>         );
> 
>         timeline.setCycleCount(Animation.INDEFINITE);
>         timeline.play();
> 
>         stage.show();
>     }
> 
>     private Scene createScene(String text, Color color) {
>         return new Scene(new StackPane(), 400, 300, color);
>     }
> 
>     public static void main(String[] args) {
>         launch(TestScenes.class, args);
>     }
> }

The fix works on Windows and macOS. I've left some additional comments.

modules/javafx.graphics/src/main/java/com/sun/prism/es2/ES2SwapChain.java line 193:

> 191:     @Override
> 192:     public ES2Graphics createGraphics() {
> 193:         if (drawable == null || drawable.getNativeWindow() != pState.getNativeWindow()) {

The null check wasn't here before, and it doesn't seem necessary as `createGLDrawable()` does not return `null`.

modules/javafx.graphics/src/main/java/com/sun/prism/es2/GLDrawable.java line 56:

> 54:     abstract boolean swapBuffers(GLContext glCtx);
> 55: 
> 56:     abstract void dispose();

`GLDrawable` could also get the dispose method by implementing `GraphicsResource`.

modules/javafx.graphics/src/main/java/com/sun/prism/es2/IOSGLDrawable.java line 57:

> 55:     @Override
> 56:     void dispose() {
> 57:         nDestroyDrawable(getNativeDrawableInfo());

You should set `nativeDrawableInfo` to zero in this method (and check this before calling the native method), as otherwise we run the risk of freeing a stale pointer when the `dispose()` method is called repeatedly. The same should be done in all other implementations.

modules/javafx.graphics/src/main/native-prism-es2/GLDrawable.c line 44:

> 42: }
> 43: 
> 44: void deleteDrawableInfo(DrawableInfo *dInfo)

Maybe delete this whole file, as its only content is now a function that just calls `memset()`.

-------------

PR Review: https://git.openjdk.org/jfx/pull/1586#pullrequestreview-2350517717
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1789053938
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1789054227
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1789054946
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1789055920