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.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.
>>
>
