8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet #1136

lukostyra · 2023-05-15T12:49:41Z

This issue happened because childSet member of Parent was modified during onProposedChange() call - that call did not recognize negative indexes as invalid, which caused an exception when actually adding the Node to a List.

This seemed like the simplest solution which doesn't rework a lot of code underneath. Exceptions coming from a backing list/collection technically are handled by VetoableListDecorator's try-catch clauses, however VetoableListDecorator does not provide an interface to react when such an exception happens - without it we cannot revert childSet back to its original state.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet

Reviewers

John Hendrikx (@hjohn - Committer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1136/head:pull/1136
$ git checkout pull/1136

Update a local copy of the PR:
$ git checkout pull/1136
$ git pull https://git.openjdk.org/jfx.git pull/1136/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1136

View PR using the GUI difftool:
$ git pr show -t 1136

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1136.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-15T12:50:52Z

👋 Welcome back lkostyra! 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.

mlbridge · 2023-05-15T12:56:02Z

Webrevs

hjohn · 2023-05-15T15:24:47Z

I think it may be better to do this check in the callers of onProposedChange (using the assertions in Objects to test for valid indices.). This will also allow to check when the index is too large, which is a similar problem. Note that you can't do the "too large" check in onProposedChange as for remove, size() would be too large, while for add(int, E) size() would still be valid.

Also, for the cases where onProposedChange is called with multiple sets of indices, those don't need a check (they come from calls like removeAll, which will all be valid).

Furthermore, it should be an IndexOutOfBoundsException as this is specified by the List interface.

I noticed there are similar problems in ObservableListWrapper. This method for example:

@Override
protected void doAdd(int index, E element) {
    if (elementObserver != null)
        elementObserver.attachListener(element);
    backingList.add(index, element);
}

This will attach a listener to the element (assuming it is an Observable) even if backingList.add fails...

lukostyra · 2023-05-16T07:03:29Z

Thanks for your notes, I was wondering if there was a better approach. I'll rework the patch.

lukostyra · 2023-05-17T16:40:01Z

As a small note, the issue is solved with current patches, however I think the best solution would be what I mentioned in the original PR message - an interface in VetoableListDecorator which would let us "roll-back" any changes from onProposedChange (ex. revert childSet to original state) when backing lists throw an exception. This would require a more thorough analysis of what bits use VetoableListDecorator and if any roll-backs are necessary there in addition to Parent code.

hjohn

I think ObservableListWrapper#remove(int, int) also needs a check index check, or it may end up remove some items and then throw an exception (exceptions thrown from collections should not modify its state).

hjohn · 2023-05-17T22:45:36Z

modules/javafx.base/src/main/java/com/sun/javafx/collections/VetoableListDecorator.java

+        Objects.requireNonNull(col);
        onProposedChange(Collections.unmodifiableList(new ArrayList<>(col)), 0, size());


It's good to make it explicit. It already threw NPE because of new ArrayList<>(col), so this is not a change.

hjohn · 2023-05-17T22:47:49Z

modules/javafx.base/src/main/java/com/sun/javafx/collections/VetoableListDecorator.java


    @Override
    public boolean removeAll(Collection<?> c) {
+        Objects.requireNonNull(c);


Looks like a good change, if c was null and the backing list was empty, this would not throw NPE, but now it does.

hjohn · 2023-05-17T22:49:57Z

modules/javafx.graphics/src/test/java/test/javafx/scene/ParentTest.java

+        // below call should throw no exception - if it does, internal state is corrupted
+        g.getChildren().remove(0);
+    }
+


I think you should also add tests for the null cases, if they aren't part of this test already.

hjohn · 2023-05-17T23:15:28Z

As a small note, the issue is solved with current patches, however I think the best solution would be what I mentioned in the original PR message - an interface in VetoableListDecorator which would let us "roll-back" any changes from onProposedChange (ex. revert childSet to original state) when backing lists throw an exception. This would require a more thorough analysis of what bits use VetoableListDecorator and if any roll-backs are necessary there in addition to Parent code.

I'm not convinced this is needed or desirable. The VetoableListDecorator is currently set up that its owner can already make "preparations" for the change when it knows it won't veto it (there can be only one vetoer, making this reasonable). Furthermore, it has the wrapped list under its own control, so it knows what to expect (no exceptions can occur (after this fix) if it decides not to veto).

A roll back mechanism would just complicate the code for very little benefit (a correct FX application won't run into these situations, you'll only be likely to encounter them during development). The whole mechanism surrounding the children list and bad additions should be seen as a best effort early warning system (like ConcurrentModificationExeption).

With this fix I don't see any other ways you could get it in a bad state.

lukostyra · 2023-05-22T13:56:54Z

A roll back mechanism would just complicate the code for very little benefit (a correct FX application won't run into these situations, you'll only be likely to encounter them during development). The whole mechanism surrounding the children list and bad additions should be seen as a best effort early warning system (like ConcurrentModificationExeption).

Understood, let's stick with current approach then.

kevinrushforth

Looks good.

openjdk · 2023-05-24T11:25:17Z

@lukostyra 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:

8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet

Reviewed-by: jhendrikx, kcr

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 10 new commits pushed to the master branch:

2f5dcfd: 8306329: Update ICU4C to 73.1
8110f54: 8306328: Update libFFI to 3.4.4
bdadcb2: 8284542: [Accessibility] [Win] Missing attribute for toggle state of CheckBox in CheckBoxTreeItem
6aeaff3: 8295078: TextField blurry when inside an TitledPane -> AnchorPane
bff41c2: 8308114: Bump minimum version of macOS for x64 to 11.0 (matching aarch64)
c1a1179: 8245919: Region#padding property rendering error
e7974bc: 8308028: Replace more uses of System.getProperty("os.name") with PlatformUtil calls
8aff525: 8307960: Create Table Column PopupMenu lazily
7095364: 8307807: Replace use of System.getProperty("os.name") with PlatformUtil calls
604fc26: 8307363: TextFlow.underlineShape()

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@hjohn, @kevinrushforth) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

