request for review (S): 6814943: getcpool001 catches more than one JvmtiThreadState problem

[Keith McGuigan]

On Jan 10, 2011, at 7:07 PM, David Holmes wrote:

> Hi Keith,
>
> Keith McGuigan said the following on 01/11/11 03:36:
>> This closes a race condition hole between  
>> JvmtiThreadState::state_for_while_locked() and ~JavaThread().   
>> Without this, the state_for_while_locked() could see a value of  
>> false for thread->is_exiting(), then the entirety of ~JavaThread()  
>> could run, the state_for_while_locked() could then finish leaving  
>> the JvmtiThreadState referring to a zombie thread.
>> webrev: http://cr.openjdk.java.net/~kamg/6814943/webrev.00/
>
> I think it would be slightly cleaner, as per my private email, to  
> keep the locking internal to the JVMTI methods ie:
>
> void
> JvmtiEventControllerPrivate::thread_ended(JavaThread *thread) {
>  // Removes the JvmtiThreadState associated with the specified thread.
>  // May be called after all environments have been disposed.
>
>  EC_TRACE(("JVMTI [%s] # thread ended",
>            JvmtiTrace::safe_get_thread_name(thread)));
>   MutexLocker mu(JvmtiThreadState_lock);
>   JvmtiThreadState *state = thread->jvmti_thread_state();
>   if (state != NULL) {
>      delete state;
>   }
> }
>
> and then in JavaThread::exit just do:
>
> if (JvmtiEnv::environments_might_exist()) {
>  JvmtiExport::cleanup_thread(this);
> }
>
> as cleanup_thread already does a null check. Note that if an  
> environment comes into existence we are still relying on another  
> thread seeing that this thread has terminated prior to the above  
> check.

Sure that sounds reasonable.  I'll change the code to do that.

Thanks!

--
- Keith