8350976: MenuBarSkin: exception initializing in a background thread #1727

andy-goryachev-oracle · 2025-03-03T23:50:07Z

Allows MenuBar to be created in a background thread by delaying MenuBarSkin::rebuildUI() call until after the MenuBar becomes a part of the scene graph.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8350976: MenuBarSkin: exception initializing in a background thread (Bug - P4)

Reviewers

Lukasz Kostyra (@lukostyra - Reviewer)
Jayathirth D V (@jayathirthrao - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1727/head:pull/1727
$ git checkout pull/1727

Update a local copy of the PR:
$ git checkout pull/1727
$ git pull https://git.openjdk.org/jfx.git pull/1727/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1727

View PR using the GUI difftool:
$ git pr show -t 1727

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1727.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-03T23:50:44Z

👋 Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-03-03T23:51:37Z

@andy-goryachev-oracle This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8350976: MenuBarSkin: exception initializing in a background thread

Reviewed-by: lkostyra, jdv

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 18 new commits pushed to the master branch:

c452189: 8351733: Crash when creating too many nested event loops
cc949cd: 8351047: TitledPane should handle titles that are resizable
5c78234: 8281384: Random chars on paste from Windows clipboard
ff777c7: 8340004: [TestBug] Call ModuleLayer.Controller::enableNativeAccess directly rather than via reflection
9143377: 8342565: [TestBug] StubTextLayout
a550e5e: 8350149: VBox ignores bias of child controls when fillWidth is set to false
fd099a7: 8347937: Canvas pattern test fails and crashes on WebKit 620.1
0eda4df: 8352268: RichEditorDemo: Save file doesn't always save
0e50961: 8351038: ConcurrentModificationException in EventType constructor
f5bdec5: 8349373: Support JavaFX preview features
... and 8 more: https://git.openjdk.org/jfx/compare/fc770fb96bdd6804c4b7633a62cbb2fe0a50345f...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

andy-goryachev-oracle · 2025-03-04T16:12:15Z

/reviewers 2

openjdk · 2025-03-04T16:13:26Z

@andy-goryachev-oracle
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

mlbridge · 2025-03-04T16:16:21Z

Webrevs

01: Full - Incremental (69a52e64)
00: Full (97aafcb9)

….thread.safety

mstr2 · 2025-03-13T04:00:42Z

modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuBarSkin.java

        ListenerHelper lh = ListenerHelper.get(this);

+        if (Platform.isFxApplicationThread()) {
+            if (Toolkit.getToolkit().getSystemMenu().isSupported()) {


You could move this check to the outer scope, because if it evalutes to false, we can skip both branches of if (Platform.isFxApplicationThread()) { completely.

not sure what you mean exactly.
Toolkit.getToolkit().getSystemMenu().isSupported() must be called in the fx application thread, and the rest of the changes were done to minimize the structural changes.

lukostyra

Looks good, verified on Windows 11 - test fails without fix and works with it.

jayathirthrao · 2025-03-28T09:21:56Z

menuBar test passes even without the fix in macOS.
Is this expected behaviour? Product change looks common and not specific to any platform.

andy-goryachev-oracle · 2025-03-28T14:30:17Z

menuBar test passes even without the fix in macOS.

That's strange: for me it fails in the current master branch (the line number is different bc I just appended the test to the end of file). macOS 15.3.2.

java.lang.IllegalStateException: This operation is permitted on the event thread only; currentThread = MenuBar
	at javafx.graphics/com.sun.glass.ui.Application.checkEventThread(Application.java:440)
	at javafx.graphics/com.sun.glass.ui.Application.supportsSystemMenu(Application.java:719)
	at javafx.graphics/com.sun.javafx.tk.quantum.GlassSystemMenu.isSupported(GlassSystemMenu.java:91)
	at javafx.controls/javafx.scene.control.skin.MenuBarSkin.rebuildUI(MenuBarSkin.java:822)
	at javafx.controls/javafx.scene.control.skin.MenuBarSkin.<init>(MenuBarSkin.java:232)

jayathirthrao · 2025-04-01T14:36:39Z

Previously i had commit until march 26 so again i synced the code to latest master branch build it and then took the changes from this PR(git fetch https://git.openjdk.org/jfx.git pull/1727/head:pull/1727 & git checkout pull/1727) and ran the test and still passes on my macOS 14.7.4 macbook pro laptop.

In the test results also i can see menuBar test as passed.

andy-goryachev-oracle · 2025-04-01T15:05:40Z

@jayathirthrao

Are you saying that the test taken from this PR passes when added to the current master branch?

If so, I wonder if the laptop/os version might be relevant: I am testing with macOS 15.3.2 on M1 silicon, and the test from this PR fails with the master branch. (I also do a clean build before running the test). Can you try running on a different machine (maybe Windows?)

java.lang.IllegalStateException: This operation is permitted on the event thread only; currentThread = MenuBar
	at javafx.graphics/com.sun.glass.ui.Application.checkEventThread(Application.java:440)
	at javafx.graphics/com.sun.glass.ui.Application.supportsSystemMenu(Application.java:719)
	at javafx.graphics/com.sun.javafx.tk.quantum.GlassSystemMenu.isSupported(GlassSystemMenu.java:91)
	at javafx.controls/javafx.scene.control.skin.MenuBarSkin.rebuildUI(MenuBarSkin.java:822)
	at javafx.controls/javafx.scene.control.skin.MenuBarSkin.<init>(MenuBarSkin.java:232)
	at test.robot.javafx.scene.NodeInitializationStressTest.lambda$73(NodeInitializationStressTest.java:755)
	at test.robot.javafx.scene.NodeInitializationStressTest.lambda$169(NodeInitializationStressTest.java:1503)
	at java.base/java.lang.Thread.run(Thread.java:1447)

jayathirthrao · 2025-04-01T17:00:12Z

Yes updated NodeInitializationStressTest with master branch code passes on my M1 macOS 14.7.4 Macbook pro laptop. I again checked with clean build.

Also since i am working Ubuntu 24.04 issues, i tested with clean master branch build and updated test. Here also i see that the menuBar test passes with master branch build. I have little order setup on my Windows with older toolchain, once i make it work on latest code i will share my findings.

jayathirthrao · 2025-04-01T17:04:37Z

More info i am running this test using below command:
gradle --continue --info -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:cleanTest :systemTests:test --tests test.robot.javafx.scene.NodeInitializationStressTest

andy-goryachev-oracle · 2025-04-01T17:05:16Z

Do you have the glass.disableThreadChecks system property set?

kevinrushforth · 2025-04-01T17:47:21Z

I also have macOS 14 (14.7.4 to be exact). NodeInitializationStressTest.menuBar test fails consistently for me without this fix and passes with the fix.

@jayathirthrao Can you double-check that you were actually running the version of the test from this PR (without the fix)? The easiest way to do that, which will also save you a bunch of time, is to run just that one test method (although it fails for me if I run the whole thing, NodeInitializationStressTest takes nearly 10 minutes on my system).

gradle --continue --info -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:cleanTest :systemTests:test \
--tests test.robot.javafx.scene.NodeInitializationStressTest.menuBar

andy-goryachev-oracle · 2025-04-01T17:51:15Z

javafx.graphics/com.sun.glass.ui.Application.checkEventThread() throws an IllegalStateException since 2013, and probably earlier than that.

The only way this functionality is suppressed is when glass.disableThreadChecks system property is set to true.

kevinrushforth · 2025-04-01T18:02:43Z

The only way this functionality is suppressed is when glass.disableThreadChecks system property is set to true.

There is no easy way to set this, so I'm almost certain that isn't the case here. Jay can confirm, but I think there is a much easier explanation: I suspect that it didn't run the new test method in question. Thus my earlier comment above about running that test in isolation.

andy-goryachev-oracle · 2025-04-01T18:05:37Z

I ran his gradle command and it failed as expected. Maybe Jay ran it in some other directory?

jayathirthrao · 2025-04-02T06:06:58Z

For test verification i was following what i do in JDK of checking out the branch in the PR and without build verify that the test fails and then build the branch(which will build source code change also) and then make sure test passes.

Got to know that we need to use -PTEST_ONLY=true in FX to make sure that source code changes are not built when i use gradle to run the test. With this approach i can see that the test fails without product change and passes with it :)

andy-goryachev-oracle · 2025-04-02T14:31:02Z

The mystery solved! Thank you @jayathirthrao !
/integrate

openjdk · 2025-04-02T14:32:26Z

Going to push as commit 4a4272b.
Since your change was applied there have been 19 commits pushed to the master branch:

056c7f5: 8350009: [XWayland] SwingNodePlatformExitCrashTest hangs on Ubuntu 24.04
c452189: 8351733: Crash when creating too many nested event loops
cc949cd: 8351047: TitledPane should handle titles that are resizable
5c78234: 8281384: Random chars on paste from Windows clipboard
ff777c7: 8340004: [TestBug] Call ModuleLayer.Controller::enableNativeAccess directly rather than via reflection
9143377: 8342565: [TestBug] StubTextLayout
a550e5e: 8350149: VBox ignores bias of child controls when fillWidth is set to false
fd099a7: 8347937: Canvas pattern test fails and crashes on WebKit 620.1
0eda4df: 8352268: RichEditorDemo: Save file doesn't always save
0e50961: 8351038: ConcurrentModificationException in EventType constructor
... and 9 more: https://git.openjdk.org/jfx/compare/fc770fb96bdd6804c4b7633a62cbb2fe0a50345f...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-02T14:32:38Z

@andy-goryachev-oracle Pushed as commit 4a4272b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

andy-goryachev-oracle added 2 commits March 3, 2025 13:45

test

7e7a0fa

possible fix

98d9784

cleanup

7bc0816

andy-goryachev-oracle changed the title ~~8350976: MenuButtonSkin: exception initializing in a background thread~~ 8350976: MenuBarSkin: exception initializing in a background thread Mar 4, 2025

andy-goryachev-oracle added 2 commits March 4, 2025 07:57

use system menu

a59007e

spelling

97aafcb

andy-goryachev-oracle marked this pull request as ready for review March 4, 2025 16:12

openjdk bot added the rfr Ready for review label Mar 4, 2025

andy-goryachev-oracle mentioned this pull request Mar 4, 2025

8350048: Enforce threading restrictions for show and hide methods in Window, Control, and Skin #1717

Closed

4 tasks

Merge remote-tracking branch 'origin/master' into 8350976.menubarskin…

69a52e6

….thread.safety

mstr2 reviewed Mar 13, 2025

View reviewed changes

lukostyra approved these changes Mar 24, 2025

View reviewed changes

andy-goryachev-oracle requested a review from mstr2 March 24, 2025 19:09

jayathirthrao approved these changes Apr 2, 2025

View reviewed changes

openjdk bot added the ready Ready to be integrated label Apr 2, 2025

openjdk bot added the integrated Pull request has been integrated label Apr 2, 2025

openjdk bot closed this Apr 2, 2025

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Apr 2, 2025

andy-goryachev-oracle deleted the 8350976.menubarskin.thread.safety branch April 2, 2025 14:39

8350976: MenuBarSkin: exception initializing in a background thread #1727

8350976: MenuBarSkin: exception initializing in a background thread #1727

Uh oh!

Conversation

andy-goryachev-oracle commented Mar 3, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Mar 3, 2025

Uh oh!

openjdk bot commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andy-goryachev-oracle commented Mar 4, 2025

Uh oh!

openjdk bot commented Mar 4, 2025

Uh oh!

mlbridge bot commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

mstr2 Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

andy-goryachev-oracle Mar 13, 2025

Choose a reason for hiding this comment

Uh oh!

lukostyra left a comment

Choose a reason for hiding this comment

Uh oh!

jayathirthrao commented Mar 28, 2025

Uh oh!

andy-goryachev-oracle commented Mar 28, 2025

Uh oh!

jayathirthrao commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andy-goryachev-oracle commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayathirthrao commented Apr 1, 2025

Uh oh!

jayathirthrao commented Apr 1, 2025

Uh oh!

andy-goryachev-oracle commented Apr 1, 2025

Uh oh!

kevinrushforth commented Apr 1, 2025

Uh oh!

andy-goryachev-oracle commented Apr 1, 2025

Uh oh!

kevinrushforth commented Apr 1, 2025

Uh oh!

andy-goryachev-oracle commented Apr 1, 2025

Uh oh!

jayathirthrao commented Apr 2, 2025

Uh oh!

andy-goryachev-oracle commented Apr 2, 2025

Uh oh!

openjdk bot commented Apr 2, 2025

Uh oh!

openjdk bot commented Apr 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

andy-goryachev-oracle commented Mar 3, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 3, 2025 •

edited

Loading

mlbridge bot commented Mar 4, 2025 •

edited

Loading

jayathirthrao commented Apr 1, 2025 •

edited

Loading

andy-goryachev-oracle commented Apr 1, 2025 •

edited

Loading