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