8347753: VetoableListDecorator doesn't accept its own sublists for bulk operations #1679

mstr2 · 2025-01-15T00:32:37Z

Passing a VetoableListDecorator.subList() to any of its bulk operations (addAll, setAll, removeAll, retainAll) throws ConcurrentModificationException. The reason is that the VetoableListDecorator.modCount field is incremented before the underlying list's bulk operation is invoked, which causes a mismatch when the sublist is interrogated by the bulk operation.

However, simply updating the modCount field after the underlying list was modified also doesn't work, as in this case listeners can't see the correct value for modCount in their callback. The fix is to make a defensive copy of the sublist before invoking the underlying list's bulk operation.

/reviewers 2

Progress

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

Issue

JDK-8347753: VetoableListDecorator doesn't accept its own sublists for bulk operations (Bug - P4)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1679

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-01-15T00:33:12Z

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

openjdk · 2025-01-15T00:34:08Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-01-15T00:34:29Z

@mstr2
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

mlbridge · 2025-01-15T00:37:49Z

Webrevs

01: Full - Incremental (81917761)
00: Full (5ba5a110)

andy-goryachev-oracle · 2025-01-16T00:07:49Z

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

@@ -387,14 +387,37 @@ public int hashCode() {
        return list.hashCode();
    }

-    private class VetoableSubListDecorator implements List<E> {
+    /**
+     * Returns the specified collection as an unmodifiable list that can safely be used in all bulk


Do you think it might be easier to create a defensive copy always?

In other words, can we guarantee that it is impossible for the user to create a convoluted code involving maybe two VetoableListDecorators where the second one loops back the changes to the first one, however ridiculous that might sound?

The way I see it, the situation that erroneously triggers ConcurrentModificationException only happens when VetoableListDecorator accesses its own sublist:

try { modCount++; boolean ret = list.addAll(index, c); // --> c is its own sublist ...

Since modCount is modified first, and the sublist refers back to the same modified modCount, the exception occurs. It can't occur when we are dealing with another list (or a sublist of another list), since in this case there is no self-referential conflict.

The way ArrayList circumvents this problem is by incrementing modCount only after the operation is done, not before it has started; it doesn't create a defensive copy.

Thank you for clarification! I'll try to come up with a test (and I think even if I succeed, it does not mean that your solution is not good, since the caller can always create a defensive copy themselves).

BTW, what were you doing to trigger this bug?

BTW, what were you doing to trigger this bug?

This happened when I was writing some tests for the other VetoableListDecorator PRs...

andy-goryachev-oracle

The scenario I was referring to is impossible, I think.

One possible alternative is to create the defensive copy each time, this will save one extra pointer every time an iterator or a sublist gets created (these objects might be long lived). The code in this PR creates a copy in many (most?) cases anyway, and in my opinion, the memory is more precious resource that CPU cycles (i.e. using extra memory costs many more CPU cycles in garbage collection etc.), so please consider that.

some minor suggestions

andy-goryachev-oracle · 2025-01-16T18:17:13Z

modules/javafx.base/src/test/java/test/javafx/collections/VetoableObservableListTest.java

+    public void testAddAll_subList() {
+        list.addAll(list.subList(0, 2));
+        assertSingleCall(new String[] {"foo", "bar"}, new int[] {4, 4});
+    }


suggestion: also check that the list contains the newly added elements?
(here and in added tests that involve subList?)

I've added checks for the list content in all modified tests.

mstr2 · 2025-01-27T18:34:14Z

One possible alternative is to create the defensive copy each time, this will save one extra pointer every time an iterator or a sublist gets created (these objects might be long lived). The code in this PR creates a copy in many (most?) cases anyway, and in my opinion, the memory is more precious resource that CPU cycles (i.e. using extra memory costs many more CPU cycles in garbage collection etc.), so please consider that.

I don't quite understand what you mean. Can you elaborate?
In particular, what does "save one extra pointer" mean?

andy-goryachev-oracle · 2025-01-27T18:47:41Z

Can you elaborate?
what does "save one extra pointer" mean?

What I mean is simply get rid of the extra pointer like VetoableListIteratorDecorator.parent and the related logic, and simply create a copy of List/Collection each time.

mstr2 · 2025-01-27T18:55:11Z

Can you elaborate?
what does "save one extra pointer" mean?

What I mean is simply get rid of the extra pointer like VetoableListIteratorDecorator.parent and the related logic, and simply create a copy of List/Collection each time.

Okay, I see. We can't do that, because a sublist is specified to be a live view onto the source list. After all, list.subList(0, 3).clear() is valid code, and the result is that three elements are removed from the source list.

andy-goryachev-oracle · 2025-01-27T19:02:57Z

We can't do that, because a sublist is specified to be a live view onto the source list.

good point, thanks for pointing this out.

andy-goryachev-oracle

lgtm

mstr2 added 2 commits January 15, 2025 00:52

failing tests

e0e84ff

allow sublists to be used for bulk operations

5ba5a11

openjdk bot added the rfr Ready for review label Jan 15, 2025

andy-goryachev-oracle reviewed Jan 16, 2025

View reviewed changes

kevinrushforth self-requested a review January 23, 2025 17:17

mstr2 added 4 commits January 27, 2025 19:21

Merge branch 'master' into fixes/vetoable-list-decorator-sublist

9384b36

check list content in tests

3090a10

update tests

ece236b

update tests

8191776

andy-goryachev-oracle approved these changes Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8347753: VetoableListDecorator doesn't accept its own sublists for bulk operations #1679

8347753: VetoableListDecorator doesn't accept its own sublists for bulk operations #1679

mstr2 commented Jan 15, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 15, 2025

openjdk bot commented Jan 15, 2025

openjdk bot commented Jan 15, 2025

mlbridge bot commented Jan 15, 2025 •

edited

Loading

andy-goryachev-oracle Jan 16, 2025

mstr2 Jan 16, 2025

andy-goryachev-oracle Jan 16, 2025

mstr2 Jan 16, 2025

andy-goryachev-oracle left a comment

andy-goryachev-oracle Jan 16, 2025

mstr2 Jan 27, 2025

mstr2 commented Jan 27, 2025 •

edited

Loading

andy-goryachev-oracle commented Jan 27, 2025

mstr2 commented Jan 27, 2025

andy-goryachev-oracle commented Jan 27, 2025

andy-goryachev-oracle left a comment

8347753: VetoableListDecorator doesn't accept its own sublists for bulk operations #1679

Are you sure you want to change the base?

8347753: VetoableListDecorator doesn't accept its own sublists for bulk operations #1679

Conversation

mstr2 commented Jan 15, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 15, 2025

openjdk bot commented Jan 15, 2025

openjdk bot commented Jan 15, 2025

mlbridge bot commented Jan 15, 2025 • edited Loading

Webrevs

andy-goryachev-oracle Jan 16, 2025

Choose a reason for hiding this comment

mstr2 Jan 16, 2025

Choose a reason for hiding this comment

andy-goryachev-oracle Jan 16, 2025

Choose a reason for hiding this comment

mstr2 Jan 16, 2025

Choose a reason for hiding this comment

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle Jan 16, 2025

Choose a reason for hiding this comment

mstr2 Jan 27, 2025

Choose a reason for hiding this comment

mstr2 commented Jan 27, 2025 • edited Loading

andy-goryachev-oracle commented Jan 27, 2025

mstr2 commented Jan 27, 2025

andy-goryachev-oracle commented Jan 27, 2025

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

mstr2 commented Jan 15, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 15, 2025 •

edited

Loading

mstr2 commented Jan 27, 2025 •

edited

Loading