-
Bug
-
Resolution: Unresolved
-
P4
-
jfx14
Below is a test that creates an inputMap, adds and removes a single mapping, the inputMap should be eligible for gc but isn't.
This seems to be a left-over ofJDK-8150636, according to a code comment in removeMapping
private void removeMapping(Mapping<?> mapping) {
EventType<?> et = mapping.getEventType();
if (this.eventTypeMappings.containsKey(et)) {
List<?> _eventTypeMappings = this.eventTypeMappings.get(et);
_eventTypeMappings.remove(mapping);
// TODO remove the event handler in the root if there are no more mappings of this type
// anywhere in the input map tree
}
}
Guessing a bit: the todo wasn't handled due using a similar (incorrect) pattern for adding/removing the eventHandler as adding/removing a listener in ButtonBehaviorJDK-8245282:
// addMapping -> calls rootInputMap.addEventHandler(type)
// the latter is implemented as
void addEventHandler(type) {
...
final EventHandler<? super Event> eventHandler = this::handle;
node.addEventHandler(et, eventHandler);
}
Note the lambda, each time a new instance even though the intention is (afaics) to handle all types with the same handler, that is the InputMap itself.
A way might be the same as in ButtonBehavior: have a single instance for the handler and add/remove that instance on add/remove mappings:
final EventHandler<? super Event> eventHandler = this::handle;
void addEventHandler(type) {
...
if (not-yet-added)
node.addEventHandler(type, eventHandler);
}
void removeMapping(mapping) {
if (has-mapping) {
// cleanup internal bookkeeping
eventTypeMappings.remove(mapping)
node.removeEventHandler(type, eventHandler)
}
With the change the test below passes. Needs further evaluation, though, all tests are commented (and some use api that didn't make it into the release).
The failing test:
@Test
public void testInputMapMemoryLeak() {
Label label = new Label();
WeakReference<InputMap<?>> inputMap = new WeakReference<>(new InputMap<Label>(label));
// do-nothing mapping
KeyMapping mapping = new KeyMapping(SPACE, KeyEvent.KEY_PRESSED, e -> {} );
inputMap.get().getMappings().add(mapping);
assertEquals("sanity: mapping added", 1, inputMap.get().getMappings().size());
inputMap.get().getMappings().remove(mapping);
assertEquals("sanity: mapping removed", 0, inputMap.get().getMappings().size());
attemptGC(inputMap);
assertNull("inputMap must be gc'ed", inputMap.get());
}
This seems to be a left-over of
private void removeMapping(Mapping<?> mapping) {
EventType<?> et = mapping.getEventType();
if (this.eventTypeMappings.containsKey(et)) {
List<?> _eventTypeMappings = this.eventTypeMappings.get(et);
_eventTypeMappings.remove(mapping);
// TODO remove the event handler in the root if there are no more mappings of this type
// anywhere in the input map tree
}
}
Guessing a bit: the todo wasn't handled due using a similar (incorrect) pattern for adding/removing the eventHandler as adding/removing a listener in ButtonBehavior
// addMapping -> calls rootInputMap.addEventHandler(type)
// the latter is implemented as
void addEventHandler(type) {
...
final EventHandler<? super Event> eventHandler = this::handle;
node.addEventHandler(et, eventHandler);
}
Note the lambda, each time a new instance even though the intention is (afaics) to handle all types with the same handler, that is the InputMap itself.
A way might be the same as in ButtonBehavior: have a single instance for the handler and add/remove that instance on add/remove mappings:
final EventHandler<? super Event> eventHandler = this::handle;
void addEventHandler(type) {
...
if (not-yet-added)
node.addEventHandler(type, eventHandler);
}
void removeMapping(mapping) {
if (has-mapping) {
// cleanup internal bookkeeping
eventTypeMappings.remove(mapping)
node.removeEventHandler(type, eventHandler)
}
With the change the test below passes. Needs further evaluation, though, all tests are commented (and some use api that didn't make it into the release).
The failing test:
@Test
public void testInputMapMemoryLeak() {
Label label = new Label();
WeakReference<InputMap<?>> inputMap = new WeakReference<>(new InputMap<Label>(label));
// do-nothing mapping
KeyMapping mapping = new KeyMapping(SPACE, KeyEvent.KEY_PRESSED, e -> {} );
inputMap.get().getMappings().add(mapping);
assertEquals("sanity: mapping added", 1, inputMap.get().getMappings().size());
inputMap.get().getMappings().remove(mapping);
assertEquals("sanity: mapping removed", 0, inputMap.get().getMappings().size());
attemptGC(inputMap);
assertNull("inputMap must be gc'ed", inputMap.get());
}
- blocks
-
JDK-8241364 ☂ Cleanup skin implementations to allow switching
- Open
- relates to
-
JDK-8290844 Add Skin.install() method
- Resolved