Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 10:04:48 GMT, Dean Wookey wrote: >> Each time a menu would change scenes, a new set of ListChangeListeners would >> be added to the items in the menu. The bigger problem however is that these >> list change listeners have a strong reference to the scene which is >> potentially a much bigger leak. >> >> The first commit was more straightforward, but there are 2 things that might >> be of concern: >> >> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but >> it'll remove all the ListChangeListeners attached to it, regardless of which >> scene was passed in. Something similar happens with changeListenerMap >> already, so I think it's fine. >> 2. I made the remove method public so that external calls from skins to >> remove the accelerators would remove the ListChangeListener and also because >> all the remove methods are public. >> >> After I wrote more tests I realised using the ObservableLists as keys in the >> WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would >> be simple. The fix is in the second commit. >> >> There are still more issues that stem from the fact that for each anchor >> there could be multiple menus and the current code doesn't account for that. >> For example, swapping context menus on a tab doesn't register change >> listeners on the new context menu because the TabPane itself had a scene >> change listener already. These other issues could relate to JDK-8268374 >> https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 >> https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to >> the fact that there's no logic to remove listeners when Tab/TableColumn's >> are removed from their associated control (TabPane, TableView, >> TreeTableView). >> >> I'm looking at these issues, but I think they're dependent on this fix. >> Either I can add to this PR or I can wait to see what comes out of this and >> fix them subsequently. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added more comments and fixed IdentityWrapper hashcode. Apologies for the delay. This looks good to me. Have no comments. please integrate. - PR Comment: https://git.openjdk.org/jfx/pull/1037#issuecomment-1610827994
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 10:04:48 GMT, Dean Wookey wrote: >> Each time a menu would change scenes, a new set of ListChangeListeners would >> be added to the items in the menu. The bigger problem however is that these >> list change listeners have a strong reference to the scene which is >> potentially a much bigger leak. >> >> The first commit was more straightforward, but there are 2 things that might >> be of concern: >> >> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but >> it'll remove all the ListChangeListeners attached to it, regardless of which >> scene was passed in. Something similar happens with changeListenerMap >> already, so I think it's fine. >> 2. I made the remove method public so that external calls from skins to >> remove the accelerators would remove the ListChangeListener and also because >> all the remove methods are public. >> >> After I wrote more tests I realised using the ObservableLists as keys in the >> WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would >> be simple. The fix is in the second commit. >> >> There are still more issues that stem from the fact that for each anchor >> there could be multiple menus and the current code doesn't account for that. >> For example, swapping context menus on a tab doesn't register change >> listeners on the new context menu because the TabPane itself had a scene >> change listener already. These other issues could relate to JDK-8268374 >> https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 >> https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to >> the fact that there's no logic to remove listeners when Tab/TableColumn's >> are removed from their associated control (TabPane, TableView, >> TreeTableView). >> >> I'm looking at these issues, but I think they're dependent on this fix. >> Either I can add to this PR or I can wait to see what comes out of this and >> fix them subsequently. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added more comments and fixed IdentityWrapper hashcode. @arapte I believe that you still wanted take a look at this. Can you please confirm? - PR Comment: https://git.openjdk.org/jfx/pull/1037#issuecomment-1610081146
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 10:04:48 GMT, Dean Wookey wrote: >> Each time a menu would change scenes, a new set of ListChangeListeners would >> be added to the items in the menu. The bigger problem however is that these >> list change listeners have a strong reference to the scene which is >> potentially a much bigger leak. >> >> The first commit was more straightforward, but there are 2 things that might >> be of concern: >> >> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but >> it'll remove all the ListChangeListeners attached to it, regardless of which >> scene was passed in. Something similar happens with changeListenerMap >> already, so I think it's fine. >> 2. I made the remove method public so that external calls from skins to >> remove the accelerators would remove the ListChangeListener and also because >> all the remove methods are public. >> >> After I wrote more tests I realised using the ObservableLists as keys in the >> WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would >> be simple. The fix is in the second commit. >> >> There are still more issues that stem from the fact that for each anchor >> there could be multiple menus and the current code doesn't account for that. >> For example, swapping context menus on a tab doesn't register change >> listeners on the new context menu because the TabPane itself had a scene >> change listener already. These other issues could relate to JDK-8268374 >> https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 >> https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to >> the fact that there's no logic to remove listeners when Tab/TableColumn's >> are removed from their associated control (TabPane, TableView, >> TreeTableView). >> >> I'm looking at these issues, but I think they're dependent on this fix. >> Either I can add to this PR or I can wait to see what comes out of this and >> fix them subsequently. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added more comments and fixed IdentityWrapper hashcode. is this ready to go? i think it's waiting for slash-integrate command... - PR Comment: https://git.openjdk.org/jfx/pull/1037#issuecomment-1610039298
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 10:04:48 GMT, Dean Wookey wrote: >> Each time a menu would change scenes, a new set of ListChangeListeners would >> be added to the items in the menu. The bigger problem however is that these >> list change listeners have a strong reference to the scene which is >> potentially a much bigger leak. >> >> The first commit was more straightforward, but there are 2 things that might >> be of concern: >> >> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but >> it'll remove all the ListChangeListeners attached to it, regardless of which >> scene was passed in. Something similar happens with changeListenerMap >> already, so I think it's fine. >> 2. I made the remove method public so that external calls from skins to >> remove the accelerators would remove the ListChangeListener and also because >> all the remove methods are public. >> >> After I wrote more tests I realised using the ObservableLists as keys in the >> WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would >> be simple. The fix is in the second commit. >> >> There are still more issues that stem from the fact that for each anchor >> there could be multiple menus and the current code doesn't account for that. >> For example, swapping context menus on a tab doesn't register change >> listeners on the new context menu because the TabPane itself had a scene >> change listener already. These other issues could relate to JDK-8268374 >> https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 >> https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to >> the fact that there's no logic to remove listeners when Tab/TableColumn's >> are removed from their associated control (TabPane, TableView, >> TreeTableView). >> >> I'm looking at these issues, but I think they're dependent on this fix. >> Either I can add to this PR or I can wait to see what comes out of this and >> fix them subsequently. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added more comments and fixed IdentityWrapper hashcode. `ControlAcceleratorSupport` is a strange beast. There's tons of manual book-keeping, global static maps (unsynchronized, which may or may not be a problem), weak references, all of that basically to do just one thing: add an accelerator to a `Scene`, and remove it after it's no longer needed. I wonder if this really is the best we can do here. (Maybe it is.) Approving the fix, but I suggest that we investigate if there's a simpler way to achieve the same thing. - Marked as reviewed by mstrauss (Committer). PR Review: https://git.openjdk.org/jfx/pull/1037#pullrequestreview-1492275398
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 10:04:48 GMT, Dean Wookey wrote: >> Each time a menu would change scenes, a new set of ListChangeListeners would >> be added to the items in the menu. The bigger problem however is that these >> list change listeners have a strong reference to the scene which is >> potentially a much bigger leak. >> >> The first commit was more straightforward, but there are 2 things that might >> be of concern: >> >> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but >> it'll remove all the ListChangeListeners attached to it, regardless of which >> scene was passed in. Something similar happens with changeListenerMap >> already, so I think it's fine. >> 2. I made the remove method public so that external calls from skins to >> remove the accelerators would remove the ListChangeListener and also because >> all the remove methods are public. >> >> After I wrote more tests I realised using the ObservableLists as keys in the >> WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would >> be simple. The fix is in the second commit. >> >> There are still more issues that stem from the fact that for each anchor >> there could be multiple menus and the current code doesn't account for that. >> For example, swapping context menus on a tab doesn't register change >> listeners on the new context menu because the TabPane itself had a scene >> change listener already. These other issues could relate to JDK-8268374 >> https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 >> https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to >> the fact that there's no logic to remove listeners when Tab/TableColumn's >> are removed from their associated control (TabPane, TableView, >> TreeTableView). >> >> I'm looking at these issues, but I think they're dependent on this fix. >> Either I can add to this PR or I can wait to see what comes out of this and >> fix them subsequently. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added more comments and fixed IdentityWrapper hashcode. Ideally, @arapte would be the best reviewer, since he has also fixed memory leaks in this area. If he is not able to look at this in the next few days, then perhaps @aghaisas can? - PR Comment: https://git.openjdk.org/jfx/pull/1037#issuecomment-1593501883
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Wed, 1 Mar 2023 13:36:46 GMT, Kevin Rushforth wrote: >> Dean Wookey has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added more comments and fixed IdentityWrapper hashcode. > > @arapte can you be the second reviewer? @kevinrushforth are there any other reviewers who could look at this? - PR Comment: https://git.openjdk.org/jfx/pull/1037#issuecomment-1592484555
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 10:04:48 GMT, Dean Wookey wrote: >> Each time a menu would change scenes, a new set of ListChangeListeners would >> be added to the items in the menu. The bigger problem however is that these >> list change listeners have a strong reference to the scene which is >> potentially a much bigger leak. >> >> The first commit was more straightforward, but there are 2 things that might >> be of concern: >> >> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but >> it'll remove all the ListChangeListeners attached to it, regardless of which >> scene was passed in. Something similar happens with changeListenerMap >> already, so I think it's fine. >> 2. I made the remove method public so that external calls from skins to >> remove the accelerators would remove the ListChangeListener and also because >> all the remove methods are public. >> >> After I wrote more tests I realised using the ObservableLists as keys in the >> WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would >> be simple. The fix is in the second commit. >> >> There are still more issues that stem from the fact that for each anchor >> there could be multiple menus and the current code doesn't account for that. >> For example, swapping context menus on a tab doesn't register change >> listeners on the new context menu because the TabPane itself had a scene >> change listener already. These other issues could relate to JDK-8268374 >> https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 >> https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to >> the fact that there's no logic to remove listeners when Tab/TableColumn's >> are removed from their associated control (TabPane, TableView, >> TreeTableView). >> >> I'm looking at these issues, but I think they're dependent on this fix. >> Either I can add to this PR or I can wait to see what comes out of this and >> fix them subsequently. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added more comments and fixed IdentityWrapper hashcode. This fix is quite complicated for sure. This memory leak currently affects us, so we're using an internal build. We would like to see a fix for this get merged in, and there's also a follow-up PR I'd like to do as well. @arapte If the complexity is the issue, we are open to any suggestions for alternative (simpler) fixes. - PR Comment: https://git.openjdk.org/jfx/pull/1037#issuecomment-1531251481
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 10:04:48 GMT, Dean Wookey wrote: >> Each time a menu would change scenes, a new set of ListChangeListeners would >> be added to the items in the menu. The bigger problem however is that these >> list change listeners have a strong reference to the scene which is >> potentially a much bigger leak. >> >> The first commit was more straightforward, but there are 2 things that might >> be of concern: >> >> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but >> it'll remove all the ListChangeListeners attached to it, regardless of which >> scene was passed in. Something similar happens with changeListenerMap >> already, so I think it's fine. >> 2. I made the remove method public so that external calls from skins to >> remove the accelerators would remove the ListChangeListener and also because >> all the remove methods are public. >> >> After I wrote more tests I realised using the ObservableLists as keys in the >> WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would >> be simple. The fix is in the second commit. >> >> There are still more issues that stem from the fact that for each anchor >> there could be multiple menus and the current code doesn't account for that. >> For example, swapping context menus on a tab doesn't register change >> listeners on the new context menu because the TabPane itself had a scene >> change listener already. These other issues could relate to JDK-8268374 >> https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 >> https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to >> the fact that there's no logic to remove listeners when Tab/TableColumn's >> are removed from their associated control (TabPane, TableView, >> TreeTableView). >> >> I'm looking at these issues, but I think they're dependent on this fix. >> Either I can add to this PR or I can wait to see what comes out of this and >> fix them subsequently. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added more comments and fixed IdentityWrapper hashcode. Just bumping this to end it for another 4 weeks. @arapte - PR Comment: https://git.openjdk.org/jfx/pull/1037#issuecomment-1495689005
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 10:04:48 GMT, Dean Wookey wrote: >> Each time a menu would change scenes, a new set of ListChangeListeners would >> be added to the items in the menu. The bigger problem however is that these >> list change listeners have a strong reference to the scene which is >> potentially a much bigger leak. >> >> The first commit was more straightforward, but there are 2 things that might >> be of concern: >> >> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but >> it'll remove all the ListChangeListeners attached to it, regardless of which >> scene was passed in. Something similar happens with changeListenerMap >> already, so I think it's fine. >> 2. I made the remove method public so that external calls from skins to >> remove the accelerators would remove the ListChangeListener and also because >> all the remove methods are public. >> >> After I wrote more tests I realised using the ObservableLists as keys in the >> WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would >> be simple. The fix is in the second commit. >> >> There are still more issues that stem from the fact that for each anchor >> there could be multiple menus and the current code doesn't account for that. >> For example, swapping context menus on a tab doesn't register change >> listeners on the new context menu because the TabPane itself had a scene >> change listener already. These other issues could relate to JDK-8268374 >> https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 >> https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to >> the fact that there's no logic to remove listeners when Tab/TableColumn's >> are removed from their associated control (TabPane, TableView, >> TreeTableView). >> >> I'm looking at these issues, but I think they're dependent on this fix. >> Either I can add to this PR or I can wait to see what comes out of this and >> fix them subsequently. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added more comments and fixed IdentityWrapper hashcode. @arapte can you be the second reviewer? - PR: https://git.openjdk.org/jfx/pull/1037
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 19:02:25 GMT, Andy Goryachev wrote: >> In general should we use new syntax like that instanceof syntax? JavaFX >> requires Java 17 now, but if changes were to be backported then using the >> new syntax makes it a bit more difficult. > > that will be "SEP" (someone else's problem) ;-) > > fx requires jdk17 as a boot jdk per JDK-8276144 Right. For newly added or modified code, it's fine to use JDK 17 language constructs and API, unless you know that the code will be backported right away. In particular, we have been using pattern-matching `instanceof` in other places. - PR: https://git.openjdk.org/jfx/pull/1037
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 19:09:03 GMT, Andy Goryachev wrote: >> I've been working on changes for a possible follow-up PR which address more >> bugs. For adding accelerators, there are 3 public methods, for removing >> there are currently 4. >> >> I've looked at it, and it can/should be made to have 3 public remove methods >> that exactly mirror the public add methods (if you use a specific one to add >> then use the corresponding one to remove). >> >> Further, for every private addAcceleratorsFromScene method and >> doAcceleratorInstall method I believe there should be a private >> corresponding method which reverses it. I had to rename a couple private >> removeAcceleratorsFromScene to doAcceleratorUninstall. >> >> So yes, this is a very confusing class. Pairing up the add/remove methods >> make sense to me. Once that's done, we might want to rename some of the >> private ones just so it's easier to understand what each one is doing, but >> the public ones are fine I think. > > I just want to avoid confusion when we have public and non-public methods > with the same name. but it's a minor comment, especially if there will be > subsequent rework later. We use this pattern of having methods with the same name and overloaded args, some of which are not public, in many places. In general this seems fine. In specific cases where it might help alleviate confusion, using "Impl" as a suffix is OK (and better than "Private" since we might make some impl methods package scope). - PR: https://git.openjdk.org/jfx/pull/1037
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Thu, 23 Feb 2023 09:52:08 GMT, Dean Wookey wrote: >> I'm not in favor of using `Private` in a method name. That is clear from the >> method signature and overloading methods is a valid choice In my opinion, >> this is fine as is. >> But we could also think about naming it: `removeAcceleratorsFromSceneImpl()` > > I've been working on changes for a possible follow-up PR which address more > bugs. For adding accelerators, there are 3 public methods, for removing there > are currently 4. > > I've looked at it, and it can/should be made to have 3 public remove methods > that exactly mirror the public add methods (if you use a specific one to add > then use the corresponding one to remove). > > Further, for every private addAcceleratorsFromScene method and > doAcceleratorInstall method I believe there should be a private corresponding > method which reverses it. I had to rename a couple private > removeAcceleratorsFromScene to doAcceleratorUninstall. > > So yes, this is a very confusing class. Pairing up the add/remove methods > make sense to me. Once that's done, we might want to rename some of the > private ones just so it's easier to understand what each one is doing, but > the public ones are fine I think. I just want to avoid confusion when we have public and non-public methods with the same name. but it's a minor comment, especially if there will be subsequent rework later. - PR: https://git.openjdk.org/jfx/pull/1037
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 10:04:48 GMT, Dean Wookey wrote: >> Each time a menu would change scenes, a new set of ListChangeListeners would >> be added to the items in the menu. The bigger problem however is that these >> list change listeners have a strong reference to the scene which is >> potentially a much bigger leak. >> >> The first commit was more straightforward, but there are 2 things that might >> be of concern: >> >> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but >> it'll remove all the ListChangeListeners attached to it, regardless of which >> scene was passed in. Something similar happens with changeListenerMap >> already, so I think it's fine. >> 2. I made the remove method public so that external calls from skins to >> remove the accelerators would remove the ListChangeListener and also because >> all the remove methods are public. >> >> After I wrote more tests I realised using the ObservableLists as keys in the >> WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would >> be simple. The fix is in the second commit. >> >> There are still more issues that stem from the fact that for each anchor >> there could be multiple menus and the current code doesn't account for that. >> For example, swapping context menus on a tab doesn't register change >> listeners on the new context menu because the TabPane itself had a scene >> change listener already. These other issues could relate to JDK-8268374 >> https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 >> https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to >> the fact that there's no logic to remove listeners when Tab/TableColumn's >> are removed from their associated control (TabPane, TableView, >> TreeTableView). >> >> I'm looking at these issues, but I think they're dependent on this fix. >> Either I can add to this PR or I can wait to see what comes out of this and >> fix them subsequently. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added more comments and fixed IdentityWrapper hashcode. I wish there was a simpler way to achieve the same goal, but this LGTM. - Marked as reviewed by angorya (Committer). PR: https://git.openjdk.org/jfx/pull/1037
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 09:49:33 GMT, Dean Wookey wrote:
>> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
>> line 366:
>>
>>> 364: public boolean equals(Object o) {
>>> 365: if (this == o) return true;
>>> 366: if (o == null || !(o instanceof
>>> IdentityWrapperListChangeListener)) return false;
>>
>> (o instanceof IdentityWrapperListChangeListener) should take care of the
>> null case.
>> if(o instanceof IdentityWrapperListChangeListener that) {
>> return innerList == that.innerList;
>> }
>> return false;
>>
>> And also, could we have curly braces in all the if statements please?
>
> In general should we use new syntax like that instanceof syntax? JavaFX
> requires Java 17 now, but if changes were to be backported then using the new
> syntax makes it a bit more difficult.
that will be "SEP" (someone else's problem) ;-)
fx requires jdk17 as a boot jdk per JDK-8276144
-
PR: https://git.openjdk.org/jfx/pull/1037
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
On Fri, 24 Feb 2023 10:04:48 GMT, Dean Wookey wrote: >> Each time a menu would change scenes, a new set of ListChangeListeners would >> be added to the items in the menu. The bigger problem however is that these >> list change listeners have a strong reference to the scene which is >> potentially a much bigger leak. >> >> The first commit was more straightforward, but there are 2 things that might >> be of concern: >> >> 1. The method removeAcceleratorsFromScene takes in a scene parameter, but >> it'll remove all the ListChangeListeners attached to it, regardless of which >> scene was passed in. Something similar happens with changeListenerMap >> already, so I think it's fine. >> 2. I made the remove method public so that external calls from skins to >> remove the accelerators would remove the ListChangeListener and also because >> all the remove methods are public. >> >> After I wrote more tests I realised using the ObservableLists as keys in the >> WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would >> be simple. The fix is in the second commit. >> >> There are still more issues that stem from the fact that for each anchor >> there could be multiple menus and the current code doesn't account for that. >> For example, swapping context menus on a tab doesn't register change >> listeners on the new context menu because the TabPane itself had a scene >> change listener already. These other issues could relate to JDK-8268374 >> https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 >> https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to >> the fact that there's no logic to remove listeners when Tab/TableColumn's >> are removed from their associated control (TabPane, TableView, >> TreeTableView). >> >> I'm looking at these issues, but I think they're dependent on this fix. >> Either I can add to this PR or I can wait to see what comes out of this and >> fix them subsequently. > > Dean Wookey has updated the pull request incrementally with one additional > commit since the last revision: > > Added more comments and fixed IdentityWrapper hashcode. This is what I'd like to do to fix JDK-8268374 and JDK-8283449 as well as an unreported bug where multiple tabs with context menus wouldn't register/remove accelerators properly. It also refactors the methods so that there are 3 public add methods and 3 public remove methods, and for every private add/install method there is a private remove/uninstall method that reverses it. I'm not really sure of the best way of tackling this. Whether to try break to separate the fixes for each of those bugs into their own PR, or do it in one go. https://github.com/openjdk/jfx/commit/25d98afc5869d013eed830ed7c370d06bd495056 - PR: https://git.openjdk.org/jfx/pull/1037
Re: RFR: 8283551: ControlAcceleratorSupport menu items listener causes memory leak [v2]
> Each time a menu would change scenes, a new set of ListChangeListeners would > be added to the items in the menu. The bigger problem however is that these > list change listeners have a strong reference to the scene which is > potentially a much bigger leak. > > The first commit was more straightforward, but there are 2 things that might > be of concern: > > 1. The method removeAcceleratorsFromScene takes in a scene parameter, but > it'll remove all the ListChangeListeners attached to it, regardless of which > scene was passed in. Something similar happens with changeListenerMap > already, so I think it's fine. > 2. I made the remove method public so that external calls from skins to > remove the accelerators would remove the ListChangeListener and also because > all the remove methods are public. > > After I wrote more tests I realised using the ObservableLists as keys in the > WeakHashMaps was a bad idea. If Java had a WeakIdentityHashMap the fix would > be simple. The fix is in the second commit. > > There are still more issues that stem from the fact that for each anchor > there could be multiple menus and the current code doesn't account for that. > For example, swapping context menus on a tab doesn't register change > listeners on the new context menu because the TabPane itself had a scene > change listener already. These other issues could relate to JDK-8268374 > https://bugs.openjdk.org/browse/JDK-8268374 and JDK-8283449 > https://bugs.openjdk.org/browse/JDK-8283449. One of the issues is related to > the fact that there's no logic to remove listeners when Tab/TableColumn's are > removed from their associated control (TabPane, TableView, TreeTableView). > > I'm looking at these issues, but I think they're dependent on this fix. > Either I can add to this PR or I can wait to see what comes out of this and > fix them subsequently. Dean Wookey has updated the pull request incrementally with one additional commit since the last revision: Added more comments and fixed IdentityWrapper hashcode. - Changes: - all: https://git.openjdk.org/jfx/pull/1037/files - new: https://git.openjdk.org/jfx/pull/1037/files/585534e5..856edac6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1037=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1037=00-01 Stats: 107 lines in 2 files changed: 87 ins; 4 del; 16 mod Patch: https://git.openjdk.org/jfx/pull/1037.diff Fetch: git fetch https://git.openjdk.org/jfx pull/1037/head:pull/1037 PR: https://git.openjdk.org/jfx/pull/1037
