<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>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.</p>
<p>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.</p>
<p>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).</p>
<p>--John<br>
</p>
On 19/04/2024 14:47, Thiago Milczarek Sayão wrote:<br>
<blockquote type="cite"
cite="mid:CAAP_wunU1X8Ax9tnnP5X-wE6Gfhj-xegH6Y2GPcV7oCfxaiGPQ@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<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>() {
Window[] windows = Window.getWindows().toArray(<span style="color:rgb(207,142,109)">new </span>Window[] {});
List<MenuItem> items = <span style="color:rgb(207,142,109)">new </span>ArrayList<>();
<span style="color:rgb(207,142,109)">for </span>(Window window : windows) {
<span style="color:rgb(207,142,109)">if </span>(window <span style="color:rgb(207,142,109)">instanceof </span>Stage stage && stage != primaryStage) {
MenuItem item = <span style="color:rgb(207,142,109)">new </span>MenuItem();
item.setText(stage.getTitle());
item.setOnAction(a -> <span style="color:rgb(199,125,187)">stage</span>.toFront());
item.setGraphic(<span style="color:rgb(207,142,109)">new </span>FontIcon());
items.add(item);
}
}
<span style="color:rgb(207,142,109)">for </span>(MenuItem item : btnWindows.getItems()) {
item.setOnAction(<span style="color:rgb(207,142,109)">null</span>);
}
btnWindows.getItems().setAll(items);
}
</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" moz-do-not-send="true"
class="moz-txt-link-freetext">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" moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">openjfx-dev-retn@openjdk.org</a>>
on behalf of Thiago Milczarek Sayão
<<a
href="mailto:thiago.sayao@gmail.com"
target="_blank"
moz-do-not-send="true"
class="moz-txt-link-freetext">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"
moz-do-not-send="true"
class="moz-txt-link-freetext">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>
</blockquote>
</body>
</html>