JDK-8209938 : Default and Cancel button cause memory leak
  • Type: Bug
  • Component: javafx
  • Sub-Component: controls
  • Affected Version: 8,9,10,openjfx11
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: x86_64
  • Submitted: 2018-08-23
  • Updated: 2022-10-21
  • Resolved: 2019-07-15
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other
openjfx13Fixed
Related Reports
Relates :  
Description
ADDITIONAL SYSTEM INFORMATION :
Windows 7 Professional 64-bit: Microsoft Windows [Version 6.1.7601]
java 10.0.2 2018-07-17
Java(TM) SE Runtime Environment 18.3 (build 10.0.2+13)
Java HotSpot(TM) 64-Bit Server VM 18.3 (build 10.0.2+13, mixed mode)

A DESCRIPTION OF THE PROBLEM :
Setting default button, ButtonSkin#defaultButtonRunnable to be added to Scene#getAccellerators() on new KeyCombination( KeyCode.ENTER ) in ButtonSkin#setDefaultButton(boolean).

Acceleration is never removed from Scene's accelerator map, only replaced by a new one - that causes memory leak.

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
1. Run attached test case application.
2. Press "Add tab".
3. Focus text field in the center, press ENTER on keyboard".
4. Remove that tab by clicking "X" in tab pane's header.
5. Add breakpoint to ButtonSkin#defaultButtonRunnable first line and press ENTER on keyboard or search for ButtonSkin in memory dump.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
After 3. Default action should be run - alert dialog will be shown.
After 5. ButtonSkin#defaultButtonRunnable is removed from the memory and breakpoint is not activated after pressing ENTER.
ACTUAL -
After 3. Default action should be run - alert dialog will be shown.
After 5. ButtonSkin#defaultButtonRunnable is held in memory, because of scene's acceleration and breakpoint is activated after pressing ENTER, but default action is not fired.

---------- BEGIN SOURCE ----------
package com.example;

import java.util.Objects;
import java.util.function.Consumer;

import javafx.application.Application;
import javafx.beans.value.ChangeListener;
import javafx.geometry.Insets;
import javafx.geometry.Pos;
import javafx.scene.Node;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.scene.control.Alert;
import javafx.scene.control.Button;
import javafx.scene.control.ButtonBar;
import javafx.scene.control.ButtonType;
import javafx.scene.control.Tab;
import javafx.scene.control.TabPane;
import javafx.scene.control.TextField;
import javafx.scene.input.KeyCode;
import javafx.scene.input.KeyCodeCombination;
import javafx.scene.layout.BorderPane;
import javafx.scene.layout.HBox;
import javafx.stage.Stage;

public class Main extends Application
{

    private int counter = 0;

    @Override
    public void start( final Stage primaryStage )
    {
        Scene scene = new Scene( createContent(), 800, 600 );
        primaryStage.setScene( scene );
        primaryStage.show();
    }

    private Parent createContent()
    {
        final BorderPane container = new BorderPane();
        container.setPadding( new Insets( 5 ) );
        final TabPane tabs = new TabPane();
        container.setCenter( tabs );
        final HBox aHBox = new HBox();
        final Button addButton = new Button( "Add tab" );
        addButton.setOnAction( event -> addTab( tabs ) );
        final Button addWorkaroundButton = new Button( "Add tab with workaround" );
        addWorkaroundButton.setOnAction( event -> addWorkaroundTab( tabs ) );
        final Button gcButton = new Button( "Run GC" );
        gcButton.setOnAction( event -> System.gc() );
        aHBox.getChildren().addAll( addButton, addWorkaroundButton, gcButton );
        container.setBottom( aHBox );
        BorderPane.setAlignment( aHBox, Pos.CENTER );
        return container;
    }

    private void addWorkaroundTab( final TabPane aTabs )
    {
        final String name = "Tab " + ++counter;
        final Tab aTab = new Tab( name );
        final DefaultButtonStateHelper defaultButtonStateHelper = new DefaultButtonStateHelper();
        aTab.setContent( createTabContent( name, button -> {
            defaultButtonStateHelper.applyDefaultButtonWorkaround( button, true );
        } ) );
        aTab.setOnClosed( event -> {
            defaultButtonStateHelper.cleanup();
        } );
        aTabs.getTabs().add( aTab );
    }

    private void addTab( final TabPane aTabs )
    {
        final String name = "Tab " + ++counter;
        final Tab aTab = new Tab( name );
        aTab.setContent( createTabContent( name, button -> button.setDefaultButton( true ) ) );
        aTabs.getTabs().add( aTab );
    }

    private Node createTabContent( final String aName, final Consumer< Button > aDefaultStateSetter )
    {
        final BorderPane ret = new BorderPane(  );
        final TextField aDummy = new TextField( "Read-only Dummy" );
        aDummy.setEditable( false );
        ret.setCenter( aDummy );
        final Button dummyButton = new Button( "DummyButton" );
        final Button defaultButton = new Button( "defaultButton" );
        aDefaultStateSetter.accept( defaultButton );
        defaultButton.setOnAction( event -> {
            final Alert aAlert =
                new Alert( Alert.AlertType.INFORMATION, "Default button was run for " + aName,
                    ButtonType.CLOSE );
            aAlert.showAndWait();
        } );
        final ButtonBar aButtonBar = new ButtonBar();
        aButtonBar.getButtons().addAll( dummyButton, defaultButton );
        ret.setBottom( aButtonBar );
        return ret;
    }

