[External] : Re: Possible leak on setOnAction

[Andy Goryachev]

What helped me in the past is to fire up VisualVM tool, select "Monitor" tab, click "Perform GC" button a couple of times, then "Heap Dump".  Once the heap dump is loaded, select the class in question and search for "GC Root".  This will show one of the paths to the root objects, often giving the answer on who is holding the reference.

-andy


From: Thiago Milczarek Sayão <thiago.sayao at gmail.com>
Date: Friday, April 19, 2024 at 07:58
To: Andy Goryachev <andy.goryachev at oracle.com>
Cc: John Hendrikx <john.hendrikx at gmail.com>, openjfx-dev at openjdk.org <openjfx-dev at openjdk.org>
Subject: [External] : Re: Possible leak on setOnAction
Calling item.setOnAction(null); avoids the leak.

But the question that remains is: When setItems is called on the menu button with new items, aren't the old items collectable by the GC?

So if the MenuItem is collectable, the stage also becomes collectable if it's the only reference left to it.

I might be missing something obvious.


Em sex., 19 de abr. de 2024 às 11:43, Andy Goryachev <andy.goryachev at oracle.com<mailto:andy.goryachev at oracle.com>> escreveu:
Are you sure the reference to stage is not held by something else?  Setting setOnAction(null) should remove the handler and its stage reference from the menu item's eventHandler, shouldn't it?

-andy

From: openjfx-dev <openjfx-dev-retn at openjdk.org<mailto:openjfx-dev-retn at openjdk.org>> on behalf of Thiago Milczarek Sayão <thiago.sayao at gmail.com<mailto:thiago.sayao at gmail.com>>
Date: Friday, April 19, 2024 at 05:47
To: John Hendrikx <john.hendrikx at gmail.com<mailto:john.hendrikx at gmail.com>>
Cc: openjfx-dev at openjdk.org<mailto:openjfx-dev at openjdk.org> <openjfx-dev at openjdk.org<mailto:openjfx-dev at openjdk.org>>
Subject: Re: Possible leak on setOnAction
When the window list changes, I'm calling item.setOnAction(null) on the "old list" before inserting a new one.
In general it's not a problem because the menu item or button is in a "context", like a Stage and everything is freed when the stage is closed. Maybe on long lasting stages.

The code goes like this:


Window.getWindows().addListener((ListChangeListener<? super Window>) change -> updateWindowList());


private void updateWindowList() {
    Window[] windows = Window.getWindows().toArray(new Window[] {});

    List<MenuItem> items = new ArrayList<>();
    for (Window window : windows) {
        if (window instanceof Stage stage && stage != primaryStage) {
            MenuItem item = new MenuItem();
            item.setText(stage.getTitle());
            item.setOnAction(a -> stage.toFront());
            item.setGraphic(new FontIcon());
            items.add(item);
        }
    }

    for (MenuItem item : btnWindows.getItems()) {
        item.setOnAction(null);
    }

    btnWindows.getItems().setAll(items);
}

Maybe there's a bug, because the old list of items is collectable.



Em sex., 19 de abr. de 2024 às 01:37, John Hendrikx <john.hendrikx at gmail.com<mailto:john.hendrikx at gmail.com>> escreveu:

This probably is a common mistake, however the Weak wrapper is also easy to use wrongly.  You can't just wrap it like you are doing in your example, because this is how the references look:

     menuItem ---> WeakEventHandler ---weakly---> Lambda

In effect, the Lambda is weakly referenced, and is the only reference, so it can be cleaned up immediately (or whenever the GC decides to run) and your menu item will stop working at a random time in the future.  The WeakEventHandler will remain, but only as a stub (and gets cleaned up when the listener list gets manipulated again at a later stage).

The normal way to use a Weak wrapper is to put a reference to the wrapped part in a private field, which in your case would not solve the problem.

I'm assuming however that you are also removing the menu item from the Open Windows list. This menu item should be cleaned up fully, and so the reference to the Stage should also disappear.  I'm wondering why that isn't happening?  If the removed menu item remains referenced somehow, then it's Action will reference the Stage, which in turns keeps the Stage in memory.

I'd look into the above first before trying other solutions.

--John


On 18/04/2024 17:50, Thiago Milczarek Sayão wrote:
I was investigating,

It probably should be menuItem.setOnAction(new WeakEventHandler<>(e -> stage.toFront()));

But I bet it's a common mistake. Maybe the setOnAction should mention it?



Em qui., 18 de abr. de 2024 às 11:54, Andy Goryachev <andy.goryachev at oracle.com<mailto:andy.goryachev at oracle.com>> escreveu:
You are correct - the lambda strongly references `stage` and since it is in turn is strongly referenced from the menu item it creates a leak.

The lambda is essentially this:

menuItem.setOnAction(new H(stage));
class $1 implements EventHandler<ActionEvent> {
  private final Stage stage;
  public $1(Stage s) {
    this.stage = s; // holds the reference and causes the leak
  }
  public void handle(ActionEvent ev) {
    stage.toFront();
  }
}

-andy

From: openjfx-dev <openjfx-dev-retn at openjdk.org<mailto:openjfx-dev-retn at openjdk.org>> on behalf of Thiago Milczarek Sayão <thiago.sayao at gmail.com<mailto:thiago.sayao at gmail.com>>
Date: Thursday, April 18, 2024 at 03:42
To: openjfx-dev <openjfx-dev at openjdk.org<mailto:openjfx-dev at openjdk.org>>
Subject: Possible leak on setOnAction
Hi,

I'm pretty sure setOnAction is holding references.

I have a "Open Windows" menu on my application where it lists the Stages opened and if you click, it calls stage.toFront():

menuItem.seOnAction(e -> stage.toFront())

I had many crash reports, all OOM. I got the hprof files and analyzed them - turns out this was holding references to all closed stages.

To fix it, I call setOnAction(null) when the stage is closed.

I will investigate further and provide an example.

-- Thiago.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240419/31031eba/attachment-0001.htm>