lukostyra · 2023-05-24T11:35:43Z

/integrate

openjdk · 2023-05-24T11:37:07Z

@lukostyra
Your change (at version f1a8797) is now ready to be sponsored by a Committer.

hjohn · 2023-05-24T11:37:18Z

/sponsor

openjdk · 2023-05-24T11:38:33Z

Going to push as commit 4b24c86.
Since your change was applied there have been 10 commits pushed to the master branch:

2f5dcfd: 8306329: Update ICU4C to 73.1
8110f54: 8306328: Update libFFI to 3.4.4
bdadcb2: 8284542: [Accessibility] [Win] Missing attribute for toggle state of CheckBox in CheckBoxTreeItem
6aeaff3: 8295078: TextField blurry when inside an TitledPane -> AnchorPane
bff41c2: 8308114: Bump minimum version of macOS for x64 to 11.0 (matching aarch64)
c1a1179: 8245919: Region#padding property rendering error
e7974bc: 8308028: Replace more uses of System.getProperty("os.name") with PlatformUtil calls
8aff525: 8307960: Create Table Column PopupMenu lazily
7095364: 8307807: Replace use of System.getProperty("os.name") with PlatformUtil calls
604fc26: 8307363: TextFlow.underlineShape()

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-24T11:38:50Z

@hjohn @lukostyra Pushed as commit 4b24c86.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

lukostyra added 2 commits May 15, 2023 12:38

Parent: Throw exception when added child index is negative

be484c3

Add negative index test for Parent

1508c62

openjdk bot added the rfr Ready for review label May 15, 2023

lukostyra added 3 commits May 16, 2023 14:29

Parent: Remove index check from onProposedChange

e2de758

ParentTest: Expect IndexOutOfBoundsException, add test cases

dfd3eb9

Add index/null checks to ObservableListWrapper and VetoableListDecorator

6302383

hjohn suggested changes May 17, 2023

View reviewed changes

lukostyra added 2 commits May 22, 2023 12:29

ObservableListWrapper: Add from-to index check to remove(int, int)

7eff40e

ParentTest: Add tests for NPE and *All calls

f1a8797

hjohn approved these changes May 22, 2023

View reviewed changes

kevinrushforth self-requested a review May 22, 2023 21:28

kevinrushforth approved these changes May 24, 2023

View reviewed changes

openjdk bot added the ready Ready to be integrated label May 24, 2023

openjdk bot added the sponsor Ready to sponsor label May 24, 2023

openjdk bot added the integrated Pull request has been integrated label May 24, 2023

openjdk bot closed this May 24, 2023

openjdk bot removed ready Ready to be integrated rfr Ready for review labels May 24, 2023

openjdk bot removed the sponsor Ready to sponsor label May 24, 2023

lukostyra deleted the index_oob_sync-8301763 branch May 24, 2023 11:57

		Objects.requireNonNull(col);
		onProposedChange(Collections.unmodifiableList(new ArrayList<>(col)), 0, size());

8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet #1136

8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet #1136

Uh oh!

Conversation

lukostyra commented May 15, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 15, 2023

Uh oh!

mlbridge bot commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

hjohn commented May 15, 2023

Uh oh!

lukostyra commented May 16, 2023

Uh oh!

lukostyra commented May 17, 2023

Uh oh!

hjohn left a comment

Choose a reason for hiding this comment

Uh oh!

hjohn May 17, 2023

Choose a reason for hiding this comment

Uh oh!

hjohn May 17, 2023

Choose a reason for hiding this comment

Uh oh!

hjohn May 17, 2023

Choose a reason for hiding this comment

Uh oh!

hjohn commented May 17, 2023

Uh oh!

lukostyra commented May 22, 2023

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented May 24, 2023

Uh oh!

lukostyra commented May 24, 2023

Uh oh!

openjdk bot commented May 24, 2023

Uh oh!

hjohn commented May 24, 2023

Uh oh!

openjdk bot commented May 24, 2023

Uh oh!

openjdk bot commented May 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

lukostyra commented May 15, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 15, 2023 •

edited

Loading