David, Please, take a look at updated webrev
http://cr.openjdk.java.net/~dsamersoff/JDK-8030708/webrev.03/ -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.
