Skip to content

Commit

Permalink
8295426: MenuButtonSkin: memory leak when changing skin
Browse files Browse the repository at this point in the history 
Reviewed-by: aghaisas
  • Loading branch information
Andy Goryachev committed Dec 1, 2022
1 parent 0a785ae commit 4a19fe7
Show file tree
Hide file tree
Showing 4 changed files with 143 additions and 66 deletions.
31 changes: 22 additions & 9 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkin.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -26,11 +26,13 @@
package javafx.scene.control.skin;

import com.sun.javafx.scene.control.ContextMenuContent;
import com.sun.javafx.scene.control.ListenerHelper;
import com.sun.javafx.scene.control.behavior.MenuButtonBehavior;
import javafx.event.ActionEvent;
import javafx.scene.AccessibleAttribute;
import javafx.scene.control.Control;
import javafx.scene.control.MenuButton;

import com.sun.javafx.scene.control.behavior.MenuButtonBehavior;
import javafx.stage.WindowEvent;

/**
* Default skin implementation for the {@link MenuButton} control.
Expand Down Expand Up @@ -78,11 +80,13 @@ public MenuButtonSkin(final MenuButton control) {

// install default input map for the MenuButton-like controls
this.behavior = new MenuButtonBehavior(control);
// control.setInputMap(behavior.getInputMap());

// MenuButton's showing does not get updated when autoHide happens,
// as that hide happens under the covers. So we add to the menuButton's
// properties map to which the MenuButton can react and update accordingly..
// JDK-8295426:
// onAutoHide triggers an Event.ANY, making it impossible to add a listener which dispose() can remove.
// keeping the existing setOnAutoHide(), making sure to setOnAutoHide(null) later.
popup.setOnAutoHide(e -> {
MenuButton menuButton = getSkinnable();
// work around for the fact autohide happens twice
Expand All @@ -92,8 +96,10 @@ public MenuButtonSkin(final MenuButton control) {
}
});

ListenerHelper lh = ListenerHelper.get(this);

// request focus on content when the popup is shown
popup.setOnShown(event -> {
lh.addEventHandler(popup, WindowEvent.WINDOW_SHOWN, (ev) -> {
if (requestFocusOnFirstMenuItem) {
requestFocusOnFirstMenuItem();
requestFocusOnFirstMenuItem = false;
Expand All @@ -105,9 +111,9 @@ public MenuButtonSkin(final MenuButton control) {
}
});

if (control.getOnAction() == null) {
control.setOnAction(e -> control.show());
}
lh.addEventHandler(control, ActionEvent.ACTION, (ev) -> {
control.show();
});

label.setLabelFor(control);
}
Expand All @@ -121,7 +127,14 @@ public MenuButtonSkin(final MenuButton control) {
**************************************************************************/

