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

[Ambarish Rapte]

On Mon, 7 Oct 2024 16:50:59 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);
>>     }
>> }
>
> Thiago Milczarek Sayao has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Let `createGraphics` fail if called after dispose

The fix itself looks good. Providing a few minor comments.

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

> 30: 
> 31:     private static native long nCreateDrawable(long nativeWindow, long nativeCtxInfo);
> 32:     private static native void nDestroyDrawable(long nativeCtxInfo);

I would recommend to rename these method as `nReleaseDrawable`.
I see we use names as  ReleaseXxxx and DeleteXxxx. The DestroyXxxx name would be a new addition, I think we could avoid the variation.

modules/javafx.graphics/src/main/native-prism-es2/ios/IOSGLDrawable.c line 55:

> 53: 
> 54:     /* initialize the structure */
> 55:     memset(dInfo, 0, sizeof(DrawableInfo));

Minor: The declaration of `initializeDrawableInfo` from line#35 should be removed.

modules/javafx.graphics/src/main/native-prism-es2/ios/IOSGLDrawable.c line 126:

> 124: 
> 125:     free(dInfo);
> 126: }

Minor: Please add a new line here.

modules/javafx.graphics/src/main/native-prism-es2/macosx/MacGLDrawable.c line 120:

> 118: 
> 119:     free(dInfo);
> 120: }

Minor: Please add a new line here.

modules/javafx.graphics/src/main/native-prism-es2/windows/WinGLDrawable.c line 76:

> 74: 
> 75:     /* initialize the structure */
> 76:     memset(dInfo, 0, sizeof(DrawableInfo));

Minor: The declaration of `initializeDrawableInfo` from line#358 should be removed.

modules/javafx.graphics/src/main/native-prism-es2/windows/WinGLDrawable.c line 148:

> 146: 
> 147:     free(dInfo);
> 148: }

Minor: Please add a new line here.

modules/javafx.graphics/src/main/native-prism-es2/x11/X11GLDrawable.c line 36:

> 34: #include "com_sun_prism_es2_X11GLDrawable.h"
> 35: 
> 36: extern void initializeDrawableInfo(DrawableInfo *dInfo);

Minor: The declaration of function `initializeDrawableInfo` should be removed as well.

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

Changes requested by arapte (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1586#pullrequestreview-2354297814
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791869606
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791777057
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791756735
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791771174
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791778528
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791771378
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791775168