8255452: Doing GC during JVMTI MethodExit event posting breaks return oop

Reviewed-by: coleenp, dlong, rrich, sspitsyn

Author: fisk

src/hotspot/share/interpreter/interpreterRuntime.cpp

@@ -1269,7 +1269,10 @@ JRT_ENTRY(void, InterpreterRuntime::post_method_entry(JavaThread *thread))
 JRT_END
 
 
-JRT_ENTRY(void, InterpreterRuntime::post_method_exit(JavaThread *thread))
+// This is a JRT_BLOCK_ENTRY because we have to stash away the return oop
+// before transitioning to VM, and restore it after transitioning back
+// to Java. The return oop at the top-of-stack, is not walked by the GC.
+JRT_BLOCK_ENTRY(void, InterpreterRuntime::post_method_exit(JavaThread *thread))
   LastFrameAccessor last_frame(thread);
   JvmtiExport::post_method_exit(thread, last_frame.method(), last_frame.get_frame());
 JRT_END

src/hotspot/share/prims/jvmtiExport.cpp

@@ -1560,16 +1560,12 @@ void JvmtiExport::post_method_entry(JavaThread *thread, Method* method, frame cu
   }
 }
 
-void JvmtiExport::post_method_exit(JavaThread *thread, Method* method, frame current_frame) {
+void JvmtiExport::post_method_exit(JavaThread* thread, Method* method, frame current_frame) {
   HandleMark hm(thread);
   methodHandle mh(thread, method);
 
-  EVT_TRIG_TRACE(JVMTI_EVENT_METHOD_EXIT, ("[%s] Trg Method Exit triggered %s.%s",
-                     JvmtiTrace::safe_get_thread_name(thread),
-                     (mh() == NULL) ? "NULL" : mh()->klass_name()->as_C_string(),
-                     (mh() == NULL) ? "NULL" : mh()->name()->as_C_string() ));
-
   JvmtiThreadState *state = thread->jvmti_thread_state();
+
   if (state == NULL || !state->is_interp_only_mode()) {
     // for any thread that actually wants method exit, interp_only_mode is set
     return;
@@ -1578,13 +1574,11 @@ void JvmtiExport::post_method_exit(JavaThread *thread, Method* method, frame cur
   // return a flag when a method terminates by throwing an exception
   // i.e. if an exception is thrown and it's not caught by the current method
   bool exception_exit = state->is_exception_detected() && !state->is_exception_caught();
-
+  Handle result;
+  jvalue value;
+  value.j = 0L;
 
   if (state->is_enabled(JVMTI_EVENT_METHOD_EXIT)) {
-    Handle result;
-    jvalue value;
-    value.j = 0L;
-
     // if the method hasn't been popped because of an exception then we populate
     // the return_value parameter for the callback. At this point we only have
     // the address of a "raw result" and we just call into the interpreter to
@@ -1594,9 +1588,36 @@ void JvmtiExport::post_method_exit(JavaThread *thread, Method* method, frame cur
       BasicType type = current_frame.interpreter_frame_result(&oop_result, &value);
       if (is_reference_type(type)) {
         result = Handle(thread, oop_result);
+        value.l = JNIHandles::make_local(thread, result());
       }
     }
+  }
 
+  // Deferred transition to VM, so we can stash away the return oop before GC
+  // Note that this transition is not needed when throwing an exception, because
+  // there is no oop to retain.
+  JRT_BLOCK
+    post_method_exit_inner(thread, mh, state, exception_exit, current_frame, value);
+  JRT_BLOCK_END
+
+  if (result.not_null() && !mh->is_native()) {
+    // We have to restore the oop on the stack for interpreter frames
+    *(oop*)current_frame.interpreter_frame_tos_address() = result();
+  }
+}
+
+void JvmtiExport::post_method_exit_inner(JavaThread* thread,
+                                         methodHandle& mh,
+                                         JvmtiThreadState *state,
+                                         bool exception_exit,
+                                         frame current_frame,
+                                         jvalue& value) {
+  EVT_TRIG_TRACE(JVMTI_EVENT_METHOD_EXIT, ("[%s] Trg Method Exit triggered %s.%s",
+                                           JvmtiTrace::safe_get_thread_name(thread),
+                                           (mh() == NULL) ? "NULL" : mh()->klass_name()->as_C_string(),
+                                           (mh() == NULL) ? "NULL" : mh()->name()->as_C_string() ));
+
+  if (state->is_enabled(JVMTI_EVENT_METHOD_EXIT)) {
     JvmtiEnvThreadStateIterator it(state);
     for (JvmtiEnvThreadState* ets = it.first(); ets != NULL; ets = it.next(ets)) {
       if (ets->is_enabled(JVMTI_EVENT_METHOD_EXIT)) {
@@ -1607,9 +1628,6 @@ void JvmtiExport::post_method_exit(JavaThread *thread, Method* method, frame cur
 
         JvmtiEnv *env = ets->get_env();
         JvmtiMethodEventMark jem(thread, mh);
-        if (result.not_null()) {
-          value.l = JNIHandles::make_local(thread, result());
-        }
         JvmtiJavaThreadEventTransition jet(thread);
         jvmtiEventMethodExit callback = env->callbacks()->MethodExit;
         if (callback != NULL) {
@@ -1801,7 +1819,9 @@ void JvmtiExport::notice_unwind_due_to_exception(JavaThread *thread, Method* met
       if(state->is_interp_only_mode()) {
         // method exit and frame pop events are posted only in interp mode.
         // When these events are enabled code should be in running in interp mode.
-        JvmtiExport::post_method_exit(thread, method, thread->last_frame());
+        jvalue no_value;
+        no_value.j = 0L;
+        JvmtiExport::post_method_exit_inner(thread, mh, state, true, thread->last_frame(), no_value);
         // The cached cur_stack_depth might have changed from the
         // operations of frame pop or method exit. We are not 100% sure
         // the cached cur_stack_depth is still valid depth so invalidate

src/hotspot/share/prims/jvmtiExport.hpp

@@ -193,6 +193,13 @@ class JvmtiExport : public AllStatic {
   // dependency information is complete or not.
   static bool _all_dependencies_are_recorded;
 
+  static void post_method_exit_inner(JavaThread* thread,
+                                     methodHandle& mh,
+                                     JvmtiThreadState *state,
+                                     bool exception_exit,
+                                     frame current_frame,
+                                     jvalue& value);
+
  public:
   inline static bool has_redefined_a_class() {
     JVMTI_ONLY(return _redefinition_count != 0);