Bug ID: JDK-8244110 NPE in MenuButtonSkinBase change listener

JDK-8244110 : NPE in MenuButtonSkinBase change listener

Type: Bug
Component: javafx
Sub-Component: controls
Affected Version: openjfx15

Priority: P3
Status: Resolved
Resolution: Fixed

Submitted: 2020-04-29
Updated: 2020-05-05
Resolved: 2020-05-05

Versions (Unresolved/Resolved/Fixed)

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
openjfx15Fixed

Related Reports

Duplicate :	JDK-8244081 - Memory leak in MenuButtonSkinBase as control.sceneProperty() listener is not removed
Relates :	JDK-8175358 - Memory leak when moving MenuButton into another Scene
Relates :	JDK-8244081 - Memory leak in MenuButtonSkinBase as control.sceneProperty() listener is not removed

Description

The fix for JDK-8175358 has resulted in an NPE in the newly added MenuButtonSkinBase change listener.

To reproduce this bug, run the test.javafx.scene.control.MenuButtonSkinBaseNPETest.testMenuButtonNPE test as follows:

$ gradle -PFULL_TEST=true :systemTests:test --tests MenuButtonSkinBaseNPETest

test.javafx.scene.control.MenuButtonSkinBaseNPETest > testMenuButtonNPE FAILED
    org.junit.ComparisonFailure: No error should be thrown expected:<[]> but was:<[Exception in thread "JavaFX Application Thread" java.lang.NullPointerException
    	at javafx.controls/javafx.scene.control.skin.MenuButtonSkinBase.lambda$new$3(MenuButtonSkinBase.java:151)
    	at javafx.base/com.sun.javafx.binding.ExpressionHelper$SingleChange.fireValueChangedEvent(ExpressionHelper.java:181)
    	at javafx.base/com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:80)
    	at javafx.base/javafx.beans.property.ReadOnlyObjectPropertyBase.fireValueChangedEvent(ReadOnlyObjectPropertyBase.java:74)
    	at javafx.base/javafx.beans.property.ReadOnlyObjectWrapper.fireValueChangedEvent(ReadOnlyObjectWrapper.java:102)
    	at javafx.graphics/javafx.scene.Node$ReadOnlyObjectWrapperManualFire.fireSuperValueChangedEvent(Node.java:1050)
    	at javafx.graphics/javafx.scene.Node.invalidatedScenes(Node.java:1121)
    	at javafx.graphics/javafx.scene.Node.setScenes(Node.java:1159)
    	at javafx.graphics/javafx.scene.Parent$3.onProposedChange(Parent.java:605)
    	at javafx.base/com.sun.javafx.collections.VetoableListDecorator.clear(VetoableListDecorator.java:294)
    	at javafx.controls/javafx.scene.control.skin.MenuBarSkin.rebuildUI(MenuBarSkin.java:848)
    	at javafx.controls/javafx.scene.control.skin.MenuBarSkin.lambda$new$13(MenuBarSkin.java:359)
    	at javafx.base/com.sun.javafx.collections.ListListenerHelper$SingleChange.fireValueChangedEvent(ListListenerHelper.java:164)
    	at javafx.base/com.sun.javafx.collections.ListListenerHelper.fireValueChangedEvent(ListListenerHelper.java:73)
    	at javafx.base/javafx.collections.ObservableListBase.fireChange(ObservableListBase.java:233)
    	at javafx.base/javafx.collections.ListChangeBuilder.commit(ListChangeBuilder.java:482)
    	at javafx.base/javafx.collections.ListChangeBuilder.endChange(ListChangeBuilder.java:541)
    	at javafx.base/javafx.collections.ObservableListBase.endChange(ObservableListBase.java:205)
    	at javafx.base/com.sun.javafx.collections.ObservableListWrapper.clear(ObservableListWrapper.java:157)
    	at test.javafx.scene.control.MenuButtonSkinBaseNPETest.lambda$testMenuButtonNPE$0(MenuButtonSkinBaseNPETest.java:72)
    	at test.util.Util.lambda$submit$0(Util.java:96)
    	at javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runLater$10(PlatformImpl.java:428)
    	at java.base/java.security.AccessController.doPrivileged(AccessController.java:391)
    	at javafx.graphics/com.sun.javafx.application.PlatformImpl.lambda$runLater$11(PlatformImpl.java:427)
    	at javafx.graphics/com.sun.glass.ui.InvokeLaterDispatcher$Future.run(InvokeLaterDispatcher.java:96)
    	at javafx.graphics/com.sun.glass.ui.win.WinApplication._runLoop(Native Method)
    	at javafx.graphics/com.sun.glass.ui.win.WinApplication.lambda$runLoop$3(WinApplication.java:174)
    	at java.base/java.lang.Thread.run(Thread.java:832)
    ]>
        at org.junit.Assert.assertEquals(Assert.java:123)
        at test.javafx.scene.control.MenuButtonSkinBaseNPETest.testMenuButtonNPE(MenuButtonSkinBaseNPETest.java:76)

