Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v16]
On Thu, 9 May 2024 19:48:19 GMT, Johan Vos wrote:
>> A listener was added but never removed.
>> This patch removes the listener when the menu it links to is cleared. Fix
>> for https://bugs.openjdk.org/browse/JDK-8319779
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add more type info
The new test passed with both the old and the new version (I reverted the code,
aside from leaving `setMenuBindings` `protected`). Is the test testing the
right thing?
(If relevant, this was done on a Windows platform)
tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
line 287:
> 285:
> 286: @Test
> 287: public void testJDK8309935() {
minor: Not sure what our policies are, but I find test names like this pretty
useless, perhaps a short indication what this is doing?
tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
line 346:
> 344: });
> 345: Util.runAndWait(() -> {
> 346: });
I checked the code that `runAndWait` would do when you pass in 0 runnables, and
it basically does no waiting at all (less than a microsecond for sure). If
this is really essential (I removed it and the test still passed) then I think
a `sleep` might be better.
-
Changes requested by jhendrikx (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1283#pullrequestreview-2114917800
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1637684395
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1637687030
Re: RFR: 8305418: [Linux] Replace obsolete XIM as Input Method Editor [v22]
On Sun, 9 Jun 2024 16:21:50 GMT, Thiago Milczarek Sayao wrote: > I did an updated build of this branch > [here](https://github.com/tsayao/jfx/releases/download/test-ime/jfx_23_linux_ime.zip) > (for testing only). Wow, thank you. I have downloaded it and will test it as soon as possible. - PR Comment: https://git.openjdk.org/jfx/pull/1080#issuecomment-2164875426
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v16]
On Thu, 9 May 2024 19:48:19 GMT, Johan Vos wrote:
>> A listener was added but never removed.
>> This patch removes the listener when the menu it links to is cleared. Fix
>> for https://bugs.openjdk.org/browse/JDK-8319779
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add more type info
I've tested on macOS, and tests pass now, and fail without the changes from
this PR.
I've left some comments, but once addressed, I'll re-approve.
modules/javafx.graphics/src/shims/java/com/sun/javafx/tk/quantum/GlassSystemMenuShim.java
line 37:
> 35:
> 36: private GlassSystemMenu gsm;
> 37: final ArrayList> uncollectedMenus = new
> ArrayList<>();
this can be private?
modules/javafx.graphics/src/shims/java/com/sun/javafx/tk/quantum/GlassSystemMenuShim.java
line 54:
> 52: protected void setMenuBindings(final Menu glassMenu, final MenuBase
> mb) {
> 53: super.setMenuBindings(glassMenu, mb);
> 54: uncollectedMenus.add(new WeakReference(glassMenu));
Add `<>` to avoid raw use of `WeakReference`
tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
line 289:
> 287: public void testJDK8309935() {
> 288: MenuBar menuBar = new MenuBar();
> 289: AtomicReference throwableRef = new AtomicReference();
Add missing `<>` to avoid raw use of `AtomicReference`
-
Marked as reviewed by jpereda (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1283#pullrequestreview-2115396776
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1637974242
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1637973453
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1637980139
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v16]
On Thu, 13 Jun 2024 07:21:47 GMT, John Hendrikx wrote: > The new test passed with both the old and the new version (I reverted the > code, aside from leaving `setMenuBindings` `protected`). Is the test testing > the right thing? > > (If relevant, this was done on a Windows platform) That's not surprising since only macOS provides the (optional) system menu capability, and the leak is in the glass system menu implementation. The tests will pass vacuously on other platforms, since the system menu code with the bug will never be reached. - PR Comment: https://git.openjdk.org/jfx/pull/1283#issuecomment-2165425472
Integrated: 8311895: CSS Transitions
On Tue, 16 Aug 2022 03:01:23 GMT, Michael Strauß wrote:
> Implementation of [CSS Transitions](https://www.w3.org/TR/css-transitions-1/).
>
> ### Future enhancements
> CSS transition support for backgrounds and borders: #1471
>
> ### Limitations
> This implementation supports both shorthand and longhand notations for the
> `transition` property. However, due to limitations of JavaFX CSS, mixing both
> notations doesn't work:
>
> .button {
> transition: -fx-background-color 1s;
> transition-easing-function: linear;
> }
>
> This issue should be addressed in a follow-up enhancement.
This pull request has now been integrated.
Changeset: 762f5902
Author:Michael Strauß
URL:
https://git.openjdk.org/jfx/commit/762f5902cdd2e6297996baf24f7a3e3ee8e26560
Stats: 4698 lines in 43 files changed: 4655 ins; 4 del; 39 mod
8311895: CSS Transitions
Reviewed-by: mmack, nlisker, kcr
-
PR: https://git.openjdk.org/jfx/pull/870
Re: 8289521: Publication to Apple App Store still not possible with JavaFX web 21
Yes, it sounds like the method cache_simulate_memory_warning_event was never public (at least I couldn’t find any documentation about it) and Apple started to enforce its removal. As far as I can tell, a user of JavaFX web cannot fix this and has currently no chance of publishing a (new?) app to Apples App Store. For a bit of context, this call is inside the method MemoryPressureHandler::platformReleaseMemory which is implemented differently on different platforms: - cocoa (Mac): with the discussed method - generic: does nothing (empty implementation) - unix: calls `malloc_trim(0);` - win: does nothing (empty implementation) The currently only solution is the one of Michael Hall to add a compiler directive similar to one which was there before 2016-12-14 and no-oped it for certain platforms/versions. I guess this idea would fix the problem of submitting it to Apples App Store. Does anyone have another idea or is willing to bring this idea forward? > On 7 Jun 2024, at 17:48, Michael Hall wrote: > > Sorry, I did a little more digging on this. > > My best guess is that Apple is rejecting this because they think you are > using a non-public API of their own. > > The OpenJFX version of MemoryPressureHandlerCocoa.mm matches what appears to > be the current WebKit version. [1] > > Older versions appear somewhat different. Including this… > > #if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 101000 > > [2][3] > > [3] seems to show where this code was actually introduced with the above > included. > > It might be that for a Cocoa version it was thought that this compiler > directive wasn’t needed but it actually still is? > Anyhow, adding something similar now might achieve a no-op that would get > past them flagging this as an error. > ‘locate’ on my machine doesn’t seem to show the libcache.dylib mentioned > anywhere other than Xcode and other developer locations. > > [1] > https://github.com/WebKit/webkit/blob/main/Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm > [2] > https://opensource.apple.com/source/WebCore/WebCore-7601.5.17/platform/cocoa/MemoryPressureHandlerCocoa.mm.auto.html > [3] > https://fossies.org/diffs/WebKit/r174650_vs_r189384/Source/WebCore/platform/cocoa/MemoryPressureHandlerCocoa.mm-diff.html > >> On Jun 6, 2024, at 6:41 PM, Michael Hall wrote: >> >> Odd, but searching shows this used in a different places and nowhere does it >> appear anyone has made any changes to the symbol referencing code. >> It seems like some applications should of used this same code as-is. >> Sort of strange. >> I’m done with what digging I think I’m going to do. >> >> GL. >
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v17]
> A listener was added but never removed. > This patch removes the listener when the menu it links to is cleared. Fix for > https://bugs.openjdk.org/browse/JDK-8319779 Johan Vos has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 18 commits: - Merge branch 'master' into 8319779-systemmenu - Add more type info - Add type info Fix issue where notifications are missed when a new property is used. - Merge branch 'master' into 8319779-systemmenu - Remove whiteline and language error - Ignore test in case the underlying GlassSystemMenu is not supported. - Update modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java Co-authored-by: John Hendrikx - Merge branch 'master' into 8319779-systemmenu - Add additional test for IOOBE detection. This test comes from JDK-8323787 - Revert some of the conditional bindings. Clear menu construction when an menuitem that is a menu needs to be removed Add a test for the latter - ... and 8 more: https://git.openjdk.org/jfx/compare/762f5902...5c733838 - Changes: https://git.openjdk.org/jfx/pull/1283/files Webrev: https://webrevs.openjdk.org/?repo=jfx&pr=1283&range=16 Stats: 663 lines in 5 files changed: 462 ins; 192 del; 9 mod Patch: https://git.openjdk.org/jfx/pull/1283.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1283/head:pull/1283 PR: https://git.openjdk.org/jfx/pull/1283
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v18]
> A listener was added but never removed. > This patch removes the listener when the menu it links to is cleared. Fix for > https://bugs.openjdk.org/browse/JDK-8319779 Johan Vos has updated the pull request incrementally with two additional commits since the last revision: - process more reviewer comments - Process reviewer comments - Changes: - all: https://git.openjdk.org/jfx/pull/1283/files - new: https://git.openjdk.org/jfx/pull/1283/files/5c733838..328ad6db Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx&pr=1283&range=17 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1283&range=16-17 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jfx/pull/1283.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1283/head:pull/1283 PR: https://git.openjdk.org/jfx/pull/1283
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v2]
On Tue, 12 Dec 2023 07:05:49 GMT, Ambarish Rapte wrote: >> Johan Vos has updated the pull request incrementally with one additional >> commit since the last revision: >> >> process reviewers comments > > Looks all good to me. > Is it possible to add an automated test ? @arapte if possible, can you re-review this? - PR Comment: https://git.openjdk.org/jfx/pull/1283#issuecomment-2166671943
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v16]
On Thu, 13 Jun 2024 10:25:15 GMT, Jose Pereda wrote:
>> Johan Vos has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add more type info
>
> modules/javafx.graphics/src/shims/java/com/sun/javafx/tk/quantum/GlassSystemMenuShim.java
> line 37:
>
>> 35:
>> 36: private GlassSystemMenu gsm;
>> 37: final ArrayList> uncollectedMenus = new
>> ArrayList<>();
>
> this can be private?
done
> modules/javafx.graphics/src/shims/java/com/sun/javafx/tk/quantum/GlassSystemMenuShim.java
> line 54:
>
>> 52: protected void setMenuBindings(final Menu glassMenu, final MenuBase
>> mb) {
>> 53: super.setMenuBindings(glassMenu, mb);
>> 54: uncollectedMenus.add(new WeakReference(glassMenu));
>
> Add `<>` to avoid raw use of `WeakReference`
done
> tests/system/src/test/java/test/com/sun/javafx/tk/quantum/SystemMenuBarTest.java
> line 289:
>
>> 287: public void testJDK8309935() {
>> 288: MenuBar menuBar = new MenuBar();
>> 289: AtomicReference throwableRef = new AtomicReference();
>
> Add missing `<>` to avoid raw use of `AtomicReference`
done
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1638809325
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1638809211
PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1638809871
Re: RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v18]
On Thu, 13 Jun 2024 20:07:32 GMT, Johan Vos wrote: >> A listener was added but never removed. >> This patch removes the listener when the menu it links to is cleared. Fix >> for https://bugs.openjdk.org/browse/JDK-8319779 > > Johan Vos has updated the pull request incrementally with two additional > commits since the last revision: > > - process more reviewer comments > - Process reviewer comments Marked as reviewed by jpereda (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1283#pullrequestreview-2117171377
