RFR: 8313678 - SymbolTable can leak Symbols during cleanup [v6]

[Oli Gillespie]

> Fix leak in SymbolTable during cleanup.
> 
> 1. symbol1 inserted in bucket, refcount 1
> 2. Decrement refcount for this symbol, refcount is now 0
> 3. symbol2 inserted in same bucket
> 4. during insertion, concurrentHashTable notices there is a dead entry in this bucket
>  4a. This triggers delete_in_bucket, which triggers SymbolTableLookup.equals for symbol2
>  4b. SymbolTableLookup.equals for symbol2 spuriously increments symbol2's refcount
> 5. symbol2 is newly inserted with a refcount of 2, thus can never be cleaned up -> leak
> 
> The cleanup routine of concurrentHashTable uses the lookup function for its secondary effect of checking whether any particular value is dead or not. It does not actually care about the main purpose of the lookup function, i.e. checking if a particular value is the desired value.
> 
> The lookup function for SymbolTable, however, has the side effect of incrementing the Symbol refcount when the lookup succeeds. This is presumably with the assumption that a successful lookup will result in a newly held reference. concurrentHashTable's delete_in_bucket can break this assumption by completing a successful lookup without retaining the result (nor ever decrementing the refcount).
> 
> Thus, with a particular sequence of events (as shown in the new test case), a newly inserted Symbol can have a refcount of 2 instead of 1. Even if all legitimate references to this Symbol are removed, it will remain live, thus leaking.
> 
> The fix allows the caller of .equals to specify if they intend to use the value after the lookup, or not, allowing SymbolTable to only increment the refcount when appropriate.
> 
> I'd be happy to hear if anyone has any alternative suggestions for the fix, it seems a bit awkward/dangerous in general that SymbolTableLookup's equals increments the refcount. Also, any suggestions for implementing the test case without relying on the String.hashCode implementation would be great.
> 
> Thanks @shipilev for help debugging and fixing this :)

Oli Gillespie has updated the pull request incrementally with one additional commit since the last revision:

  Tweak comment

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

Changes:
  - all: https://git.openjdk.org/jdk/pull/15137/files
  - new: https://git.openjdk.org/jdk/pull/15137/files/a8445534..dd5ee5fe

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=15137&range=05
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15137&range=04-05

  Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/15137.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/15137/head:pull/15137

PR: https://git.openjdk.org/jdk/pull/15137