8348100: Tooltips cannot be instantiated on background thread #1696
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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
|
|
|
/reviewers 2 |
|
@andy-goryachev-oracle |
There was a problem hiding this comment.
I think the root cause of the problem is in Tooltip::getStyleableParent, which does not return a stable result when called from a background thread. The implementation does this:
if (BEHAVIOR.hoveredNode == null) {
return super.getStyleableParent();
}
return BEHAVIOR.hoveredNode;
Where BEHAVIOR is a static field of Tooltip.
This might be a better fix:
@Override public Styleable getStyleableParent() {
if (!Platform.isFxApplicationThread() || BEHAVIOR.hoveredNode == null) {
return super.getStyleableParent();
}
return BEHAVIOR.hoveredNode;
}
and then you don't even need the null checks you added to CssStyleHelper.
| if (parent == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
While this does prevent the exception, I think this is fixing a symptom rather than the root cause. The only reason that parent can ever be null here and in the other place you added the null check is that Tooltip::getStyleableParent is returning a different answer between the time it is called to calculate depth and the time it is used here in a loop. This is happening because Tooltip::getStyleableParent uses static state that is only valid when on the FX application thread.
You are right, this is the root cause and a better fix! (What was I thinking? The ticket points to it directly in the description). Fun fact: one can add junit5 annotation |
| import com.sun.javafx.beans.IDProperty; | ||
| import com.sun.javafx.css.StyleManager; | ||
| import com.sun.javafx.scene.NodeHelper; | ||
| import com.sun.javafx.stage.PopupWindowHelper; |
There was a problem hiding this comment.
That's a lot of import noise for such a small patch...
|
/integrate |
|
Going to push as commit 3ce3af0. |
|
@andy-goryachev-oracle Pushed as commit 3ce3af0. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Root Cause
navigating up the parent hierarchy during CSS processing in a background thread encounters a null parent, causing the NPE. The null parent comes from a static Toolkit.BEHAVIOR field.
Solution
avoid that code path if not in the fx application thread.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1696/head:pull/1696$ git checkout pull/1696Update a local copy of the PR:
$ git checkout pull/1696$ git pull https://git.openjdk.org/jfx.git pull/1696/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1696View PR using the GUI difftool:
$ git pr show -t 1696Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1696.diff
Using Webrev
Link to Webrev Comment