    public static void main(String[] args) {
        System.err.println( System.getProperty( "javafx.runtime.version" ) );
	    Application.launch( args );
    }


    public static class DefaultButtonStateHelper
    {

        private static final KeyCodeCombination DEFAULT_BUTTON_KEY_COMBINATION =
            new KeyCodeCombination( KeyCode.ENTER );
        private ChangeListener< Scene > defaultButtonSceneListener;
        private Button lastDefaultButton;

        private static void removeDefaultbuttonAcceleratorFromScene( final Scene aScene )
        {
            if( aScene == null )
            {
                return;
            }
            aScene.getAccelerators().remove( DEFAULT_BUTTON_KEY_COMBINATION );
        }

        public void applyDefaultButtonWorkaround( final Button aButton, final boolean isDefaultButton )
        {
            Objects.requireNonNull( aButton );
            if( isDefaultButton )
            {
                if( aButton != lastDefaultButton )
                {
                    cleanup();
                    lastDefaultButton = aButton;
                }
                if( defaultButtonSceneListener == null )
                {
                    defaultButtonSceneListener = ( observable, oldValue, newValue ) -> {
                        aButton.setDefaultButton( newValue != null );
                        if( newValue == null )
                        {
                            removeDefaultbuttonAcceleratorFromScene( oldValue );
                        }
                    };
                    aButton.sceneProperty().addListener( defaultButtonSceneListener );
                }
            }
            else
            {
                if( aButton != lastDefaultButton )
                {
                    throw new IllegalStateException(
                        "Last default button which has been registered here is different than given button." );
                }
                cleanup();
            }
        }

        public void cleanup()
        {
            if( lastDefaultButton != null && defaultButtonSceneListener != null )
            {
                lastDefaultButton.sceneProperty().removeListener( defaultButtonSceneListener );
            }
            defaultButtonSceneListener = null;
            lastDefaultButton = null;
        }
    }


}

---------- END SOURCE ----------

CUSTOMER SUBMITTED WORKAROUND :
added to executable test case, but instead of "Add tab", press "Add tab with workaround"

FREQUENCY : always



Comments
Changeset: ab32008d11a2 Author: arapte Date: 2019-07-16 01:20 +0530 URL: https://hg.openjdk.java.net/openjfx/13-dev/rt/rev/ab32008d11a2 8209938: Default and Cancel button cause memory leak Reviewed-by: kcr, jvos, nlisker
15-07-2019

Indeed, the listener is added only once per control so that should be ok. +1
12-07-2019

I note that the listener in question is associated with the per-node ReadOnlyObjectWrapper of the Button node and not with the Scene itself, and is only added at construction time of the ButtonSkin. Changing scene causes the listener to fire, but doesn't cause a new listener to be recreated. So I don't see a leak with the approach taken in webrev .01. I'm not 100% sure why DatePickerContent and TextFieldBehavior use WeakChangeListeners, but it doesn't seem like the same case (DatePickerContent in particular is adding a listener to an enclosing GridPane which might be associated with more than one DatePickerContent; using a WeakListener allows the DatePickerContent to be GC'ed even when there is still a reference to the GridPane). I can confirm that this fixes the leak. I also verified that the unit test fails without the fix and passes with the fix. I further instrumented the attached test case to prove that the last button doesn't get GC'ed due to the strong reference from the accelerator to the button without the fix, and that all buttons are GC'ed with the fix. The fix looks good to me. +1
11-07-2019

I understand the listeners are GC'ed when the control is gone, but when the sceneProperty is changing often, the old listeners are not removed (until the control is gc'ed). I noticed that e.g. DatePickerContent and TextFieldBehavior use WeakChangeListeners. I can live with the PR as it is though, this is very unlikely to be a major issue.
03-07-2019

The fix for the variable name looks good.
03-07-2019

Thanks Johan and Nir for the review, >>Isn't the sceneProperty.addListener() not introducing a memory leak? (the listener is never removed) The sceneProperty() is inherited by all controls from Node class. The property and added listeners would get GCed when the control gets GCed. Such listeners are also added to several other properties of control and are not explicitly removed. >>I would give it a more descriptive name Changed the method parameter name to isDefault and isCancel. Please take a look: http://cr.openjdk.java.net/~arapte/fx/8209938/webrev.01/
03-07-2019

Please review the fix for this issue : http://cr.openjdk.java.net/~arapte/fx/8209938/webrev.00/ Cause: Accelerators added for default and cancel button are not removed from scene, when the parent of button gets removed from scene. Fix: When the Button.sceneProperty() gets changed, then Default and Cancel button accelerators should be removed from the current scene's accelerators. Verification: Existing test do not fail due to this change. Added a unit test(fails without fix and passes with fix).
02-07-2019

There are methods that use a boolean named "value". What does that value represent? I would give it a more descriptive name.
02-07-2019

Looks ok from a functional point. Isn't the sceneProperty.addListener() not introducing a memory leak? (the listener is never removed)
02-07-2019

Issue is reproducible in all JDK 8u60, 8u172, 8u192, and latest openjfx (64-bit windows 10). Pressing "workaround tab" does not activate the breakpoint.
24-08-2018