On Sat, 4 Dec 2021 00:02:04 GMT, Brent Christian <bchri...@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. > > 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()); > } Yes, this is my concern too; lastReturned is a weak ref, and it is possible for it to have been cleared at this point, so get() can return null. I will post a more complete explanation. ------------- PR: https://git.openjdk.java.net/jdk/pull/6488
