jfx
jfx copied to clipboard
openjdk
8279640: ListView with null SelectionModel/FocusModel throws NPE
This PR fixes a bunch of NPEs when a null SelectionModel or FocusModel is set on a ListView.
The following NPEs are fixed (all are also covered by exactly one test case): NPEs with null selection model:
- Mouse click on a
ListCell - SPACE key press
- KP_UP (arrow up) key press
- HOME key press
- END key press
- BACK_SLASH + CTRL key press
NPEs with null focus model:
- SPACE key press
- Select an items: getSelectionModel().select(1)
- Clear-Select an item and add one after: listView.getSelectionModel().clearAndSelect(1); listView.getItems().add("3");
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)
Issue
- JDK-8279640: ListView with null SelectionModel/FocusModel throws NPE
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/711/head:pull/711
$ git checkout pull/711
Update a local copy of the PR:
$ git checkout pull/711
$ git pull https://git.openjdk.org/jfx pull/711/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 711
View PR using the GUI difftool:
$ git pr show -t 711
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/711.diff
:wave: Welcome back mhanl! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
Webrevs
- 04: Full - Incremental (2cc2bd16)
- 03: Full - Incremental (5f61fe0a)
- 02: Full - Incremental (9146e080)
- 01: Full - Incremental (ffe781dd)
- 00: Full (d31d3ecc)
Was it considered to not allow these models to be set to null instead? Or to use a Null Object? I'd much prefer this instead of polluting the code with null checks on every access of these models.
Furthermore, the API makes no mention what is supposed to happen when these models are set to null, so this seems undefined at this time. If null is supposed to be valid then I think the documentation should clearly state what this means. Having no focus model for example should probably mean that nothing can ever get focus and that the focused item is always null and index is always -1. Instead of adding null checks to achieve this however I'd use a NullFocusModel instance as it is easy to forget a null check (also when making future changes).
Same goes for the selection model; if set to null it should not allow any kind of selection, which again seems best achieved by installing a model that ignores all such actions and always returns empty values.
Same goes for the selection model; if set to
nullit should not allow any kind of selection, which again seems best achieved by installing a model that ignores all such actions and always returns empty values.
We had a similar discussion on https://github.com/openjdk/jfx/pull/557. We came to the conclusion that we allow null as a selection or focusmodel and don't set a noop selection model or something of this kind.
The biggest problem with this is the following example:
I as a developer set the selection model to null via setSelectionModel(null). Now if the code silently set the selection model to an empty noop selection model, we won't get null when calling getSelectionModel(), which a developer would expect.
Also from a look of the code, even if we use a noop focus model, we would still the focus because of the anchor stuff, which is set in the ListViewBehaviour.
If we do a null check and fast return like now, they won't be set -> there are not visible. So it might not even fix our problem but makes even more.
We had a similar discussion on #557. We came to the conclusion that we allow null as a selection or focusmodel and don't set a noop selection model or something of this kind.
Even earlier though in https://github.com/javafxports/openjdk-jfx/issues/569 a NONE model was suggested. And I still think that makes much more sense, because in effect the "behavior" of the NONE model is now spread out over dozens of places in the code (as the null checks are what is defining the behavior) instead of in a single place.
The biggest problem with this is the following example: I as a developer set the selection model to null via
setSelectionModel(null). Now if the code silently set the selection model to an empty noop selection model, we won't get null when callinggetSelectionModel(), which a developer would expect.
This is an example from ListView which already does something custom because the property is lazily instantiated:
public final MultipleSelectionModel<T> getSelectionModel() {
return selectionModel == null ? null : selectionModel.get();
}
Just write that as:
public final MultipleSelectionModel<T> getSelectionModel() {
return selectionModel == null ? null :
selectionModel.get() == NONE_SELECTION_MODEL ? null : selectionModel.get();
}
NONE_SELECTION_MODEL is stateless so only a single instance is needed.
Also from a look of the code, even if we use a noop focus model, we would still the focus because of the
anchorstuff, which is set in theListViewBehaviour. If we do a null check and fast return like now, they won't be set -> there are not visible. So it might not even fix our problem but makes even more.
A check can also be done to see if something matches the NONE model, just like you can check for null, so you can still fast return in special cases.
The chosen approach works as well of course.
public final MultipleSelectionModel<T> getSelectionModel() { return selectionModel == null ? null : selectionModel.get() == NONE_SELECTION_MODEL ? null : selectionModel.get(); }
That would work altough I think it's a bit hacky as when I listen on the selectionModelProperty I still would get the noop selection model.
I'm in general not a fan of 'self-changing' properties. When a null value is possible, guards against are needed.
Unfortunately there is no built-in way to forbid null in Java as whole.
A check can also be done to see if something matches the
NONEmodel, just like you can check fornull, so you can still fast return in special cases.
But then I don't see any advantage as it makes no difference if we check for NONE model or null. Checks are needed in any case.
@kevinrushforth The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).
When a null value is possible, guards against are needed. Unfortunately there is no built-in way to forbid null in Java as whole.
I think null checks should only be done to verify the preconditions of an operation, and not to guard against bugs. This means that, when null is not a valid value for a property (regardless of whether it is a possible value), no attempt should be made to guard against this in downstream code. In fact, I would argue that doing so is a strong code smell.
The question is whether or not null should be a valid value for the selectionModel and focusModel properties. I think there are good reasons to think that it shouldn't. The primitive property classes (IntegerProperty, BooleanProperty, etc.) have the same issue, and they deal with this situation by coercing the invalid null value to their respective default value.
It's not clear to me whether the ListCellBehavior.anchor attached property setter would need any special-casing. When null (or -1 in case of ListViewBehavior.setAnchor) is passed in, it seems to remove the anchor. Isn't that what we'd expect?
The question is whether or not
nullshould be a valid value for theselectionModelandfocusModelproperties. I think there are good reasons to think that it shouldn't. The primitive property classes (IntegerProperty,BooleanProperty, etc.) have the same issue, and they deal with this situation by coercing the invalidnullvalue to their respective default value.
I think the property classes for primitives are another use case, since those values really can't be null.
I get your point but I think a null selectionModel or focusModel should be allowed.
At least an use case would be to have a read-only ListView which should not be selectable nor focusable.
If you just set a ListView to uneditable (via setEditable(..)), you can still select entries. If you disable the ListView it is grey styled which is not desired in this use case. Of course I can change the style, but a null selection and-focusModel may make sense as well.
But I agree to a certain point, maybe it make's sense to fail for some properties when null is set/or use default values (but without changing the property?). Right now some properties in e.g. Node can be set to null and it will throw a NPE somewhere (e.g. setCacheHint(null), setRotationAxis(null), Labeled.setFont(null) ...).
But on the other hand a lot of code handle properties with null with some kind of default behaviour (the property itself is never changed).
Examples for this:
-
Labeled.getTextOverrun()-> null will be handled as ELLIPSIS in LabeledSkinBase -
Labeled.getEllipsisString()-> null will be handled as "" in LabeledSkinBase - ...
So there is no consistent behaviour for this, but a lot of code already handles null by behaving in some kind of default way without changing the property directly, and I think it might be the best to adapt to this.
@Maran23
Adding additional issue to issue list: 8230098: NPEs in ListView with no SelectionModel.
I'd like to see this get resolved for JavaFX 20. I note there is a similar issue with TableView, which is tracked by JDK-8187145.
As I mentioned in that JBS issue, I tend to agree that if we were starting today with a blank sheet of paper, we might have disallowed null and defined a "noop" selection model and/or focus model. At this point, though, it would be a breaking change, even though we don't specify the behavior of null, so we ought to make it work in those places where it doesn't.
So there is no consistent behaviour for this, but a lot of code already handles null by behaving in some kind of default way without changing the property directly, and I think it might be the best to adapt to this.
This seems like the best way forward to me, but let's see what comes out of the review.
As part of this, we should clarify the spec that a null selection (and focus) model is allowed, and say what it does. This might mean a CSR, but I'd want to look at the current docs first.
Setting selection model to null (i.e. disabling selection) is a fairly frequent operation, @kevinrushforth . The code must support that. So no CSR is needed, I think.
I took a look at the bug description for JDK-8230098, and it should be closed as a duplicate of JDK-8279640 rather than adding both as solved by this PR (adding additional issues to a PR is used when that PR solves two different, but related issues).
So let's closed JDK-8230098 as a duplicate and you can remove it from this PR with /issue remove.
So no CSR is needed, I think.
Probably you are right if all we end up doing is clarifying existing behavior. For example, if we add a sentence to the effect that "a null selection model disables selection", and that is what already happens today, then a CSR would likely not be needed. Unlike the JDK, which requires a CSR for any non-trivial spec change, we have often added clarifying comments to JavaFX API without a CSR.
good point. +1 for clarification; same applies to TableView JDK-8187145. I wonder if we also need to apply the same treatment to TreeTableView.
Quite possibly. Can you check it as part of fixing JDK-8187145 and expand that issue to cover TreeTableView if needed?
Can you check it as part of fixing JDK-8187145 and expand that issue to cover TreeTableView if needed?
Absolutely. Just checked - we have the same problem there. Should it be a different ticket though?
As I mentioned in that JBS issue, I tend to agree that if we were starting today with a blank sheet of paper, we might have disallowed null and defined a "noop" selection model and/or focus model. At this point, though, it would be a breaking change, even though we don't specify the behavior of null, so we ought to make it work in those places where it doesn't.
I agree, if we could do it from scratch we can just implement something like a NonNullObjectProperty which won't allow null in some way. Or the JDK implements some way of declaring a variable as not nullable. 😃
And I agree that improving the docs makes sense and more information is always good.
I wonder if a null check should be added to:
- ListViewSkin:381 calls getSelectionModel()
There is a condition observed while debugging JDK-8187145, when a non-null selection model is set, the user creates a non-empty selection, then the selection is set to null.
The UI still shows selected cells - does the same bug exists in ListView?
ListViewSkin:381 calls getSelectionModel()
probably, same to focusModel here.
There is a condition observed while debugging JDK-8187145, when a non-null selection model is set, the user creates a non-empty selection, then the selection is set to null.
The UI still shows selected cells - does the same bug exists in ListView?
Just tried and it looks fine for ListView.
There is also an unguarded access in CellBehaviorBase.selectRows(int,int) : 312 and CellBehaviorBase.sompleSelect(MouseButton,int,boolean) : 274, which are being fixed by #876
I'd suggest to integrate #876 first, followed by this PR, in order to avoid merge conflicts.
There is also an unguarded access in CellBehaviorBase.selectRows(int,int) : 312 and CellBehaviorBase.sompleSelect(MouseButton,int,boolean) : 274, which are being fixed by #876
I'd suggest to integrate #876 first, followed by this PR, in order to avoid merge conflicts.
@andy-goryachev-oracle hmm .. are those methods reached if selectionModel == null? On skimming across its code, it looks like all callers back out (that is not call it) without selectionModel. Might be wrong, though - hard to tell without documented preconditions, and without tests 😉
@kleopatra : simpleSelect and selectRows are being called from CellBehaviorBase.mousePressed and mouseReleased, so probably yes. in any way, they are updated in #876 ; fixing them here would likely create a merge conflict.
