RFR: 8252871: fatal error: must own lock JvmtiThreadState_lock

[Robbin Ehn]

On Mon, 7 Sep 2020 23:07:13 GMT, Yasumasa Suenaga <ysuenaga at openjdk.org> wrote:

>> When these two methods (set_frame_pop/clear_frame_pop) are called in a handshake the requesting thread will have lock
>> the JvmtiThreadState_lock. But the thread executing one of these in the handshake may not be the owner.
>> So we only check that JvmtiThreadState_lock is locked.
>> 
>> When verifying the callers to these methods I notice "clear_to_frame_pop" was unused, so instead of fixing it I remove
>> it.
>> Passes testing locally, still running T3 and T7.
>> 
>> Now passed!
>
> Thanks for catching up this. Looks good.

Hi David,

> > When these two methods (set_frame_pop/clear_frame_pop) are called in a handshake the requesting thread will have lock
> > the JvmtiThreadState_lock. But the thread executing one of these in the handshake may not be the owner.
> 
> Ouch! We continue to get bitten by the fact the executor of a handshake
> operation is not known a-priori. :(
> 

We could add this. But I would prefer not doing it in this bug fix.

> > So we only check that JvmtiThreadState_lock is locked.
> 
> That avoids the problem but it is not really a sufficient check - we
> need to know that it is the handshaker thread that owns the lock and
> that it acquired it purely for the purpose of this handshake operation!
> 

Right now we know that because I traced the code :)

As I said I can make the requesting thread known to execution thread,
but I would really prefer not doing that in this bug fix.

> More importantly this issue shows that the locking code is in fact
> incorrect. We are on very dangerous ground if we are going to allow
> locks to be "proxied" in this fashion! This is akin to reintroducing
> "sneaky locks"!

A problem is that we use a safepoiting global lock to protect per thread resource.
The way we get around this with safepoints is:
"assert_locked_or_safepoint" (assert may be passed by a non-JavaThread or JavaThread in blocked/native without lock
during a safepoint (safepoint + VM thread would fix that)) Which have this in it:
  // see if invoker of VM operation owns it
  VM_Operation* op = VMThread::vm_operation();
  if (op != NULL && op->calling_thread() == lock->owner()) return;

So we already have proxied locks and in other cases just avoiding them (by just checking if at safepoint).

And as I said a few times now, I said I can make the requesting thread known to execution thread,
but I would really prefer not doing that in this bug fix.

> 
> If a handshake operation can be executed by either the target thread or
> the handshaker thread, and locks are required, then they will have to be
> acquired in the context of the handshake operation. If we can't do that
> because of potential deadlocks in the handshake mechanism then I feel
> that handshakes have a serious flaw that needs to be rectified. Perhaps
> we need a way to force the operation to be executed by the handshaker so
> that they can setup the right execution environment prior to the handshake?

The problem is that it's a safepointing lock.
Only executing the handshake by requester is an interesting idea, I'll investigate that.

> 
> > When verifying the callers to these methods I notice "clear_to_frame_pop" was unused, so instead of fixing it I remove
> > it.
> 
> There is already a bug for that:
> 
> https://bugs.openjdk.java.net/browse/JDK-8252816
> 
> so if this proceeds you should add that issue to the PR.

Ok, I'll do that.

Thanks for having a look, let me know howto proceed.

> 
> Thanks,
> David
> -----

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

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