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