RFR: JDK-8180599: Possibly miss to iterate monitors on thread exit

[David Holmes]

Hi Roman,

On 18/05/2017 10:16 PM, Roman Kennke wrote:
> As David pointed out here:
> 
> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2017-May/023468.html
> 
> The fix to JDK-8180175: ObjectSynchronizer only needs to iterate in-use
> monitors introduced a bug where we could potentially miss to iterate
> oops in monitors of exiting threads. The proposed fix is as Robbin Ehn
> suggested to move the call to omFlush() before we remove the thread from
> _thread_list:
> 
> http://cr.openjdk.java.net/~rkennke/8180599/webrev.00/
> <http://cr.openjdk.java.net/%7Erkennke/8180599/webrev.00/>
> 
> This solves the problem because there's no window where monitors would
> not appear in any list.
> 
> I tested a number of programs and jtreg hotspot_gc tests and it doesn't
> show any ill effects.
> 
> Ok?

I think this is okay as an approach.

There is one issue I can see this will expose and that is that omFlush 
doesn't zero omFreeCount. With the current change we may go to a 
safepoint when acquiring the Threads_lock after calling omFlush and that 
could lead to monitor deflating and (if enabled) a call to verifyInUse, 
where we would fail this assert:

   assert(free_tally == Self->omFreeCount, "free count off");

but the fix is trivial: just zero omFreeCount the same way we already 
zero omInUseCount. And we should also have a similar assert that 
tally==omFreeCount.

I also verified that there are no thread inspection mechanisms that 
might be surprised if they find a thread with its monitor lists nulled 
out. (Again due to a safepoint being taken immediately after omFlush.)

The comment:

// Reclaim the objectmonitors from the omFreeList of the moribund thread.

was already inaccurate since in-use lists were added, and should be 
updated to reflect the transfer of free or in-use monitors to the global 
lists.

Also the comment block before ObjectSynchronizer::omFlush in 
synchronizer.cpp needs to be updated to reflect this change.

Thanks,
David

> Roman
>