JDK-8245303 : InputMap: memory leak due to incomplete cleanup on remove mapping
  • Type: Bug
  • Component: javafx
  • Sub-Component: controls
  • Affected Version: openjfx14
  • Priority: P4
  • Status: Open
  • Resolution: Unresolved
  • Submitted: 2020-05-19
  • Updated: 2022-07-21
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
tbdUnresolved
Related Reports
Blocks :  
Relates :  
Description
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 of JDK-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 ButtonBehavior JDK-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());
    }