RFR: 8340586: JdkJfrEvent::get_all_klasses stores non-strong oops in JNI handles

[Coleen Phillimore]

On Tue, 5 Nov 2024 06:34:02 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

> JdkJfrEvent::get_all_klasses walks the class hierarchy of the Event class to find all event sub klasses. The mirrors of these classes are then stored in JNI handles and eventually transfered to an arraylist which is returned to the java caller.
> 
> Because the klasses are found by walking the class hierarchy, their is no guarantee that the klass is strongly reachable, and thus that the java_mirror load is strong. So we must explicitly keep the klass alive (and not have a safepoint between the load and the store into the object graph).
> 
> Having both `fill_klasses` and `transform_klasses_to_local_jni_handles` made the call to `keep_alive` and the comment explaining the reason why somewhat obscured, so I merged them. (The split seems to have served no purpose except to introduce the need for a lot of const and reinterpret casts.)

This looks good and thanks for the cleanup of the jfr code.  I went through this code for either this bug or another one, and thought it looked okay.  We should look for other uses of java_mirror that assume that having a reference to it keeps the CLD alive.

src/hotspot/share/oops/klass.inline.hpp line 44:

> 42:   static_cast<void>(klass_holder());
> 43: }
> 44: 

Can you add a comment to Klass::java_mirror() that says it doesn't keep the loader alive, "see above".

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

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21893#pullrequestreview-2415590248
PR Review Comment: https://git.openjdk.org/jdk/pull/21893#discussion_r1829330520