I think this code is bug free, and the added fix shouldn't be needed --
the old MenuItems should be cleaned up, and so should the referenced Stages.
So I think the bug is elsewhere, maybe not even in your code at all. I
did see PR's dealing with how MenuItems are linked to their platform
specific counterparts and how that can be a bit messy. I'm starting to
suspect maybe the MenuItems themselves are somehow still referenced
(perhaps only when they've been displayed once). These should also be
GC'd. They are however much smaller, so as long as they don't reference
a Stage it is probably not noticeable memory wise.
An inspection with VisualVM should give an answer (or you could make a
MenuItem very large by attaching some huge objects to it that are not
referenced elsewhere, via its properties map for example).
--John
On 19/04/2024 14:47, Thiago Milczarek Sayão wrote:
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 (windowinstanceof 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.hendr...@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.goryac...@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-r...@openjdk.org> on behalf
of Thiago Milczarek Sayão <thiago.sa...@gmail.com>
*Date: *Thursday, April 18, 2024 at 03:42
*To: *openjfx-dev <openjfx-dev@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.
