RFR: 8261391: ZGC crash - SEGV in RevokeOneBias::do_thread [v3]

[Erik Osterlund]

Hi David,

As for the general problem of accessing a private handle from another thread, my thinking was that this could only possibly be safe if a thread ”grabs on to” a remote thread so it’s not concurrently running. Otherwise doing so would never be safe with or without the new concurrent root processing code.
So the reasoning was that if we make it safe for safepoint operations to access any thread’s private oops, and for handshakes to access the handshakee’s private oops, then that should cover that kind of interactions.

However, I did think about the possibility of the handshaker’s private oops being accessed by the handshakee, and thought like you that you just shouldn’t do that. You should use global handles for such shared interactions. But ever since then I have now and then thought ”what if I missed a handle”. And this bug proves that I did indeed miss a handle.

Therefore, while I agree that passing a private handle to be used by another thread is something we shouldn’t do, I would sleep better at night knowing that it is safe to do so anyway, should it happen, in case there is another strange interaction that I missed, or some new code comes in that thinks it’s safe. This is the more robust way of dealing with it.

Thanks,
/Erik

> On 18 Feb 2021, at 13:48, David Holmes <david.holmes at oracle.com> wrote:
> 
> Correction ...
> 
>> On 18/02/2021 12:22 pm, David Holmes wrote:
>> More generally I am concerned by the fact that we have a potential bug if a Handle created by one thread is accessed by another, without first calling start_processing() on the creator of the Handle. This would seem to be error prone to me unless it is actually hidden away inside the Handle implementation - though how to do that efficiently is unclear. I'm very concerned that it is too easy to write code (eg. handing something off to the  service thread or notification thread) that will be buggy.
> 
> I was confusing Handles with Resources in terms of lifecycle - not realising Handles are stack-local to a Thread and can only be shared with another thread very, very carefully.
> 
> David
> 
>> Perhaps @fisk can comment on the general problem.
>> Thanks,
>> David
>> src/hotspot/share/runtime/handshake.cpp line 84:
>>> 82:  public:
>>> 83:   AsyncHandshakeOperation(AsyncHandshakeClosure* cl, JavaThread* target, jlong start_ns)
>>> 84:     : HandshakeOperation(cl, target, NULL), _start_time_ns(start_ns) {}
>> Not at all clear why requester is always NULL here. There is obviously a requester thread even in the async case. If that thread created a Handle in the op then don't we need to call start_processing on it?
>> src/hotspot/share/runtime/handshake.cpp line 328:
>>> 326:
>>> 327: void Handshake::execute(HandshakeClosure* hs_cl) {
>>> 328:   HandshakeOperation cto(hs_cl, NULL, Thread::current());
>> It is unfortunate that we need to manifest the current thread here (and elsewhere).
>> -------------
>> Changes requested by dholmes (Reviewer).
>> PR: https://git.openjdk.java.net/jdk/pull/2571