<div dir="ltr">Calling item.setOnAction(null); avoids the leak.<div><br></div><div>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?</div><div><br></div><div>So if the MenuItem is collectable, the stage also becomes collectable if it's the only reference left to it.</div><div><br></div><div>I might be missing something obvious.</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Em sex., 19 de abr. de 2024 às 11:43, Andy Goryachev <<a href="mailto:andy.goryachev@oracle.com">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 class="msg4512306884120391755">
<div lang="EN-US" style="overflow-wrap: break-word;">
<div class="m_-6712235551185331346WordSection1">
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">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?<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16""><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16"">-andy<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:"Iosevka Fixed SS16""><u></u> <u></u></span></p>
<div id="m_-6712235551185331346mail-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>Friday, April 19, 2024 at 05:47<br>
<b>To: </b>John Hendrikx <<a href="mailto:john.hendrikx@gmail.com" target="_blank">john.hendrikx@gmail.com</a>><br>
<b>Cc: </b><a href="mailto:openjfx-dev@openjdk.org" target="_blank">openjfx-dev@openjdk.org</a> <<a href="mailto:openjfx-dev@openjdk.org" target="_blank">openjfx-dev@openjdk.org</a>><br>
<b>Subject: </b>Re: Possible leak on setOnAction<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">When the window list changes, I'm calling item.setOnAction(null) on the "old list" before inserting a new one.<u></u><u></u></span></p>
<div>
<p class="MsoNormal"><span style="font-size:12pt">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.<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"><u></u> <u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">The code goes like this:<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"><u></u> <u></u></span></p>
</div>
<div>
<div>
<pre style="background:rgb(30,31,34)"><span style="color:rgb(188,190,196)">Window.</span><span style="color:rgb(86,168,245)">getWindows</span><span style="color:rgb(188,190,196)">().</span><span style="color:rgb(86,168,245)">addListener</span><span style="color:rgb(188,190,196)">((ListChangeListener<? </span><span style="color:rgb(207,142,109)">super </span><span style="color:rgb(188,190,196)">Window>) change -> </span><span style="color:rgb(86,168,245)">updateWindowList</span><span style="color:rgb(188,190,196)">());<u></u><u></u></span></pre>
</div>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"><u></u> <u></u></span></p>
</div>
<div>
<div>
<pre style="background:rgb(30,31,34)"><span style="color:rgb(207,142,109)">private void </span><span style="color:rgb(86,168,245)">updateWindowList</span><span style="color:rgb(188,190,196)">() {<br> Window[] windows = Window.getWindows().toArray(</span><span style="color:rgb(207,142,109)">new </span><span style="color:rgb(188,190,196)">Window[] {});<br><br> List<MenuItem> items = </span><span style="color:rgb(207,142,109)">new </span><span style="color:rgb(188,190,196)">ArrayList<>();<br> </span><span style="color:rgb(207,142,109)">for </span><span style="color:rgb(188,190,196)">(Window window : windows) {<br> </span><span style="color:rgb(207,142,109)">if </span><span style="color:rgb(188,190,196)">(window </span><span style="color:rgb(207,142,109)">instanceof </span><span style="color:rgb(188,190,196)">Stage stage && stage != primaryStage) {<br> MenuItem item = </span><span style="color:rgb(207,142,109)">new </span><span style="color:rgb(188,190,196)">MenuItem();<br> item.setText(stage.getTitle());<br> item.setOnAction(a -> </span><span style="color:rgb(199,125,187)">stage</span><span style="color:rgb(188,190,196)">.toFront());<br> item.setGraphic(</span><span style="color:rgb(207,142,109)">new </span><span style="color:rgb(188,190,196)">FontIcon());<br> items.add(item);<br> }<br> }<br><br> </span><span style="color:rgb(207,142,109)">for </span><span style="color:rgb(188,190,196)">(MenuItem item : btnWindows.getItems()) {<br> item.setOnAction(</span><span style="color:rgb(207,142,109)">null</span><span style="color:rgb(188,190,196)">);<br> }<br><br> btnWindows.getItems().setAll(items);<br>}<u></u><u></u></span></pre>
</div>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"><u></u> <u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">Maybe there's a bug, because the old list of items is collectable.<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"><u></u> <u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"><u></u> <u></u></span></p>
</div>
</div>
<p class="MsoNormal"><span style="font-size:12pt"><u></u> <u></u></span></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">Em sex., 19 de abr. de 2024 às 01:37, John Hendrikx <<a href="mailto:john.hendrikx@gmail.com" target="_blank">john.hendrikx@gmail.com</a>> escreveu:<u></u><u></u></span></p>
</div>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in">
<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:<u></u><u></u></p>
<p> menuItem ---> WeakEventHandler ---weakly---> Lambda<u></u><u></u></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).<u></u><u></u></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.<u></u><u></u></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.<u></u><u></u></p>
<p>I'd look into the above first before trying other solutions.<u></u><u></u></p>
<p>--John<u></u><u></u></p>
<p><u></u> <u></u></p>
<div>
<p class="MsoNormal"><span style="font-size:12pt">On 18/04/2024 17:50, Thiago Milczarek Sayão wrote:<u></u><u></u></span></p>
</div>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<div>
<p class="MsoNormal"><span style="font-size:12pt">I was investigating, <u></u><u></u></span></p>
<div>
<p class="MsoNormal"><span style="font-size:12pt"><u></u> <u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">It probably should be menuItem.setOnAction(new WeakEventHandler<>(e -> stage.toFront()));<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"><u></u> <u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">But I bet it's a common mistake. Maybe the setOnAction should mention it?<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"><u></u> <u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"><u></u> <u></u></span></p>
</div>
</div>
<p class="MsoNormal"><span style="font-size:12pt"><u></u> <u></u></span></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">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:<u></u><u></u></span></p>
</div>
<blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0in 0in 0in 6pt;margin-left:4.8pt;margin-right:0in">
<div>
<div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">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.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt"> <u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt">The lambda is essentially this:<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt"> <u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt">menuItem.setOnAction(new H(stage));<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt">class $1 implements EventHandler<ActionEvent> {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt"> private final Stage stage;<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt"> public $1(Stage s) {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt"> this.stage = s; // holds the reference and causes the leak<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt"> }<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt"> public void handle(ActionEvent ev) {<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt"> stage.toFront();<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt"> }<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt">}<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt"> <u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt">-andy<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:12pt"> <u></u><u></u></span></p>
<div id="m_-6712235551185331346m_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><span style="font-size:12pt"><u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">Hi,<u></u><u></u></span></p>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> <u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">I'm pretty sure setOnAction is holding references. <u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> <u></u><u></u></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():<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> <u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">menuItem.seOnAction(e -> stage.toFront())<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> <u></u><u></u></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.<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> <u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">To fix it, I call setOnAction(null) when the stage is closed.<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> <u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">I will investigate further and provide an example.<u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt"> <u></u><u></u></span></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:12pt">-- Thiago.<u></u><u></u></span></p>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</div>
</div></blockquote></div>