C1 code installation and JNIHandle::deleted_handle() oop

[Vladimir Ivanov]

>> Looking at the crash log, the problematic code is under assert:
>>
>> void ConstantOopWriteValue::write_on(DebugInfoWriteStream* stream) {
>>    assert(JNIHandles::resolve(value()) == NULL ||
>>           Universe::heap()->is_in_reserved(JNIHandles::resolve(value())),
>>           "Should be in heap");
>>    stream->write_int(CONSTANT_OOP_CODE);
>>    stream->write_handle(value());
>> }
>>
>> So, the proper fix would be to make the verification code more robust.
> 
> Actually, in Shenandoah case, the failure is here (it is obscured by inlining, but visible in
> slowdebug):
>    http://hg.openjdk.java.net/jdk/hs/file/5caa1d5f74c1/src/hotspot/share/runtime/jniHandles.hpp#l220
> 
> ... "== deleted_handle()" or "!= deleted_handle()". So fortifying assert code does not help, the bug
> is touching the oop in improper thread state. Nightmarish scenario would be to fail "!="/"==" check
> because the deleted_handle oop had been already updated, but the actual handle value did not.

> The similar thing happens after we write the oop into the debug stream: it is not handelize-d
> anymore, and so opaque for GC, and thus has all the chance to be garbled by the time GC finishes.

I don't follow you. Can you point to the code you are talking about?

ConstantOopWriteValue::write_on() writes the _handle_ and not the naked 
oop [1].

I still believe the assert is the only problem here. If it's valuable to 
keep it, then it should perform all necessary precautions itself before 
accessing the oop.

The assert is a relatively recent addition. It was added as part of 
PermGen removal and most likely the fact it isn't executed in VM was 
overlooked.

Best regards,
Vladimir Ivanov

[1]

void ConstantOopWriteValue::write_on(DebugInfoWriteStream* stream) {
...
stream->write_handle(value());

...

class ConstantOopWriteValue: public ScopeValue {
  private:
   jobject _value;
  public:
   jobject value() const                { return _value;  }
...

void DebugInfoWriteStream::write_handle(jobject h) {
   write_int(recorder()->oop_recorder()->find_index(h));
}


Best regards,
Vladimir Ivanov

> Having these two thoughts in mind, I think we have to consider _thread_in_vm on that C1 path. But
> where exactly?
> 
> Thanks,
> -Aleksey
>