RFR: 8263402: MemoryLeak: Node hardreferences it's previous Parent after csslayout and getting removed from the scene [v2]

[Florian Kirmaier]

On Fri, 12 Mar 2021 07:55:54 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> Florian Kirmaier has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   8263402
>>   Added new verison of the unit-test
>
> tests/system/src/test/java/test/javafx/scene/StyleMemoryLeakTest.java line 106:
> 
>> 104:         });
>> 105:     }
>> 106: }
> 
> In order to make this test similar to existing system tests, I made some relevant changes. Modified test is added below.
> The modified test fails with this fix, but I expected it to pass. Can you please check this.
> 
> Changes are 
> 1. `Thread.sleep()` is removed.
> 2. `root` and `toBeRemoved` button are now class members.
> 3. Scenegraph is constructed and shown in `TestApp.start()` method.
> 
> 
> public class StyleMemoryLeakTest {
> 
>     static CountDownLatch startupLatch;
>     static Stage stage;
>     static Button toBeRemoved;
>     static Group root;
> 
>     public static class TestApp extends Application {
> 
>         @Override
>         public void start(Stage primaryStage) throws Exception {
>             stage = primaryStage;
>             toBeRemoved = new Button();
>             root = new Group();
>             root.getChildren().add(toBeRemoved);
>             stage.setScene(new Scene(root));
>             stage.setOnShown(l -> {
>                 Platform.runLater(() -> startupLatch.countDown());
>             });
>             stage.show();
>         }
>     }
> 
>     @BeforeClass
>     public static void initFX() throws Exception {
>         startupLatch = new CountDownLatch(1);
>         new Thread(() -> Application.launch(StyleMemoryLeakTest.TestApp.class, (String[])null)).start();
>         assertTrue("Timeout waiting for FX runtime to start", startupLatch.await(15, TimeUnit.SECONDS));
>     }
> 
>     @Test
>     public void testRootNodeMemoryLeak() throws Exception {
>         Util.runAndWait(() -> {
>             root.getChildren().clear();
>             stage.hide();
>         });
>         JMemoryBuddy.memoryTest((checker) -> {
>             checker.assertCollectable(stage);
>             checker.setAsReferenced(toBeRemoved);
>             stage = null;
>         });
>     }
> 
>     @AfterClass
>     public static void teardownOnce() {
>         Platform.runLater(() -> {
>             Platform.exit();
>         });
>     }
> }

I've added your verison of the unit-test. You forgot `root = null;` which was why the test failed.

If I would rewrite the test, I would put everything into the TestMethod. Because this way it's not necessary to set all the class-variables to null. It also wouldn't reuse the Stage of the Application. The scope of the test would be much smaller, because the actual test and the initialization of JavaFX would be separated. 

Should I change it that way?

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

PR: https://git.openjdk.java.net/jfx/pull/424