David, > Don't you need to execute the END_WITH_LOCAL_REFS before you can return?
done. http://cr.openjdk.java.net/~dsamersoff/JDK-8030708/webrev.04/ -Dmitry On 2015-01-28 10:24, David Holmes wrote: > Hi Dmitry, > > Sorry this slipped through the cracks. > > On 21/01/2015 11:05 PM, Dmitry Samersoff wrote: >> David, >> >> Please, take a look at updated webrev >> >> http://cr.openjdk.java.net/~dsamersoff/JDK-8030708/webrev.03/ > > src/jdk.jdwp.agent/share/native/libjdwp/StringReferenceImpl.c. > > Don't you need to execute the END_WITH_LOCAL_REFS before you can return? > > Otherwise seems okay. > > Thanks, > David > >> -Dmitry >> >> >> On 2014-10-16 16:07, David Holmes wrote: >>> Hi Dmitry, >>> >>> On 16/10/2014 8:08 PM, Dmitry Samersoff wrote: >>>> David, >>>> >>>> Changed. Thank you for review! >>>> >>>> please, see: >>>> >>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8030708/webrev.02/ >>> >>> 102 if (weakRef == NULL || (*env)->ExceptionCheck(env)) { >>> >>> Isn't the only time it will return NULL when an exception occurs? >>> Conversely if an exception occurs then it must return NULL - so the >>> exception check seems redundant. >>> >>> But this also suggests you need similar logic at: >>> >>> 182 weakRef = JNI_FUNC_PTR(env,NewWeakGlobalRef)(env, >>> node->ref); >>> >>> 456 lref = JNI_FUNC_PTR(env,NewLocalRef)(env, >>> node->ref); >>> >>> Or more generally any JNI call from JVMTI should be wrapped in a way >>> that checks for exceptions and clears them. >>> >>> David >>> >>>> -Dmitry >>>> >>>> On 2014-10-16 04:24, David Holmes wrote: >>>>> On 16/10/2014 12:33 AM, Dmitry Samersoff wrote: >>>>>> David, >>>>>> >>>>>> Sorry, copied wrong function! >>>>>> >>>>>> I mean this call >>>>>> >>>>>> weakRef = JNI_FUNC_PTR(env,NewWeakGlobalRef)(env, ref); >>>>>> >>>>>> that can post OutOfMemoryError >>>>> >>>>> Okay, so shouldn't that be where the exception is cleared: >>>>> >>>>> /* Create weak reference to make sure we have a reference */ >>>>> weakRef = JNI_FUNC_PTR(env,NewWeakGlobalRef)(env, ref); >>>>> if (weakRef == NULL) { >>>>> + // < clear exception here > >>>>> jvmtiDeallocate(node); >>>>> return NULL; >>>>> } >>>>> >>>>> Thanks, >>>>> David >>>>> ----- >>>>> >>>>>> commonRef_refToID() -> >>>>>> >>>>>> createNode(JNIEnv *env, jobject ref) -> >>>>>> >>>>>> weakRef = JNI_FUNC_PTR(env,NewWeakGlobalRef)(env, ref); >>>>>> >>>>>> -Dmitry >>>>>> >>>>>> On 2014-10-15 16:21, David Holmes wrote: >>>>>>> On 15/10/2014 8:39 PM, Dmitry Samersoff wrote: >>>>>>>> On 2014-10-15 14:27, David Holmes wrote: >>>>>>>>> On 15/10/2014 8:08 PM, Dmitry Samersoff wrote: >>>>>>>>>> Please review the fix: >>>>>>>>>> >>>>>>>>>> http://cr.openjdk.java.net/~dsamersoff/JDK-8030708/webrev.01/ >>>>>>>>>> >>>>>>>>>> Added missed exception checks. >>>>>>>>> >>>>>>>>> src/jdk.jdwp.agent/share/native/libjdwp/outStream.c >>>>>>>>> >>>>>>>>> What is potentially posting the exception? >>>>>>>> >>>>>>>> JvmtiEnv::GetTag(jobject object, jlong* tag_ptr) called from >>>>>>>> commonRef_refToID() >>>>>>> >>>>>>> You mean this call: >>>>>>> >>>>>>> error = JVMTI_FUNC_PTR(gdata->jvmti,GetTag)(gdata->jvmti, ref, >>>>>>> &tag); >>>>>> >>>>>> x >>>>>>> >>>>>>> in findNodeByRef which is called by commonRef_refToID? JVM TI >>>>>>> doesn't >>>>>>> post exceptions. >>>>>>> >>>>>>> "JVM TI functions never throw exceptions; error conditions are >>>>>>> communicated via the function return value. Any existing exception >>>>>>> state >>>>>>> is preserved across a call to a JVM TI function." >>>>>>> >>>>>>> http://docs.oracle.com/javase/8/docs/platform/jvmti/jvmti.html >>>>>>> >>>>>>> David >>>>>>> >>>>>>>> -Dmitry >>>>>>>> >>>>>>>> >>>>>> >>>>>> >>>> >>>> >> >> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
