On Mon, 4 Dec 2023 18:13:50 GMT, Daniel D. Daugherty <dcu...@openjdk.org> wrote:
>> Initially I thought this was not the right fix as we should not be exposing >> an attaching thread that may still have a partially constructed `threadObj`. >> But this issue shows that we must expose such a thread because the >> constructor of the `Thread` object can trigger these events on the current >> thread so it must have a valid JVMTI state! >> >> Thanks. > > @dholmes-ora - Thanks for the review. > @dcubed-ojdk and @dholmes-ora, is there a case > JvmtiVMObjectAllocEventCollector::JvmtiVMObjectAllocEventCollector() > might also see an attaching thread without the thread obj allocated? JvmtiVMObjectAllocEventCollector is the code path that we ran into with the internal stress test, but in the case that tripped, we had a thread obj allocated. If we ran the same code path without the thread obj allocated, then we would return `nullptr` which was the goal of your original fix. I don't know if that specific test case is actually reached by any of our tests, but if such a case occurred, it did not result in the guarantee() firing or any other failure mode that I saw. The `guarantee(state != nullptr) failed: exiting thread called setup_jvmti_thread_state` has not been seen in Mach5 Tier[1-8] testing with this fix in place and I didn't see any other test failures that could be tracked back to problems with this code. A JvmtiVMObjectAllocEventCollector object is created in 34 places in jvm.cpp alone and I haven't checked all of those. I only checked out the one use in JVM_GetStackAccessControlContext. ------------- PR Comment: https://git.openjdk.org/jdk/pull/16934#issuecomment-1839456136
