RR(XXS) 8023478: Test fails with HS crash in GCNotifier.

[Kevin Walls]

Hi Staffan --

Yes, I made some attempts but didn't reproduce it, though anything that 
allocates can presumably cause an exception if the timing and memory 
usage is right (or wrong).

On the clearing of exceptions...  All about not crashing the Service 
Thread I suppose, but it may not have been obivous where if anywhere any 
exceptions should be logged: should the running app's output be 
interrupted with exceptions because somebody attached jconsole to 
monitor it?... (that's one way of getting these GC notifications turned on).


I'm thinking the call to createGcInfo was just an oversight, the calls 
such as for String creation are done with CHECK, and yes any failures 
there would get caught and cleared by the caller, sendNotification, as 
you mention.

Thanks
Kevin


On 05/09/13 14:35, Staffan Larsen wrote:
> I agree that your change looks good, but there are a couple of things that bug me.
>
> Why did createGcInfo() fail? Hard to tell if you can't repro the failure, though.
>
> The code below will make sure we swallow any exceptions that occurred without reporting them. This isn't good practice. I would like these exceptions to be logged somehow. This isn't your code or part of your change, it just makes me worried.
>
> void GCNotifier::sendNotification(TRAPS) {
>    GCNotifier::sendNotificationInternal(THREAD);
>    // Clearing pending exception to avoid premature termination of
>    // the service thread
>    if (HAS_PENDING_EXCEPTION) {
>      CLEAR_PENDING_EXCEPTION;
>    }
> }
>
>
> /Staffan
>
> On 5 sep 2013, at 15:12, Kevin Walls <kevin.walls at oracle.com> wrote:
>
>> Hi,
>>
>> If I have my CHECK and THREAD thinking straight, I have a small review request in gcNotifier, to avoid a crash that came up in testing recently.
>>
>> The report is a hotspot crash in a test, where there's an exception pending when calling java_lang_String::create_from_str, which allocates.
>>
>> We are in GCNotifier::sendNotificationInternal  and before the call that crashes, we called createGcInfo which have several allocations can return a null handle if an exception occurs.
>>
>> If we have returned from createGcInfo, which returns null for an exception,
>> we need to return rather than continue in sendNotificationInternal, which will try to allocate again.
>>
>> sendNotificationInternal has a caller that clears any exception.
>>
>> So that should be a very small change:
>>
>> $ hg diff src/share/vm/services/gcNotifier.cpp
>> diff --git a/src/share/vm/services/gcNotifier.cpp b/src/share/vm/services/gcNotifier.cpp
>> --- a/src/share/vm/services/gcNotifier.cpp
>> +++ b/src/share/vm/services/gcNotifier.cpp
>> @@ -209,7 +209,7 @@
>>    GCNotificationRequest *request = getRequest();
>>    if (request != NULL) {
>>      NotificationMark nm(request);
>> -    Handle objGcInfo = createGcInfo(request->gcManager, request->gcStatInfo, THREAD);
>> +    Handle objGcInfo = createGcInfo(request->gcManager, request->gcStatInfo, CHECK);
>>
>>      Handle objName = java_lang_String::create_from_str(request->gcManager->name(), CHECK);
>>      Handle objAction = java_lang_String::create_from_str(request->gcAction, CHECK);
>> kwalls at klaptop:~/work/bugs/8023478.gcbean/hotspot-rt$
>>
>>
>>
>> http://bugs.sun.com/view_bug.do?bug_id=8023478
>> http://cr.openjdk.java.net/~kevinw/8023478/webrev.00/ (the above one line)
>>
>>
>> Thanks
>> Kevin
>> --------
>> PS
>> As the bug is not available on the bugs site at the moment, this is part of the crashing stack to give some context:
>>
>> V [libjvm.so+0x7069d0] void report_vm_error(const char*,int,const char*,const char*)+0x78;; __1cPreport_vm_error6Fpkci11_v_+0x78
>> V [libjvm.so+0x63125c] void CollectedHeap::check_for_valid_allocation_state()+0x4c;; __1cNCollectedHeapbGcheck_for_valid_allocation_state6F_v_+0x4c
>> V [libjvm.so+0x911f68] instanceOop InstanceKlass::allocate_instance(Thread*)+0x78;; __1cNInstanceKlassRallocate_instance6MpnGThread__nLinstanceOop__+0x78
>> V [libjvm.so+0x9ffd7c] Handle java_lang_String::basic_create(int,Thread*)+0x174;; __1cQjava_lang_StringMbasic_create6FipnGThread__nGHandle__+0x174
>> V [libjvm.so+0xa00520] Handle java_lang_String::create_from_str(const char*,Thread*)+0x40;; __1cQjava_lang_StringPcreate_from_str6FpkcpnGThread__nGHandle__+0x40
>> V [libjvm.so+0x83eb0c] void GCNotifier::sendNotificationInternal(Thread*)+0x8c;; __1cKGCNotifierYsendNotificationInternal6FpnGThread__v_+0x8c
>> V [libjvm.so+0x83ea4c] void GCNotifier::sendNotification(Thread*)+0xc;; __1cKGCNotifierQsendNotification6FpnGThread__v_+0xc
>> V [libjvm.so+0x103faf8] void ServiceThread::service_thread_entry(JavaThread*,Thread*)+0x4a0;; __1cNServiceThreadUservice_thread_entry6FpnKJavaThread_pnGThread__v_+0x4a0
>>
>>