8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash

[Daniil Titov]

Hi Serguei,

I could be missing something but as I understood the suggested alternative approach was  to not introduce the callbacksEnabled  flag and instead just unregister other callbacks inside VMDeath callback.  My concern was the case when one thread is on line 1302 (it is going to call a class loaded callback), while another thread just entered VMDeath callback. Without callbacksEnabled   flag the first thread will enter the callback and wait on the raw monitor acquired by VMDeath callback thread, after that it will continue and call JVMTI API that might throw WRONG_PHASE error if by this time the second thread  already returned from the VMDeath callback and changed the JVMTI phase.

1277	void JvmtiExport::post_class_load(JavaThread *thread, Klass* klass) {
  1278	  if (JvmtiEnv::get_phase() < JVMTI_PHASE_PRIMORDIAL) {
  1279	    return;
  1280	  }
  1281	  HandleMark hm(thread);
  1282	
  1283	  EVT_TRIG_TRACE(JVMTI_EVENT_CLASS_LOAD, ("[%s] Trg Class Load triggered",
  1284	                      JvmtiTrace::safe_get_thread_name(thread)));
  1285	  JvmtiThreadState* state = thread->jvmti_thread_state();
  1286	  if (state == NULL) {
  1287	    return;
  1288	  }
  1289	  JvmtiEnvThreadStateIterator it(state);
  1290	  for (JvmtiEnvThreadState* ets = it.first(); ets != NULL; ets = it.next(ets)) {
  1291	    if (ets->is_enabled(JVMTI_EVENT_CLASS_LOAD)) {
  1292	      JvmtiEnv *env = ets->get_env();
  1293	      if (env->phase() == JVMTI_PHASE_PRIMORDIAL) {
  1294	        continue;
  1295	      }
  1296	      EVT_TRACE(JVMTI_EVENT_CLASS_LOAD, ("[%s] Evt Class Load sent %s",
  1297	                                         JvmtiTrace::safe_get_thread_name(thread),
  1298	                                         klass==NULL? "NULL" : klass->external_name() ));
  1299	      JvmtiClassEventMark jem(thread, klass);
  1300	      JvmtiJavaThreadEventTransition jet(thread);
  1301	      jvmtiEventClassLoad callback = env->callbacks()->ClassLoad;
  1302	      if (callback != NULL) {
  1303	        (*callback)(env->jvmti_external(), jem.jni_env(), jem.jni_thread(), jem.jni_class());
  1304	      }
  1305	    }
  1306	  }
  1307	}

Thanks!

--Daniil

On 3/20/19, 3:40 PM, "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote:

    Hi Daniil,
    
    No chance for it if you lock a raw monitor in the event callbacks.
    
    Thanks,
    Serguei
    
    On 3/20/19 14:58, Daniil Titov wrote:
    > Hi Serguei,
    >
    > I think that just disabling event notifications inside VMDeath callback still leaves a small window for VMDeath callback being called after the classload callback is called (or about being called)  but before it enters a raw monitor. Thus I decided to follow your original suggestion  and restore volatile modifier for callbacksEnabled.  Please review a new version of the patch.
    >
    > Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.03/
    > Bug : https://bugs.openjdk.java.net/browse/JDK-8218401
    >
    > Thanks!
    >
    > Best regards,
    > Daniil
    >
    >
    >
    > From: <serguei.spitsyn at oracle.com>
    > Organization: Oracle Corporation
    > Date: Tuesday, March 19, 2019 at 7:17 PM
    > To: Daniil Titov <daniil.x.titov at oracle.com>, OpenJDK Serviceability <serviceability-dev at openjdk.java.net>, Jean Christophe Beyler <jcbeyler at google.com>
    > Subject: Re: 8218401: WRONG_PHASE: vmTestbase/nsk/jvmti test crash
    >
    > Hi Daniil,
    >
    > I'd keep the volatile modifier for callbacksEnabled to disable compiler optimizations.
    > Otherwise, looks good to me.
    >
    >
    > Another approach could be to disable event notifications in VMDeath callback with:
    >    SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_CLASS_LOAD, NULL);
    >    SetEventNotificationMode(JVMTI_DISABLE, JVMTI_EVENT_BREAKPOINT, NULL);
    >    . . .
    >
    > Thanks,
    > Serguei
    >
    > On 3/18/19 6:58 PM, Daniil Titov wrote:
    > Hi Serguei and JC,
    >
    > Please review a new version of the fix that locks a monitor across the callbacks, as Serguei suggested.
    >
    > Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.02/
    > Bug: https://bugs.openjdk.java.net/browse/JDK-8218401
    >
    > Thanks!
    > --Daniil
    >
    >
    > On 3/18/19, 9:47 AM, mailto:serguei.spitsyn at oracle.com mailto:serguei.spitsyn at oracle.com wrote:
    >
    >      Hi Daniil,
    >      
    >      The JVMTI phase can change in the middle of callback work after the
    >      check you added.
    >      I'd suggest to lock a raw monitor across the callbacks to make them atomic.
    >      
    >      Thank you for taking care about this issue!
    >      
    >      Thanks,
    >      Serguei
    >      
    >      
    >      
    >      On 3/15/19 16:08, Daniil Titov wrote:
    >      > Please review the change that fixes 3 tests that intermittently fail with JVMTI_ERROR_WRONG_PHASE error.
    >      >
    >      > The problem here is that the callbacks these tests enable keep processing events and perform JVMTI calls after VM is terminated. The fix makes these test listen for VMDeath event and  quick return from the callbacks after VMDeath event is received.
    >      >
    >      > Webrev: http://cr.openjdk.java.net/~dtitov/8218401/webrev.01/
    >      > Bug: https://bugs.openjdk.java.net/browse/JDK-8218401
    >      >
    >      > Thanks!
    >      > -Daniil
    >      >
    >      >
    >      
    >      
    >
    >
    >
    >
    >
    >