/** {@inheritDoc} */
@Override public void dispose() {
@Override
public void dispose() {
if (getSkinnable() == null) {
return;
}

popup.setOnAutoHide(null);

super.dispose();

if (behavior != null) {
Expand Down
87 changes: 41 additions & 46 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java
View file
Expand Up @@ -25,30 +25,27 @@

package javafx.scene.control.skin;

import java.util.ArrayList;
import java.util.List;
import com.sun.javafx.scene.NodeHelper;
import com.sun.javafx.scene.control.ContextMenuContent;
import com.sun.javafx.scene.control.ControlAcceleratorSupport;
import com.sun.javafx.scene.control.LabeledImpl;
import com.sun.javafx.scene.control.ListenerHelper;
import com.sun.javafx.scene.control.behavior.MenuButtonBehaviorBase;
import com.sun.javafx.scene.control.skin.Utils;
import javafx.application.Platform;
import javafx.beans.value.ChangeListener;
import javafx.collections.ListChangeListener;
import javafx.event.ActionEvent;
import javafx.scene.Node;
import javafx.scene.Scene;
import javafx.scene.control.ContextMenu;
import javafx.scene.control.MenuButton;
import javafx.scene.control.MenuItem;
import javafx.scene.control.Skin;
import javafx.scene.control.SkinBase;
import javafx.scene.input.Mnemonic;
import javafx.scene.input.MouseEvent;
import javafx.scene.layout.Region;
import javafx.scene.layout.StackPane;
import com.sun.javafx.scene.control.behavior.MenuButtonBehaviorBase;

import java.util.ArrayList;
import java.util.List;

/**
* Base class for MenuButtonSkin and SplitMenuButtonSkin. It consists of the
Expand All @@ -73,8 +70,6 @@ public class MenuButtonSkinBase<C extends MenuButton> extends SkinBase<C> {
* If true, the control should behave like a button for mouse button events.
*/
boolean behaveLikeButton = false;
private ListChangeListener<MenuItem> itemsChangedListener;
private final ChangeListener<? super Scene> sceneChangeListener;


/* *************************************************************************
Expand All @@ -93,20 +88,22 @@ public class MenuButtonSkinBase<C extends MenuButton> extends SkinBase<C> {
public MenuButtonSkinBase(final C control) {
super(control);

ListenerHelper lh = ListenerHelper.get(this);

if (control.getOnMousePressed() == null) {
control.addEventHandler(MouseEvent.MOUSE_PRESSED, e -> {
lh.addEventHandler(control, MouseEvent.MOUSE_PRESSED, (ev) -> {
MenuButtonBehaviorBase behavior = getBehavior();
if (behavior != null) {
behavior.mousePressed(e, behaveLikeButton);
behavior.mousePressed(ev, behaveLikeButton);
}
});
}

if (control.getOnMouseReleased() == null) {
control.addEventHandler(MouseEvent.MOUSE_RELEASED, e -> {
lh.addEventHandler(control, MouseEvent.MOUSE_RELEASED, (ev) -> {
MenuButtonBehaviorBase behavior = getBehavior();
if (behavior != null) {
behavior.mouseReleased(e, behaveLikeButton);
behavior.mouseReleased(ev, behaveLikeButton);
}
});
}
Expand Down Expand Up @@ -136,44 +133,39 @@ public MenuButtonSkinBase(final C control) {

getSkinnable().requestLayout();

itemsChangedListener = c -> {
while (c.next()) {
popup.getItems().removeAll(c.getRemoved());
popup.getItems().addAll(c.getFrom(), c.getAddedSubList());
lh.addListChangeListener(control.getItems(), (ch) -> {
while (ch.next()) {
popup.getItems().removeAll(ch.getRemoved());
popup.getItems().addAll(ch.getFrom(), ch.getAddedSubList());
}
};
control.getItems().addListener(itemsChangedListener);

if (getSkinnable().getScene() != null) {
ControlAcceleratorSupport.addAcceleratorsIntoScene(getSkinnable().getItems(), getSkinnable());
}
});

List<Mnemonic> mnemonics = new ArrayList<>();
sceneChangeListener = (scene, oldValue, newValue) -> {
if (oldValue != null) {
ControlAcceleratorSupport.removeAcceleratorsFromScene(getSkinnable().getItems(), oldValue);

lh.addChangeListener(control.sceneProperty(), true, (src, oldScene, newScene) -> {
if (oldScene != null) {
ControlAcceleratorSupport.removeAcceleratorsFromScene(getSkinnable().getItems(), oldScene);

// We only need to remove the mnemonics from the old scene,
// they will be added to the new one as soon as the popup becomes visible again.
removeMnemonicsFromScene(mnemonics, oldValue);
removeMnemonicsFromScene(mnemonics, oldScene);
}

// FIXME: null skinnable should not happen
if (getSkinnable() != null && getSkinnable().getScene() != null) {
if (getSkinnable().getScene() != null) {
ControlAcceleratorSupport.addAcceleratorsIntoScene(getSkinnable().getItems(), getSkinnable());
}
};
control.sceneProperty().addListener(sceneChangeListener);
});

// Register listeners
registerChangeListener(control.showingProperty(), e -> {
lh.addChangeListener(control.showingProperty(), (ev) -> {
if (getSkinnable().isShowing()) {
show();
} else {
hide();
}
});
registerChangeListener(control.focusedProperty(), e -> {

lh.addChangeListener(control.focusedProperty(), (ev) -> {
// Handle tabbing away from an open MenuButton
if (!getSkinnable().isFocused() && getSkinnable().isShowing()) {
hide();
Expand All @@ -182,11 +174,13 @@ public MenuButtonSkinBase(final C control) {
hide();
}
});
registerChangeListener(control.mnemonicParsingProperty(), e -> {

lh.addChangeListener(control.mnemonicParsingProperty(), (ev) -> {
label.setMnemonicParsing(getSkinnable().isMnemonicParsing());
getSkinnable().requestLayout();
});
registerChangeListener(popup.showingProperty(), e -> {

lh.addChangeListener(popup.showingProperty(), (ev) -> {
if (!popup.isShowing() && getSkinnable().isShowing()) {
// Popup was dismissed. Maybe user clicked outside or typed ESCAPE.
// Make sure button is in sync.
Expand All @@ -197,13 +191,6 @@ public MenuButtonSkinBase(final C control) {
boolean showMnemonics = NodeHelper.isShowMnemonics(getSkinnable());
Utils.addMnemonics(popup, getSkinnable().getScene(), showMnemonics, mnemonics);
} else {
// we wrap this in a runLater so that mnemonics are not removed
// before all key events are fired (because KEY_PRESSED might have
// been used to hide the menu, but KEY_TYPED and KEY_RELEASED
// events are still to be fired, and shouldn't miss out on going
// through the mnemonics code (especially in case they should be
// consumed to prevent them being used elsewhere).
// See JBS-8090026 for more detail.
Scene scene = getSkinnable().getScene();
// JDK-8244234: MenuButton: NPE on removing from scene with open popup
if (scene != null) {
Expand All @@ -223,17 +210,17 @@ public MenuButtonSkinBase(final C control) {

/** {@inheritDoc} */
@Override public void dispose() {
if (getSkinnable() == null) return;
if (getSkinnable() == null) {
return;
}

// Cleanup accelerators
if (getSkinnable().getScene() != null) {
ControlAcceleratorSupport.removeAcceleratorsFromScene(getSkinnable().getItems(), getSkinnable().getScene());
}

// Remove listeners
getSkinnable().sceneProperty().removeListener(sceneChangeListener);
getSkinnable().getItems().removeListener(itemsChangedListener);
super.dispose();

if (popup != null ) {
if (popup.getSkin() != null && popup.getSkin().getNode() != null) {
ContextMenuContent cmContent = (ContextMenuContent)popup.getSkin().getNode();
Expand Down Expand Up @@ -319,6 +306,14 @@ private void hide() {
private void removeMnemonicsFromScene(List<Mnemonic> mnemonics, Scene scene) {
List<Mnemonic> mnemonicsToRemove = new ArrayList<>(mnemonics);
mnemonics.clear();

// we wrap this in a runLater so that mnemonics are not removed
// before all key events are fired (because KEY_PRESSED might have
// been used to hide the menu, but KEY_TYPED and KEY_RELEASED
// events are still to be fired, and shouldn't miss out on going
// through the mnemonics code (especially in case they should be
// consumed to prevent them being used elsewhere).
// See JDK-8090026 for more detail.
Platform.runLater(() -> mnemonicsToRemove.forEach(scene::removeMnemonic));
}

Expand Down
87 changes: 78 additions & 9 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/MenuButtonTest.java
View file
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand All @@ -25,27 +25,37 @@

package test.javafx.scene.control;

import javafx.scene.control.skin.MenuButtonSkin;
import com.sun.javafx.tk.Toolkit;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;

import javafx.beans.property.SimpleObjectProperty;
import javafx.event.Event;
import javafx.event.EventHandler;
import javafx.event.EventType;
import javafx.geometry.Side;
import javafx.scene.Scene;
import javafx.scene.Group;
import javafx.scene.control.ContentDisplay;
import javafx.scene.Scene;
import javafx.scene.control.Button;
import javafx.scene.control.ContentDisplay;
import javafx.scene.control.MenuButton;
import javafx.scene.control.MenuItem;
import javafx.scene.control.skin.MenuButtonSkin;
import javafx.scene.input.MouseEvent;
import javafx.scene.layout.VBox;
import javafx.scene.shape.Rectangle;
import javafx.stage.Stage;
import javafx.event.Event;
import javafx.event.EventType;
import javafx.event.EventHandler;

import org.junit.Before;
import org.junit.Test;

import static org.junit.Assert.*;
import com.sun.javafx.scene.SceneHelper;
import com.sun.javafx.tk.Toolkit;

import test.com.sun.javafx.scene.control.infrastructure.MouseEventGenerator;

/**
*
Expand Down Expand Up @@ -653,4 +663,63 @@ public void handle(Event event) {
assertEquals("MenuButton.onHidden bean should be MenuButton object.",
mbtn, mbtn.onHiddenProperty().getBean());
}

/**
* JDK-8295426 menu button popup is not shown on mouse click after skin is
* replaced.
*/
@Test
public void testMenuButtonPopupAfterSkinReplaced() {
MenuItem mi = new MenuItem("MenuItem1");

MenuButton b = new MenuButton("Menu Button");
b.getItems().add(mi);

VBox root = new VBox();
root.getChildren().addAll(b);

Scene scene = new Scene(root, 800, 600);
Stage stage = new Stage();
// keep stage at 0,0 so the screen X/Y to be the same as local Node X/Y, for
// MouseEventGenerator
stage.setX(0);
stage.setY(0);
stage.setScene(scene);
stage.show();
stage.requestFocus();

Toolkit.getToolkit().firePulse();

MenuButtonSkin skin = (MenuButtonSkin)b.getSkin();
assertTrue(skin != null);

double offset = 15;
double x = (b.localToScene(b.getLayoutBounds())).getMinX() + offset;
double y = (b.localToScene(b.getLayoutBounds())).getMinY() + offset;

// click on menu button to show the popup
SceneHelper.processMouseEvent(scene, MouseEventGenerator.generateMouseEvent(MouseEvent.MOUSE_PRESSED, x, y));
SceneHelper.processMouseEvent(scene, MouseEventGenerator.generateMouseEvent(MouseEvent.MOUSE_RELEASED, x, y));
assertTrue(b.isShowing());

// change the skin
b.setSkin(new MenuButtonSkin(b));
b.requestFocus();
Toolkit.getToolkit().firePulse();
assertTrue(b.isFocused());

// click once more on menu button to hide the popup
SceneHelper.processMouseEvent(scene, MouseEventGenerator.generateMouseEvent(MouseEvent.MOUSE_PRESSED, x, y));
SceneHelper.processMouseEvent(scene, MouseEventGenerator.generateMouseEvent(MouseEvent.MOUSE_RELEASED, x, y));
Toolkit.getToolkit().firePulse();
assertFalse(b.isShowing()); // fails JDK-8295426

// click again to show the popup
SceneHelper.processMouseEvent(scene, MouseEventGenerator.generateMouseEvent(MouseEvent.MOUSE_PRESSED, x, y));
SceneHelper.processMouseEvent(scene, MouseEventGenerator.generateMouseEvent(MouseEvent.MOUSE_RELEASED, x, y));
Toolkit.getToolkit().firePulse();
assertTrue(b.isShowing());

stage.hide();
}
}

0 comments on commit 4a19fe7

Please sign in to comment.