RFR: 8314225: SIGSEGV in JavaThread::is_lock_owned [v9]

[Kevin Walls]

On Wed, 8 May 2024 07:57:08 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

>> Removal of JavaThread's MonitorChunks member.  This held lock information during deoptimization, but access to it is unnecessary for anything other than the deoptimization itself.
>> 
>> Access to it in is_lock_owned() was racy, and caused rare crashes.
>
> Kevin Walls has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 14 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'upstream/master' into 8314225_is_lock_owned_no_monitor_chunks_check
>  - Add back the null checks in unpack_on_stack asserts
>  - Merge remote-tracking branch 'upstream/master' into 8314225_is_lock_owned_no_monitor_chunks_check
>  - fill_in assert update
>  - JavaThread comment update and synchronizer check before cast
>  - monitor->owner() == nullptr handling in fill_in
>  - Missing include
>  - Move is_lock_owned from Thread to JavaThread
>  - Remove JavaThread's is_lock_owned
>  - Feedback from Dean
>  - ... and 4 more: https://git.openjdk.org/jdk/compare/e2822004...f4fe65d8

On the null checks in the unpack_on_stack asserts: I'm putting them back 8-)

In vframearrayelememnt::fill_in we itereate MonitorInfo*
That gives us the is_scalar_replaced information, which means the MonitorInfo has a null owner(),
and there is an incoming param realloc_failures which can tell us if something in the preceeding Deoptimization::realloc_objects call had an allocation failure, so something has a null oop (which could be a monitor owner I think we can say).

If the MonitorInfo is_scalar_replaced is true, then we do dest->set_obj(nullptr);

That should leave vframeArrayElement::unpack_on_stack seeing a null src->obj() where we iterate at line 315.

In unpack_on_stack there is no realloc_failures flag, and we aren't using a MonitorInfo so we don't have is_scalar_replaced to check.  It just has a BasicObjectLock that it pulls from the MonitorChunks array (and those aren't specifically initialized, other than what we set in fill_in).

unpack_on_stack has always called  src->lock()->move_to(src->obj(), top->lock());  without checking src->obj(), and this works, but there are decisions in move_to that make that work.  There should be a follow-up bug on making sure this is works intentionally, which I can log.

So should there be a src->obj() == nullptr check in the unpack_on_stack asserts?  Looks like yes.

assert(src->obj() == nullptr || ObjectSynchronizer::current_thread_holds_lock(thread, Handle(thread, src->obj())),  "should be held, before/after move_to");

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

PR Comment: https://git.openjdk.org/jdk/pull/18940#issuecomment-2099981747