RFR: 8003417: WeakHashMap$HashIterator removes wrong entry

Brent Christian bchristi at openjdk.java.net
Sat Dec 4 00:05:15 UTC 2021


On Sat, 20 Nov 2021 10:08:41 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

> Can I please get a review for this change which proposes to fix the issue reported in https://bugs.openjdk.java.net/browse/JDK-8003417?
> 
> The issue notes that this is applicable for `WeakHashMap` which have `null` keys. However, the issue is even applicable for `WeakHashMap` instances which don't have `null` keys, as reproduced and shown by the newly added jtreg test case in this PR.
> 
> The root cause of the issue is that once the iterator is used to iterate till the end and the `remove()` is called, then the `WeakHashMap$HashIterator#remove()` implementation used to pass `null` as the key to remove from the map, instead of the key of the last returned entry. The commit in this PR fixes that part.
> 
> A new jtreg test has been added which reproduces the issue as well as verifies the fix.
> `tier1` testing and this new test have passed after this change. However, I guess this will require a JCK run to be run too, perhaps? If so, I will need help from someone who has access to them to have this run against those please.

Changes requested by bchristi (Reviewer).

src/java.base/share/classes/java/util/WeakHashMap.java line 826:

> 824:                 throw new ConcurrentModificationException();
> 825: 
> 826:             WeakHashMap.this.remove(lastReturned.get());

Unlike `currentKey`, `HashIterator` does not necessarily maintain a strong reference to the referent of `lastReturned`. If the `lastReturned` WeakReference were cleared between `lastReturned` being set, and the `remove()` call, the null key could again be erroneously removed. It would be cool to add a test case for this (maybe using large objects as keys would tempt the GC to clear the weakrefs via `jdk.test.lib.util.ForceGC`).

Since we store the `NULL_KEY` sentinel value for the null key, I believe `Entry.get()` will only return null in the case that the weakref has been cleared.  So could we instead fix this with:


if (lastReturned.get() != null) {
    WeakHashMap.this.remove(lastReturned.get());
}


?
Or, if we're worried about the ref being cleared between the null check and remove(), maybe:

Object lastReturnedKey = lastReturned.get();
if (lastReturnedKey != null) {
    WeakHashMap.this.remove(lastReturned.get());
}

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

PR: https://git.openjdk.java.net/jdk/pull/6488


More information about the core-libs-dev mailing list