The code that is getting the NPE was added as part of the fix for for JDK-8175358 :

            if (oldValue != null) {
==>             ControlAcceleratorSupport.removeAcceleratorsFromScene(getSkinnable().getItems(), oldValue);
            }

this will get an NPE if getSkinnable() returns null at the time the the listener is called.

Comments

Changeset: 39d9c3b7 Author: Ajit Ghaisas <aghaisas@openjdk.org> Date: 2020-05-05 16:22:00 +0000 URL: https://git.openjdk.java.net/jfx/commit/39d9c3b7
05-05-2020
[~fastegal] Nice catch. For this specific case, your suggestion sounds good to me. We should also file a test stabilization bug to look into making the tests more robust in the long term.
02-05-2020
digging a bit into why this wasn't detected earlier (apart from me not running the system test ;) actually, we have 3 failing unit tests introduced in the commit of JDK-8175358, all in MenuBarTest, all NPEs blowing in the added block in sceneListener: testKeyNavigationWithAllDisabledMenuItems testMenuBarUpdateOnMenuVisibilityChange testRemovingMenuItemFromMenuNotInScene And we didn't notice them: command line gradle (and the suite running in Eclipse) all signaled green. That's because all errors happening during change notification are redirected to the uncaughtExceptionHandler, which by default simply prints them to system.out (or err, but details don't matter). That's where I found them, hidden in the usual bunch of log messages (should be removed from the tests) and warnings from csshelper (no idea where these come from) when running in Eclipse. The normal remedy is to redirect the uncaughtExceptionHandler (as we already do in several unit tests), something like: @After public void cleanup() { stage.hide(); Thread.currentThread().setUncaughtExceptionHandler(null); } @Before public void setup() { Thread.currentThread() .setUncaughtExceptionHandler((thread, throwable) -> { if (throwable instanceof RuntimeException) { throw (RuntimeException) throwable; } else { Thread.currentThread().getThreadGroup() .uncaughtException(thread, throwable); } }); root = new VBox(); scene = new Scene(root); stage = new Stage(); stage.setScene(scene); menuBar = new MenuBar(); } The problem here is that doing such in some tests (f.i. MenuBarTest) totally wrecks test integrity: I get tons of failures in unrelated tests, all/most (?) related to an NPE in ScenePulseListener (I don't understand why and don't really want dig - looks like some global state that's broken ;). As is, we can't redirect the uncaughtException when forcing a tk.pulse to be fired. What I did in this case is to extract the failing tests into a separate test, redirected the uncaughtException and now am seeing the errors as failures before the fix. After cleanup of the sceneListener two are passing (the third is testing update of dimensions, which aren't updated without forcing a layout). Added the extracted test here. Not sure how we can guard against overlooking errors in future. Cleanup the logging and look more closely at the sysout/err?
02-05-2020
seems to work: ChangeListener<? super Scene> 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()); } }; // in constructor, add to scene property control.sceneProperty().addListener(sceneChangeListener); // cleanup in dispose @Override public void dispose() { // uncomment next line for fix of JDK-8244112: backout if we are already disposed // doesn't affect this issue .. so I'm not certain if it should be added here? // if (getSkinnable() == null) return; // JDK-8244081: cleanup accelerators and remove scene listener if (getSkinnable().getScene() != null) { ControlAcceleratorSupport.removeAcceleratorsFromScene(getSkinnable().getItems(), getSkinnable().getScene()); } getSkinnable().sceneProperty().removeListener(sceneChangeListener); getSkinnable().getItems().removeListener(itemsChangedListener); super.dispose(); ... } the system test (and its adapted unit test version) fail without and pass with the cleanup in dispose. The pattern used in ButtonSkin is similar to this, but slightly different in that wraps the listener into a weakListener and adds/removes the weak wrapper. Which might be too much, because we remove it explicitly. Might do it the one or other way, but should be consistent :)
30-04-2020
can't know, but assume that the null check in the next line was added due to some not quite understood npe (maybe spurious, the test method above fails always in my context. Or maybe just a visibility issue? Need to set an uncaughtExceptionHandler in the test to re-direct the exception thrown by a property onto the test thread) Now we add another check guarding against a npe that's equally not quite understood .. and the next maintainer does it at another location with the argument and .. <g> What I would prefer is - not add a null check in the added branch, instead remove scenelistener cleanly in dispose (beware of JDK-8244112) - ideally remove the null check in the adding branch (or at least add a code comment that this should never happen)
30-04-2020
[~fastegal], In principle, I agree with you about null checks in skin code. This particular listener already has a null check (right on next line to my added line). So, I prefer to add this check in this case. Also, correct state would be achieved - by fixing - JDK-8244081. For the unit test, have you tested what you had suggested above? Looks like the unit test simply passes without catching NPE. That might be the reason why it has been added as a SystemTest.
30-04-2020
hmm ... be careful: no part of a skin is meant to be used when its skinnable is null. Or the other way round: getting there is an illegalState for some reason. A mere null check might be doctoring on symptons, in the extreme would end in spreading such checks across the whole skin code base. Just saying ..
30-04-2020
Fix of JDK-8244081(Memory leak in MenuButtonSkinBase as control.sceneProperty() listener is not removed) : may fix this NPE; but, for the sake of code correctness, we need this null check as suggested by Kevin.
30-04-2020
> - not add a null check in the added branch, instead remove scenelistener cleanly in dispose (beware of JDK-8244112) This does seem a cleaner solution, so as long as it works robustly, I would agree with this. > - ideally remove the null check in the adding branch (or at least add a code comment that this should never happen) This is an unrelated change. It would be fine to add a comment, but let's not remove the null check as part of fixing this bug.
30-04-2020
fix of the related might fix this as well ..
29-04-2020
oops .. guilty of not having run the system tests :( And I rarely do cough .. Actually, don't see any reason for this test being a system test - can be added as plain unit test to any of the MenuXXTests, and blows the same: /* * NPE introduced by fixing https://bugs.openjdk.java.net/browse/JDK-8175358 * was system test - trying to move it into normal scope. */ @Test public void testMenuButtonSkinNPE() { Menu menu = new Menu("Menu", null, new MenuItem("Press '_a'")); MenuBar menuBar = new MenuBar(menu); Scene scene = new Scene(menuBar); Stage primaryStage = new Stage(); primaryStage.setScene(scene); primaryStage.show(); menu.show(); menu.hide(); menuBar.getMenus().clear(); } As to the fix suggestion: I suspect that the real reason is the not-removing of the scene listener in dispose - we have seen a similar NPE during fixing JDK-8236840 before it was explicitly removed.
29-04-2020
A likely fix is to add a check for getSkinnable() != null here: if (oldValue != null && getSkinnable() != null) { ControlAcceleratorSupport.removeAcceleratorsFromScene(getSkinnable().getItems(), oldValue); }
29-04-2020