RFR: 8224852: JVM crash on watched field access from native code [v2]

[Leonid Mesnik]

On Tue, 21 Oct 2025 07:35:42 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fixed comment
>
> src/hotspot/share/prims/jvmtiExport.cpp line 2226:
> 
>> 2224:     javaVFrame *jvf = thread->last_java_vframe(&reg_map);
>> 2225:     method = jvf->method();
>> 2226:     address = jvf->method()->code_base();
> 
> Nit: I'm thinking if this code can be simplified a little bit if the same approach with `reg_map` can be used for interpreter frame as well.

Thanks for suggestion, yes it works. The same approach might be used for compiler/interpreter frame.

> test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/TestFieldsEventsFromJNI.java line 29:
> 
>> 27:  * @bug 8224852
>> 28:  * @run main/othervm/native -agentlib:JvmtiFieldEventsFromJNI TestFieldsEventsFromJNI
>> 29:  * @run main/othervm/native -agentlib:JvmtiFieldEventsFromJNI -Xcomp TestFieldsEventsFromJNI
> 
> Nit: It is convenient when the test folder and the Java file have the same name. Many tests in the `serviceability/jvmti` folder follow this rule. So, I'd suggest to get rid of the `Test` prefix in this class name. An additional advantage would be that the class name is shorter. :)

We have all 3 different patterns:

jvmtiFieldEventsFromJNI/TestvmtiFieldEventsFromJNI.java
jvmtiFieldEventsFromJNI/jvmtiFieldEventsFromJNI.java
jvmtiFieldEventsFromJNI/jvmtiFieldEventsFromJNITest.java

and somtimes

jvmtiFieldEventsFromJNITest.java
TestvmtiFieldEventsFromJNI.java

while 
`jvmtiFieldEventsFromJNI/`
 directory contains native path only.

The 'Test' prefix or suffix useful to quickly find test entry if it has more then on java file.

Do we want to use this convention for serviceability/jvmti tests?

jvmtiFieldEventsFromJNI/
jvmtiFieldEventsFromJNI/jvmtiFieldEventsFromJNI.java
jvmtiFieldEventsFromJNI/libJvmtiFieldEventsFromJNI.jcpp



If there are no objections, I'll rename the test.

> test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libJvmtiFieldEventsFromJNI.cpp line 43:
> 
>> 41:   jni->DeleteLocalRef(object_class);
>> 42:   return obj_class_name;
>> 43: }
> 
> Nit: Would it be good to add this into `jvmti_common.hpp`?

Done.

> test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldEventsFromJNI/libJvmtiFieldEventsFromJNI.cpp line 102:
> 
>> 100:   deallocate(jvmti,jni, f_name);
>> 101: 
>> 102: 
> 
> Nit: Unneeded empty lines: 68, 102 and 137.

done

> test/hotspot/jtreg/serviceability/jvmti/events/FieldAccess/FieldsEventsFromJNI/FieldsEventsFromJNI.java line 48:
> 
>> (failed to retrieve contents of file, check the PR for context)
> Nit: Is this really needed? In fact, we have it in many tests though.

Yes, it is needed for tests that have native methods to communicate with agent. This test has:

    private native void enableEventsAndAccessField(boolean isEventExpected, Thread eventThread);
    private native void enableEventsAndModifyField(boolean isEventExpected, Thread eventThread);

> test/lib/jdk/test/lib/jvmti/jvmti_common.hpp line 335:
> 
>> 333:   check_jvmti_status(jni, err, "get_field_name: error in JVMTI GetFieldName call");
>> 334:   deallocate(jvmti,jni, signature);
>> 335:   deallocate(jvmti,jni, generic);
> 
> Nit: The lines 334 and 335 miss space after comma in the argument lists. Also, it is better to pass `nullptr` instead of `&generic`, so there would not be need to have this local and to deallocate the returned string.

Thanks, updated.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2448930076
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2448918343
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2448924000
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2448925404
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2448922851
PR Review Comment: https://git.openjdk.org/jdk/pull/27584#discussion_r2448926538