8236840: Memory leak when switching ButtonSkin

[arapte]

ButtonSkin adds a ChangeListener to Control.sceneProperty() which results in leaking the ButtonSkin itself when the Button's skin is changed to a new ButtonSkin.
Using a WeakChangeListener instead of ChangeListener solves the issue.

Please take a look.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8236840: Memory leak when switching ButtonSkin

Reviewers

• Jeanette Winzenburg (fastegal - Author)
• Kevin Rushforth (kcr - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/147/head:pull/147
$ git checkout pull/147

[bridgekeeper]

👋 Welcome back arapte! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

[mlbridge]

Webrevs

• 03: Full - Incremental (bf974f6)
• 02: Full - Incremental (e81f175)
• 01: Full - Incremental (4362454)
• 00: Full (d393e81)

[kevinrushforth]

/reviewers 2

[openjdk]

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[kevinrushforth]

In general, there are two approaches to avoiding listener-related memory leaks. One is to use a WeakListener; the other is to explicitly remove the listener when the object is removed or otherwise no longer needed.

Using a WeakListener is certainly easier, but runs the risk of the listener being removed too early and not cleaning up after itself. I'm not suggesting that's the case here, but it is worth looking at. The one thing I would ask you to take a look at is whether it would matter if the old skin didn't call setDefaultButton(oldScene, false) when removed (and similarly setCancelButton).

[kevinrushforth]

@aghaisas can you also review?

[arapte]

Using a WeakListener is certainly easier, but runs the risk of the listener being removed too early and not cleaning up after itself. I'm not suggesting that's the case here, but it is worth looking at.

The listener does not get early GCed here. I did verify this by creating large number of ButtonSkins. The latest ButtonSkin set on the Button is never GCed

The one thing I would ask you to take a look at is whether it would matter if the old skin didn't call setDefaultButton(oldScene, false) when removed (and similarly setCancelButton).

This seems to be a bigger issue.
If we create multiple ButtonSkins for a given Button, then all the ButtonSkin register listeners with the properties of that Button. And it results in various listeners being added to same property of the Button. and this should be a common issue across all skins.

[arapte]

Hi Kevin, Please take a look at the updated changes.
Added tests as we discussed and an explicit call to remove the listener.

[kleopatra]

+1! Actually, looks like that manual remove is really needed - otherwise we get an NPE when removing the default button from its parent after the skin has been switched:

@Test
public void testDefaultButtonSwitchSkinAndRemove() {
    Button button = new Button();
    button.setDefaultButton(true);
    Group root = new Group(button);
    Scene scene = new Scene(root);
    Stage stage = new Stage();
    stage.setScene(scene);
    stage.show();
    
    button.setSkin(new ButtonSkin1(button));
    root.getChildren().remove(button);
}

Note: to see this NPE as failing test (vs. its printout on sysout), we need to re-wire the uncaughtExceptionHandler, see ComboBoxTest setup/cleanup for an example.

[arapte]

Thanks for the test case, I did minor changes to it and included in the next commit.
The NPE can occur even without button.setDefaultButton(true);.
Please take a look

[kleopatra]

good catch! You are right, without explicit removal of the listener, the NPE happens always.

Actually, I had been sloppy and got distracted by the NPE from my actual goal which was to dig into Kevin's "not cleaning up after itself" and .. finally found a (concededly extreme corner-case :) where that's happening: when setting the skin to null. Two failing tests:

@Test
public void testDefaultButtonNullSkinReleased() {
    Button button = new Button();
    button.setDefaultButton(true);
    Group root = new Group(button);
    Scene scene = new Scene(root);
    Stage stage = new Stage();
    stage.setScene(scene);
    stage.show();
    WeakReference<ButtonSkin> defSkinRef = new WeakReference<>((ButtonSkin)button.getSkin());
    button.setSkin(null);
    
    attemptGC(defSkinRef);
    assertNull("skin must be gc'ed", defSkinRef.get());
}

@Test
public void testDefaultButtonNullSkinAcceleratorRemoved() {
    Button button = new Button();
    button.setDefaultButton(true);
    Group root = new Group(button);
    Scene scene = new Scene(root);
    Stage stage = new Stage();
    stage.setScene(scene);
    stage.show();
    KeyCodeCombination key = new KeyCodeCombination(KeyCode.ENTER);

    assertNotNull(scene.getAccelerators().get(key));
    button.setSkin(null);
    assertNull(scene.getAccelerators().get(key));
    
}

An explicitly cleanup in dispose makes them pass:

@Override
public void dispose() {
    setDefaultButton(false);
    setCancelButton(false);
    getSkinnable().sceneProperty().removeListener(weakChangeListener);

[arapte]

I have included this change and the test, with slight modification to include same test for Cancel button.

[kleopatra]

looks good :)

[openjdk]

@arapte This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8236840: Memory leak when switching ButtonSkin

Reviewed-by: fastegal, kcr

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 12 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 247a65d2aad0332af0e35fa41f0aaaa8ced89cc2.

➡️ To integrate this PR with the above commit message, type /integrate in a new comment.

[arapte]

/integrate

[openjdk]

@arapte The following commits have been pushed to master since your change was applied:

• 247a65d: 8236971: [macos] Gestures handled incorrectly due to missing events
• 560ef17: 8241455: Memory leak on replacing selection/focusModel
• 5906521: 8241370: Crash in JPEGImageLoader after fix for JDK-8212034
• 159f651: 8240542: Switch FX build to use JDK 14 as boot JDK
• 6d098fe: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
• d7f13f4: 8089828: RTL Orientation, the flag of a mnemonic is not placed under the mnemonic letter.
• 9ecc107: 8240539: Upgrade gradle to version 6.3
• f3a3ea0: 8234471: Canvas in webview displayed with wrong scale on Windows
• d12e71c: 8241474: Build failing on Ubuntu 20.04
• 2a7ab36: 8089134: [2D traversal, RTL] TraversalEngine only handles left/right key traversal correctly in RTL for top-level engine in ToolBar
• 2aa8218: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
• e9c6119: 8240692: Cleanup of the javafx property objects

Your commit was automatically rebased without conflicts.

Pushed as commit 418675a.