RFR: 8349091: Charts: exception initializing in a background thread [v4]

[Kevin Rushforth]

On Thu, 20 Feb 2025 23:04:15 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Root Cause:
>> (Multiple) properties are getting bound to the global `Platform.accessibilityActive` property. Binding (and I say, accessing) of properties is not thread-safe.
>> 
>> I also changed the design a bit.  Originally, every symbol in a chart had its `focusTraversableProperty` bound to `Platform.accessibilityActive` in order to enable the accessibility navigation across the chart data points.  This is rather inefficient, as the property has to manage (thousands?) of listeners.
>> 
>> Instead, a single boolean property is added to each chart, with a listener added to it which iterates over data symbols to toggle the `focusTraversableProperty` directly.
>> 
>> The exact moment when the new property gets bound is also important, and has to happen in the FX application thread.
>> 
>> With this change, it is safe to create and populate charts with data in a background thread.
>> 
>> ---
>> 
>> ## NOTES
>> 
>> 1. It looks like the `Platform.accessibilityActive` property never transitions back to false  after it transitioned to true.  Some say it is because there is no mechanism in the platform to get notified which cannot possibly be true.
>> 2. The javadoc for `Platform.accessibilityActiveProperty()` method stipulates that "_This method may be called from any thread_" which is patently not true as the current implementation is simply not thread-safe.
>> 
>> ## Note to the Reviewers
>> 
>> To avoid merge conflicts, the preferred order of integrations:
>> 
>> #1697 
>> #1713 
>> #1717
>
> Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 22 commits:
> 
>  - Merge remote-tracking branch 'origin/master' into 8349091.charts.thread.safety
>  - enabled pie chart test
>  - Merge branch 'master' into 8349091.charts.thread.safety
>  - Merge branch 'master' into 8349091.charts.thread.safety
>  - whitespace
>  - Merge remote-tracking branch 'origin/master' into 8349091.charts.thread.safety
>  - cleanup
>  - tests pass
>  - chart tests only
>  - more jitter
>  - ... and 12 more: https://git.openjdk.org/jfx/compare/307e3087...e60c027b

This is heading in the right direction, but has some bugs that will need to be fixed.

modules/javafx.controls/src/main/java/javafx/scene/chart/AreaChart.java line 546:

> 544:             symbol.setAccessibleRole(AccessibleRole.TEXT);
> 545:             symbol.setAccessibleRoleDescription("Point");
> 546:             symbol.setFocusTraversable(isAccessibilityActive());

It looks like you are mixing the setting up of the listeners with setting the focus traversable of this symbol to the current state of accessibilityActive, which will be `Platform.accessibilityActiveProperty` for the FX app thread) and "false" (for background thread. As a result, this method, which is called once per symbol, will set up a new listener each time it is called.

You might consider refactoring this a bit to only ever set up the listeners when the object is constructed. Either that or ensure that you guard against repeatedly recreating the listener.

modules/javafx.controls/src/main/java/javafx/scene/chart/Chart.java line 536:

> 534:      */
> 535:     // package protected: custom charts must handle accessbility on their own
> 536:     boolean isAccessibilityActive() {

This can be final, although as I mentioned in an earlier comment, it could probably use to be refactored.

modules/javafx.controls/src/main/java/javafx/scene/chart/Chart.java line 551:

> 549:             ChangeListener<Scene> li = new ChangeListener<>() {
> 550:                 @Override
> 551:                 public void changed(ObservableValue<? extends Scene> src, Scene prev, Scene scene) {

This is insufficient. It will miss the case where both the node and the scene are created in the background thread, and the scene is later added to a window on the FX app thread. I tested this case and verified my assertion. This also means that in some cases, if this listener is called from a background thread, you will create a new scene listener from the current scene listener.

Consider instead using `TreeShowingProperty`, which is used by `ProgressIndicatorSkin` to set up its animation.`TreeShowingProperty` sets up a listener chain that fires when the the "tree showing" status of a node changed (tree showing means the node and all ancestors are visible and is attached to a scene that is attached to a window that is showing.

Alternatively, since you might not care whether the node is visible, you can just set up a binding using `ObservableValue::flatMap`. The code snippet of the docs:


ObservableValue<Boolean> isShowing = sceneProperty()
    .flatMap(Scene::windowProperty)
    .flatMap(Window::showingProperty)
    .orElse(false);


And then you can then add a listener (or a value subscription might be better) on `isShowing` -- when it becomes true, you can setup the binding or listener to `Platform.accessibilityActiveProperty`.

With either of the above two solutions, the showing state will only become true on the FX app thread.

-------------

PR Review: https://git.openjdk.org/jfx/pull/1697#pullrequestreview-2634906931
PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1968581638
PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1968583768
PR Review Comment: https://git.openjdk.org/jfx/pull/1697#discussion_r1968591700