Re: Possible leak on setOnAction
I think the leaks (seems to be more than one) happens on:
ContextMenuContent.disposeVisualItems
There are visual items holding references.
They seem to be on MenuItemContainer
It looks like its held on:
mouseEnteredEventHandler
mouseReleasedEventHandler
actionEventHandler
And somewhere else I didn't find.
That's my theory for now.
Em sáb., 20 de abr. de 2024 às 10:47, Thiago Milczarek Sayão <
thiago.sa...@gmail.com> escreveu:
> Yeah, I've been doing some investigating.
>
> Using the below example, when the "Refresh Menu" button is clicked, it
> will replace the items making the old one collectable by the GC.
> If the Menu was never used/drop down shown, it will get collected.
> If it was used (just shown), it remains in memory (seen on VisualVM).
>
> GC Root points to ContextMenuContent.MenuItemContainer
>
> @Override
> public void start(Stage primaryStage) {
> this.primaryStage = primaryStage;
> primaryStage.setTitle("Hello World!");
> Button btn = new Button("New Stage");
>
> Button btnRefresh = new Button("Refresh Menu");
>
> ButtonBar buttonBar = new ButtonBar();
> buttonBar.getButtons().addAll(menu, btnRefresh);
>
> btnRefresh.setOnAction(e -> {
> menu.getItems().setAll(getMenuItem());
> });
>
> Scene scene = new Scene(buttonBar);
> primaryStage.setScene(scene);
> primaryStage.show();
> menu.getItems().add(getMenuItem());
> }
>
> private MenuItem getMenuItem() {
> MenuItem item = new MenuItem();
> item.setText("Menu Item");
> item.setOnAction(a -> {
> System.out.println("ACTION");
> });
>
> return item;
> }
>
>
>
> Em sáb., 20 de abr. de 2024 às 05:01, John Hendrikx <
> john.hendr...@gmail.com> escreveu:
>
>> 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) change
>> -> updateWindowList());
>>
>>
>> private void updateWindowList() {
>> Window[] windows = Window.getWindows().toArray(new Window[] {});
>>
>> List 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
>>> h
Re: Possible leak on setOnAction
Yeah, I've been doing some investigating.
Using the below example, when the "Refresh Menu" button is clicked, it will
replace the items making the old one collectable by the GC.
If the Menu was never used/drop down shown, it will get collected.
If it was used (just shown), it remains in memory (seen on VisualVM).
GC Root points to ContextMenuContent.MenuItemContainer
@Override
public void start(Stage primaryStage) {
this.primaryStage = primaryStage;
primaryStage.setTitle("Hello World!");
Button btn = new Button("New Stage");
Button btnRefresh = new Button("Refresh Menu");
ButtonBar buttonBar = new ButtonBar();
buttonBar.getButtons().addAll(menu, btnRefresh);
btnRefresh.setOnAction(e -> {
menu.getItems().setAll(getMenuItem());
});
Scene scene = new Scene(buttonBar);
primaryStage.setScene(scene);
primaryStage.show();
menu.getItems().add(getMenuItem());
}
private MenuItem getMenuItem() {
MenuItem item = new MenuItem();
item.setText("Menu Item");
item.setOnAction(a -> {
System.out.println("ACTION");
});
return item;
}
Em sáb., 20 de abr. de 2024 às 05:01, John Hendrikx
escreveu:
> 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) change
> -> updateWindowList());
>
>
> private void updateWindowList() {
> Window[] windows = Window.getWindows().toArray(new Window[] {});
>
> List 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
Re: Possible leak on setOnAction
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) change
->updateWindowList());
private void updateWindowList() {
Window[] windows = Window.getWindows().toArray(new Window[] {});
List 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
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
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 {
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 on behalf
of Thiago Milczarek Sayão
*Date: *Thursday, April 18, 2024 at 03:42
*To: *openjfx-dev
*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.
Re: [External] : Re: Possible leak on setOnAction
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
Date: Friday, April 19, 2024 at 07:58
To: Andy Goryachev
Cc: John Hendrikx , openjfx-dev@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
mailto:andy.goryac...@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
mailto:openjfx-dev-r...@openjdk.org>> on behalf
of Thiago Milczarek Sayão
mailto:thiago.sa...@gmail.com>>
Date: Friday, April 19, 2024 at 05:47
To: John Hendrikx mailto:john.hendr...@gmail.com>>
Cc: openjfx-dev@openjdk.org<mailto:openjfx-dev@openjdk.org>
mailto:openjfx-dev@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) change ->
updateWindowList());
private void updateWindowList() {
Window[] windows = Window.getWindows().toArray(new Window[] {});
List 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
mailto: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
mailto: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 {
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
mailto:openjfx-dev-r...@openjdk.org>> on behalf
of Thiago
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.goryac...@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 on behalf of Thiago
> Milczarek Sayão
> *Date: *Friday, April 19, 2024 at 05:47
> *To: *John Hendrikx
> *Cc: *openjfx-dev@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) change
> -> updateWindowList());
>
>
>
> private void updateWindowList() {
> Window[] windows = Window.getWindows().toArray(new Window[] {});
>
> List 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 {
>
> 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 on behalf of Thiago
> Milczarek Sayão
> *Date: *Thursday, April 18, 2024 at 03:42
> *To: *openjfx-dev
> *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.
>
>
Re: Possible leak on setOnAction
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 on behalf of Thiago Milczarek
Sayão
Date: Friday, April 19, 2024 at 05:47
To: John Hendrikx
Cc: openjfx-dev@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) change ->
updateWindowList());
private void updateWindowList() {
Window[] windows = Window.getWindows().toArray(new Window[] {});
List 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
mailto: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
mailto: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 {
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
mailto:openjfx-dev-r...@openjdk.org>> on behalf
of Thiago Milczarek Sayão
mailto:thiago.sa...@gmail.com>>
Date: Thursday, April 18, 2024 at 03:42
To: openjfx-dev mailto: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.
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)
change -> updateWindowList());
private void updateWindowList() {
Window[] windows = Window.getWindows().toArray(new Window[] {});
List 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
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 {
>>
>> 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 on behalf of Thiago
>> Milczarek Sayão
>> *Date: *Thursday, April 18, 2024 at 03:42
>> *To: *openjfx-dev
>> *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.
>>
>
Re: Possible leak on setOnAction
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
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 {
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 on behalf of
Thiago Milczarek Sayão
*Date: *Thursday, April 18, 2024 at 03:42
*To: *openjfx-dev
*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.
Re: Possible leak on setOnAction
I didn't find such memory leaks in my application, though I don't do stage handling. What I would look at is where the `stage` reference in the lambda is coming from. You say you have a list of open stages. When you close a stage, do you remove the references to that stage from all places? What about the object that is holding the reference `stage` in the lambda? On Thu, Apr 18, 2024 at 1:58 PM Thiago Milczarek Sayão < thiago.sa...@gmail.com> wrote: > 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. >
Re: [External] : Re: Possible leak on setOnAction
Well, then all these setOnXXX() should mention it as well, wouldn't you say?
Perhaps a better solution might be a general purpose tutorial which addresses
the common pitfalls like memory leaks, and new features like Subscription.
We do have these tutorials for java8 -
https://docs.oracle.com/javase/8/javafx/events-tutorial/events.htm
But since JavaFX has been decoupled from the JDK we don't seem to have anything
more recent. I don't know whether this situation is going to change any time
soon.
-andy
From: Thiago Milczarek Sayão
Date: Thursday, April 18, 2024 at 08:51
To: Andy Goryachev
Cc: openjfx-dev
Subject: [External] : Re: Possible leak on setOnAction
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
mailto: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 {
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
mailto:openjfx-dev-r...@openjdk.org>> on behalf
of Thiago Milczarek Sayão
mailto:thiago.sa...@gmail.com>>
Date: Thursday, April 18, 2024 at 03:42
To: openjfx-dev mailto: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.
Re: Possible leak on setOnAction
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 {
>
> 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 on behalf of Thiago
> Milczarek Sayão
> *Date: *Thursday, April 18, 2024 at 03:42
> *To: *openjfx-dev
> *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.
>
Re: Possible leak on setOnAction
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 {
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 on behalf of Thiago Milczarek
Sayão
Date: Thursday, April 18, 2024 at 03:42
To: openjfx-dev
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.
