RFR: 8350048: Enforce threading restrictions for show and hide methods in Window, Control, and Skin [v3]

[Kevin Rushforth]

On Thu, 27 Feb 2025 15:53:34 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

> > For example, I recommend checking the following:
> > MenuBarSkin line 799
> > ComboBoxListViewSkin line 199
> 
> I think it should be sufficient to mention the exception in the actual methods where the check is done, such as `hide()` and `show()`.

I'm not talking about docs here. What I meant that if the code in question is ever executed on a background thread such that hide is called, this will now cause an exception at runtime. I don't know whether it is possible, but it isn't immediately obvious that this can't happen on a background thread -- at least not for the two cases I mentioned. In fact, I'm pretty sure that at least the second case (ComboBoxListViewSkin line 199) is possible.


        lh.addInvalidationListener(comboBox.sceneProperty(), (o) -> {
            if (((ObservableValue)o).getValue() == null) {
                comboBox.hide();
            }
        });


I suspect that the following sequence of operations, all on a background thread, will trigger the exception:

1. Create a ComboBox
2. Create a Skin
3. Add the combobox to a Scene (the scene must not be showing, of course)

A solution might be to check whether the combobox is showing before hiding it.


        lh.addInvalidationListener(comboBox.sceneProperty(), (o) -> {
            if (((ObservableValue)o).getValue() == null) {
                if (comboBox.isShowing()) {
                    comboBox.hide();
                }
            }
        });


And, if the code in MenuBarSkin is a problem, then something like this might be in order:


            if (menuButton.isShowing()) {
                menuButton.hide();
            }


Similarly, I think this listener in `MenuBarSkin::rebuildUI` (starting on line 913) might need a check before calling `menuButton.hide()`:


            menuButton.menuListener = (observable, oldValue, newValue) -> {
                if (menu.isShowing()) {
                    menuButton.show();
                    menuModeStart(container.getChildren().indexOf(menuButton));
                } else {
                    menuButton.hide();     // <--  check if (menuButton.isShowing()) before calling
                }
            };

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

PR Comment: https://git.openjdk.org/jfx/pull/1717#issuecomment-2690955428