8244110: NPE in MenuButtonSkinBase change listener

Co-authored-by: Jeanette Winzenburg <fastegal@openjdk.org> Reviewed-by: kcr, fastegal
openjdk · May 5, 2020 · 39d9c3b · 39d9c3b
1 parent 99f7747
commit 39d9c3b
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 5 deletions.
diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java
@@ -31,6 +31,7 @@
 import com.sun.javafx.scene.control.LabeledImpl;
 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;
@@ -73,7 +74,7 @@ public class MenuButtonSkinBase<C extends MenuButton> extends SkinBase<C> {
      */
     boolean behaveLikeButton = false;
     private ListChangeListener<MenuItem> itemsChangedListener;
-
+    private final ChangeListener<? super Scene> sceneChangeListener;
 
 
     /***************************************************************************
@@ -146,15 +147,18 @@ public MenuButtonSkinBase(final C control) {
         if (getSkinnable().getScene() != null) {
             ControlAcceleratorSupport.addAcceleratorsIntoScene(getSkinnable().getItems(), getSkinnable());
         }
-        control.sceneProperty().addListener((scene, oldValue, newValue) -> {
+
+        sceneChangeListener = (scene, oldValue, newValue) -> {
             if (oldValue != null) {
                 ControlAcceleratorSupport.removeAcceleratorsFromScene(getSkinnable().getItems(), oldValue);
             }
 
+             // FIXME: null skinnable should not happen
             if (getSkinnable() != null && getSkinnable().getScene() != null) {
                 ControlAcceleratorSupport.addAcceleratorsIntoScene(getSkinnable().getItems(), getSkinnable());
             }
-        });
+        };
+        control.sceneProperty().addListener(sceneChangeListener);
 
         // Register listeners
         registerChangeListener(control.showingProperty(), e -> {
@@ -214,6 +218,16 @@ public MenuButtonSkinBase(final C control) {
 
     /** {@inheritDoc} */
     @Override public void dispose() {
+        // FIXME : JDK-8244112 - backout if we are already disposed
+        // should check for getSkinnable to be null and 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 ) {

diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/MenuBarTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/MenuBarTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2020, 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
@@ -40,6 +40,7 @@
 import javafx.scene.layout.VBox;
 import javafx.stage.Stage;
 
+import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -70,11 +71,34 @@ public class MenuBarTest {
     private Stage stage;
 
     @Before public void setup() {
+        setUncaughtExceptionHandler();
+
         tk = (StubToolkit)Toolkit.getToolkit();
         menuBar = new MenuBar();
         menuBar.setUseSystemMenuBar(false);
     }
 
+    @After public void cleanup() {
+        if (stage != null) {
+            stage.hide();
+        }
+        removeUncaughtExceptionHandler();
+    }
+
+    private void setUncaughtExceptionHandler() {
+        Thread.currentThread().setUncaughtExceptionHandler((thread, throwable) -> {
+            if (throwable instanceof RuntimeException) {
+                throw (RuntimeException)throwable;
+            } else {
+                Thread.currentThread().getThreadGroup().uncaughtException(thread, throwable);
+            }
+        });
+    }
+
+    private void removeUncaughtExceptionHandler() {
+        Thread.currentThread().setUncaughtExceptionHandler(null);
+    }
+
     protected void startApp(Parent root) {
         scene = new Scene(root,800,600);
         stage = new Stage();
@@ -452,7 +476,6 @@ protected void startApp(Parent root) {
     }
 
     @Test public void testKeyNavigationWithAllDisabledMenuItems() {
-
         // Test key navigation with a single disabled menu in menubar
         VBox root = new VBox();
         Menu menu1 = new Menu("Menu1");