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

[David Holmes]

On Wed, 17 Feb 2021 19:50:00 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Handshakes may contain oop Handles from the requesting thread.
>> A handshake may be executed by another thread than the requester.
>> We must make sure any such Handle is safe to use by other threads before executing the handshake.
>> This change-set adds SWS::start_process() for the requesting thread also (we already do this for the target JavaThread).
>> 
>> Passes t1-5
>
> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Tiny spelling error

Hi Robbin,

I think there is a bug in your fix wrt. async handshakes - see below.

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.

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