8349098: TabPane: exception initializing in a background thread #1699
Conversation
|
👋 Welcome back angorya! A progress list of the required criteria for merging this PR into |
|
@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: 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 6 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
|
Reviewer: @arapte /reviewers 2 |
|
@kevinrushforth |
| @@ -522,7 +520,7 @@ private void removeTabs(List<? extends Tab> removedList) { | |||
| } | |||
| }; | |||
|
|
|||
| if (closeTabAnimation.get() == TabAnimation.GROW) { | |||
| if (Platform.isFxApplicationThread() && (closeTabAnimation.get() == TabAnimation.GROW)) { | |||
There was a problem hiding this comment.
With this check, non Application thread cannot play the animation but in else block the cleanup is executed on the same thread.
Can there still be a situation when, this non-Application thread and Application thread be concurrently modifying the tab?
There was a problem hiding this comment.
If you mean two threads accessing this same TabPaneSkin instance, then that's not a valid case. JavaFX objects are not thread-safe when accessed from multiple threads. This bug (and the other related bugs fixed or under review) is about making sure that multiple threads, including the JavaFX application thread, each concurrently accessing their own instance, don't interfere with each other.
So it would only be a problem if "cleanup" attempted some animation or touched static state or similar, which is doesn't look like it does.
There was a problem hiding this comment.
Although, having said that, the purpose of the cleanup is to clean up after the animation. So a better fix might be to put the entire if-else inside an if (Platform.isFxApplicationThread()) test.
There was a problem hiding this comment.
Actually, no. It also calls requestLayout. I think the fix is correct as is.
There was a problem hiding this comment.
Right. The change prevents from starting the animation if these code paths are entered in a background thread. With the change, the code follows an animation-off path.
|
/integrate |
|
Going to push as commit d1f5ea8.
Your commit was automatically rebased without conflicts. |
|
@andy-goryachev-oracle Pushed as commit d1f5ea8. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Root Cause
Animation gets started in a background thread, which causes the animation handler to run in the FX application thread, thus creating simultaneous access to the control's fields (list of children in this case).
Solution
Skip the animation.
The fix is similar to #1698
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1699/head:pull/1699$ git checkout pull/1699Update a local copy of the PR:
$ git checkout pull/1699$ git pull https://git.openjdk.org/jfx.git pull/1699/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1699View PR using the GUI difftool:
$ git pr show -t 1699Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1699.diff
Using Webrev
Link to Webrev Comment