<div dir="ltr">When the window list changes, I'm calling item.setOnAction(null) on the "old list" before inserting a new one.<div>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.<br></div><div><br></div><div>The code goes like this:</div><div><br></div><div><div style="background-color:rgb(30,31,34);color:rgb(188,190,196)"><pre style="font-family:"JetBrains Mono",monospace">Window.<span style="color:rgb(86,168,245)">getWindows</span>().<span style="color:rgb(86,168,245)">addListener</span>((ListChangeListener<? <span style="color:rgb(207,142,109)">super </span>Window>) change -> <span style="color:rgb(86,168,245)">updateWindowList</span>());</pre></div></div><div><br></div><div><div style="background-color:rgb(30,31,34);color:rgb(188,190,196)"><pre style="font-family:"JetBrains Mono",monospace"><span style="color:rgb(207,142,109)">private void </span><span style="color:rgb(86,168,245)">updateWindowList</span>() {<br> Window[] windows = Window.getWindows().toArray(<span style="color:rgb(207,142,109)">new </span>Window[] {});<br><br> List<MenuItem> items = <span style="color:rgb(207,142,109)">new </span>ArrayList<>();<br> <span style="color:rgb(207,142,109)">for </span>(Window window : windows) {<br> <span style="color:rgb(207,142,109)">if </span>(window <span style="color:rgb(207,142,109)">instanceof </span>Stage stage && stage != primaryStage) {<br> MenuItem item = <span style="color:rgb(207,142,109)">new </span>MenuItem();<br> item.setText(stage.getTitle());<br> item.setOnAction(a -> <span style="color:rgb(199,125,187)">stage</span>.toFront());<br> item.setGraphic(<span style="color:rgb(207,142,109)">new </span>FontIcon());<br> items.add(item);<br> }<br> }<br><br> <span style="color:rgb(207,142,109)">for </span>(MenuItem item : btnWindows.getItems()) {<br> item.setOnAction(<span style="color:rgb(207,142,109)">null</span>);<br> }<br><br> btnWindows.getItems().setAll(items);<br>}<br></pre></div></div><div><br></div><div>Maybe there's a bug, because the old list of items is collectable.</div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Em sex., 19 de abr. de 2024 às 01:37, John Hendrikx <<a href="mailto:john.hendrikx@gmail.com">john.hendrikx@gmail.com</a>> escreveu:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p>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:</p>
<p> menuItem ---> WeakEventHandler ---weakly---> Lambda</p>
<p>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).</p>
<p>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.<br>
</p>
<p>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.</p>
<p>I'd look into the above first before trying other solutions.</p>
<p>--John<br>
</p>
<p><br>
</p>
<div>On 18/04/2024 17:50, Thiago Milczarek
Sayão wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">I was investigating,
<div><br>
</div>
<div>It probably should be menuItem.setOnAction(new
WeakEventHandler<>(e -> stage.toFront()));</div>
<div><br>
</div>
<div>But I bet it's a common mistake. Maybe the setOnAction
should mention it?</div>
<div><br>
</div>
<div><br>
</div>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">Em qui., 18 de abr. de 2024 às
11:54, Andy Goryachev <<a href="mailto:andy.goryachev@oracle.com" target="_blank">andy.goryachev@oracle.com</a>>
escreveu:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<div lang="EN-US">
<div>
<p class="MsoNormal"><span>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.</span></p>
<p class="MsoNormal"><span> </span></p>
<p class="MsoNormal"><span>The lambda is essentially this:</span></p>
<p class="MsoNormal"><span> </span></p>
<p class="MsoNormal"><span>menuItem.setOnAction(new
H(stage));</span></p>
<p class="MsoNormal"><span>class $1 implements
EventHandler<ActionEvent> {</span></p>
<p class="MsoNormal"><span> private final Stage stage;</span></p>
<p class="MsoNormal"><span> public $1(Stage s) {</span></p>
<p class="MsoNormal"><span> this.stage = s; // holds the
reference and causes the leak</span></p>
<p class="MsoNormal"><span> }</span></p>
<p class="MsoNormal"><span> public void handle(ActionEvent
ev) {</span></p>
<p class="MsoNormal"><span> stage.toFront();</span></p>
<p class="MsoNormal"><span> }</span></p>
<p class="MsoNormal"><span>}</span></p>
<p class="MsoNormal"><span> </span></p>
<p class="MsoNormal"><span>-andy</span></p>
<p class="MsoNormal"><span> </span></p>
<div id="m_6611137440671845182m_-1465540377573045932mail-editor-reference-message-container">
<div>
<div style="border-right:none;border-bottom:none;border-left:none;border-top:1pt solid rgb(181,196,223);padding:3pt 0in 0in">
<p class="MsoNormal" style="margin-bottom:12pt"><b><span style="font-size:12pt;color:black">From:
</span></b><span style="font-size:12pt;color:black">openjfx-dev
<<a href="mailto:openjfx-dev-retn@openjdk.org" target="_blank">openjfx-dev-retn@openjdk.org</a>>
on behalf of Thiago Milczarek Sayão <<a href="mailto:thiago.sayao@gmail.com" target="_blank">thiago.sayao@gmail.com</a>><br>
<b>Date: </b>Thursday, April 18, 2024 at
03:42<br>
<b>To: </b>openjfx-dev <<a href="mailto:openjfx-dev@openjdk.org" target="_blank">openjfx-dev@openjdk.org</a>><br>
<b>Subject: </b>Possible leak on setOnAction</span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">Hi,</span></p>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> </span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">I'm pretty sure
setOnAction is holding references. </span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> </span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">I have a "Open
Windows" menu on my application where it
lists the Stages opened and if you click, it
calls stage.toFront():</span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> </span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">menuItem.seOnAction(e
-> stage.toFront())</span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> </span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">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.</span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> </span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">To fix it, I call
setOnAction(null) when the stage is closed.</span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> </span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">I will investigate
further and provide an example.</span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> </span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">-- Thiago.</span></p>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</blockquote></div>