RFR: 8252871: fatal error: must own lock JvmtiThreadState_lock

[Robbin Ehn]

Hi David, our mail service did the wrong thing here.

This was a reply to your comment, so you should have been in To-field, 
this quote above "Hi David" should not have been here, and this was a 
reply your mail (via github comment).

Thanks, Robbin


On 2020-09-08 09:39, Robbin Ehn wrote:
> 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
>