JDK-8244531 : Tests: add support to identify recurring issues with controls et al
  • Type: Enhancement
  • Component: javafx
  • Sub-Component: controls
  • Affected Version: openjfx15
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2020-05-06
  • Updated: 2020-11-19
  • Resolved: 2020-05-26
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
Blocks :  
JDK-8245282 - Button/Combo Behavior: memory leak on dispose
Relates :  
JDK-8256649 - Parameterized tests must not use instances as parameters
Relates :  
JDK-8241364 - ��� Cleanup skin implementations to allow switching
Relates :  
JDK-8244112 - Skin implementations: must not violate contract of dispose
Description
Currently, we identified several recurring issues with skin implementations, all (at least loosely) related to replacing skins. Amongst them:

- memory leaks: JDK-8241364 
- NPE, IOOBE or other harmful side-effects, f.i. JDK-8244110
- contract violation in dispose JDK-8244112 
- maybe others?

The very first task is to _find_ all these issues across all skins (not just by chance). The solution are (hopefully :) tests that are parameterized in control classes, focusing onto just a single (or several related) common misbehavior. That's already done for the contract violation in dispose. 

The task addressed here is use a similar pattern for memory leaks/side-effects.
Comments
Changeset: f3190db7 Author: Jeanette Winzenburg <fastegal@openjdk.org> Committer: Ajit Ghaisas <aghaisas@openjdk.org> Date: 2020-05-26 04:41:19 +0000 URL: https://git.openjdk.java.net/jfx/commit/f3190db7
26-05-2020
preliminary test snippets: the test methods: /** * default skin -> set null */ @Test public void testMemoryLeakNullSkin() { installDefaultSkin(control); WeakReference<?> weakRef = new WeakReference<>(control.getSkin()); assertNotNull(weakRef.get()); control.setSkin(null); attemptGC(weakRef); assertEquals("Skin must be gc'ed", null, weakRef.get()); } @Test public void testSideEffectNullSkin() { if (sideEffect == null) return; installDefaultSkin(control); control.setSkin(null); sideEffect.accept(control); } // the parameters // class of control to test, consumer for testing for side-effects after skin replaced Object[][] data = new Object[][] { // @Ignore("JDK-8241364") // {Accordion.class, null, }, {Button.class, null, }, // @Ignore("JDK-8241364") // {ButtonBar.class, null, }, {CheckBox.class, null, }, // was: memory leak and side-effect, fixed locally {ChoiceBox.class, (Consumer<Control>) c -> { ChoiceBox box = (ChoiceBox) c; box.getItems().add("added"); }, }, // @Ignore("JDK-8241364") // {ColorPicker.class, null, }, // @Ignore("JDK-8241364") // {ComboBox.class, null, }, {DateCell.class, null, }, // @Ignore("JDK-8241364") // {DatePicker.class, null, }, {Hyperlink.class, null, }, {Label.class, null, }, // @Ignore("JDK-8241364") // {ListCell.class, null, }, // @Ignore("JDK-8241364") // {ListView.class, null, }, // @Ignore("JDK-8241364") // {MenuBar.class, null, }, // @Ignore("JDK-8241364") // {MenuButton.class, null, }, // @Ignore("JDK-8241364") // {Pagination.class, null, }, // @Ignore("JDK-8241364") // {PasswordField.class, null, }, {ProgressBar.class, null, }, {ProgressIndicator.class, null, }, {RadioButton.class, null, }, // @Ignore("JDK-8241364") // {ScrollBar.class, null, }, // @Ignore("JDK-8241364") // {ScrollPane.class, null, }, {Separator.class, null, }, {Slider.class, null, }, // @Ignore("JDK-8241364") // {Spinner.class, null, }, // @Ignore("JDK-8241364") // {SplitMenuButton.class, null, }, // @Ignore("JDK-8241364") // {SplitPane.class, null, }, {TableCell.class, null, }, // @Ignore("JDK-8241364") // {TableRow.class, null, }, // @Ignore("JDK-8241364") // {TableView.class, null, }, // @Ignore("JDK-8241364") // {TabPane.class, null, }, // // @Ignore("8244419") // // {TextArea.class, null, }, // {TextField.class, null, }, {TitledPane.class, null, }, {ToggleButton.class, null, }, // @Ignore("JDK-8241364") // {ToolBar.class, null, }, // @Ignore("JDK-8241364") // {TreeCell.class, null, }, {TreeTableCell.class, null, }, // @Ignore("JDK-8241364") // {TreeTableRow.class, null, }, // @Ignore("JDK-8241364") // {TreeTableView.class, null, }, // @Ignore("JDK-8241364") // {TreeView.class, null, }, The un/commented classes pass/fail respectively the memoryLeak test (without testing side-effects). ChoiceBox did fail both the memoryLeak and the side-effect test - the failures were related to missing/incomplete listener cleanup (for different listeners, each), fixed locally for testing the test :) Once this support is stable enough, its useage would be to - file an issue per commented control, one after the other whenever we feel like it :) - uncomment the parameter and see the memory test fail - change the skin until the memory test passes: typically this involves finding listeners that were not auto-removed, manually remove until the one/s that cause the memory leak is nailed - during the last step, there might have been listeners that didn't cause the memory leak - these might still cause side-effect (like being active after the skin is disposed), try to find a change in the control that might make them blow, add that change as a consumer and cleanup until the side-effect test passes as well. As an example, ChoiceBoxSkin had two listeners that were not auto-removed (JDK-8244657) - a path listener to selectionModel.selectedIndexProperty that produced the memory leak - weakChoiceBoxItemsListener (the listener to modification of items): this might still be active after the skin is replaced, in the time after the skin was replaced until it will be gc'ed.
18-05-2020
diverted a bit from the initial plan (side-effects turned out to be too varying for a common handling) Final state (pending pull request): there's a utility class - to access lists of all control/classes, - to access/create all behaviors - has alternative skins classes for all controls and utility to install/replace skins POC: - has test examples (not run in normal testing due to not following naming conventions) for using - changed SkinDisposeContractTest to use
18-05-2020