RFR: 8295816: jdwp jck tests failing with "FATAL ERROR in native method: JDWP SetTag, jvmtiError=JVMTI_ERROR_WRONG_PHASE(112)"

[Daniel D. Daugherty]

On Mon, 24 Oct 2022 05:29:26 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

> Various debugger tests (mainly JCK vm/jdwp tests) are failing with:
> 
> `FATAL ERROR in native method: JDWP SetTag, jvmtiError=JVMTI_ERROR_WRONG_PHASE(112) `
> 
> Sometimes instead of `SetTag` the message says `GetTag` or `signature`.  This is a new issue caused by [JDK-8295375](https://bugs.openjdk.org/browse/JDK-8295375), which changed the debug agent CLASS_PREPARE event handling that is used for class tracking. It moved it from the main debug agent JVMTIEnv to a JVMTIEnv that already existed for class tracking purposes to deal with OBJECT_FREE events. Because of this, synchronizing around VMDEATH was lost, so it's possible for the CLASS_PREPARE event to be received just before VMDEATH, but then VMDEATH completes before the CLASS_PREPARE callback can complete. This results in WRONG_PHASE errors when making some JVMTI calls.
> 
> The easiest fix is for the code in `classTrack_addPreparedClass()` to check for JVMTI_ERROR_WRONG_PHASE, and if it gets it then just return. It should also assert that `gData->vmDead` is true when this happens. This is needed in 3 places where JVMTI is called.
> 
> The purist fix would be to somehow synchronize with VMDEATH, either with the same locking done by the main event handling code (for example, use BEGIN_CALLBACK and END_CALLBACK), or by having the class tracking JVMTIEnv also enable VMDEATH events. The VMDEATH callback block would disable the CLASS_PREPARE events, and also block until any currently executing CLASS_PREPARE callback has exited. I think this is overkill, and also adds locking overhead to the CLASS_PREPARE callback, so I'm going with the simpler solution of allowing WRONG_PHASE.

Thanks for adding is_wrong_phase() with the comments about
why it is okay. The deletion of the 'gdata != NULL' check also
caught my eye, but I'm getting more and more convinced that
it's okay to catch this kind of "can't happen" error via the deref
of the pointer that shouldn't be NULL...


Thumbs up.

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

Marked as reviewed by dcubed (Reviewer).

PR: https://git.openjdk.org/jdk/pull/10831