Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8245303

InputMap: memory leak due to incomplete cleanup on remove mapping

XMLWordPrintable

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: P4 P4
    • tbd
    • jfx14
    • javafx

      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());
          }
          

            fastegal Jeanette Winzenburg
            fastegal Jeanette Winzenburg
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated: