RFR 4505697: nsk/jdi/ExceptionEvent/_itself_/exevent006 and exevent008 tests fail with InvocationTargetException

[Jaroslav Bachorik]

On 20.2.2014 11:40, David Holmes wrote:
> Hi Jaroslav,
>
> instanceKlass.cpp:
>
> Comment is wrong:
>
> 913     // JVMTI internal flag reset is needed in order to report
> InvocationTargetException
>
> It will be ExceptionInInitializerError

Will fix. Copypaste ...

>
> You added this:
>
>   917
> this_oop->set_initialization_state_and_notify(initialization_error,
> THREAD);
>    918       CLEAR_PENDING_EXCEPTION;   // ignore any exception thrown,
> class initialization error is thrown below
> + 919       // JVMTI has already reported the pending exception
> + 920       // JVMTI internal flag reset is needed in order to report
> InvocationTargetException
> + 921       JvmtiExport::clear_detected_exception((JavaThread*)THREAD);
>
> but there are a number of places where
> set_initialization_state_and_notify is called when a pending exception
> has been cleared, and then CLEAR_PENDING_EXCEPTION is called again, but
> you didn't modify those other locations. They will rethrow the original
> exception so I suppose that is okay from JVMTI's perspective. But the
> flip-side of this is that if set_initialization_state_and_notify does
> throw an exception, JVMTI will never see it.

I don't know if it supposed to see it. It seems that any exception 
thrown from set_initialization_state_and_notify is thoroughly ignored 
and hidden from the outer world. Perhaps someone more experienced in 
JVMTI than me would like to chime in here? Serguei?

-JB-

>
> ---
>
> jvm.cpp
>
> Comment is wrong again - not InvocationTargetException.
>
> ---
>
> David
> ------
>
>
>
>
> On 20/02/2014 1:59 AM, Jaroslav Bachorik wrote:
>> On 18.2.2014 11:18, serguei.spitsyn at oracle.com wrote:
>>> On 2/17/14 12:04 AM, Jaroslav Bachorik wrote:
>>>> On 14.2.2014 23:13, serguei.spitsyn at oracle.com wrote:
>>>>> On 2/14/14 12:33 PM, Daniel D. Daugherty wrote:
>>>>>> On 2/14/14 11:46 AM, serguei.spitsyn at oracle.com wrote:
>>>>>>> Jaroslav,
>>>>>>>
>>>>>>> It looks good in general modulo indent comments from Dan.
>>>>>>>
>>>>>>> But I have a doubt that acquiring the JvmtiThreadState_lock is
>>>>>>> needed
>>>>>>> or right thing to do in the JvmtiExport::clear_detected_exception().
>>>>>>> It seems, both clear_exception_detected() and
>>>>>>> set_exception_detected() are always
>>>>>>> called on current thread and so, it has to be safe to do without
>>>>>>> acquiring any locks.
>>>>>>
>>>>>> My JVM/TI-foo is rusty, but I believe that JvmtiThreadState stuff
>>>>>> can also be queried/modified by other threads so grabbing the
>>>>>> associated lock is a good idea.
>>>>>
>>>>> The lock synchronization is cooperative.
>>>>> It does not help much if the lock is not acquired in other places.
>>>>> I can be wrong, but I've not found yet any place in the code where the
>>>>> clear_exception_detected() and set_exception_detected() are called
>>>>> under protection of the JvmtiThreadState_lock.
>>>>
>>>> I copied the locking over from
>>>> "JvmtiExport::cleanup_thread(JavaThread* thread)". That method is also
>>>> supposed to work only with the current thread but acquires the lock
>>>> nonetheless. But if you are sure that the lock is not required I have
>>>> no objections removing it.
>>>
>>> I'm suggesting to remove it, as it is not used in other places in the
>>> code.
>>> It is going to be confusing if it is used in one place and missed in
>>> others.
>>
>> I've removed the lock and applied the same cleanup logic to other places
>> where exceptions are rewrapped.
>>
>> Webrev: http://cr.openjdk.java.net/~jbachorik/4505697/webrev.02
>> JPRT run:
>> http://prt-web.us.oracle.com//archive/2014/02/2014-02-19-114618.jbachorik.hotspot/
>>
>>
>> Aurora Adhoc:
>> http://aurora.ru.oracle.com//faces/Batch.xhtml?batchName=418853.VMSQE.adhoc.JPRT.full
>>
>> (still running at the moment; no failures so far)
>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>> -JB-
>>>>
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> And I'm repeating my question about pre-integration testing (Dan is
>>>>>>> asking about the same).
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>>
>>>>>>>
>>>>>>> On 2/14/14 3:07 AM, Jaroslav Bachorik wrote:
>>>>>>>> This is a round-0 review request.
>>>>>>>>
>>>>>>>> The reflection code intercepting the exceptions thrown in the
>>>>>>>> invoked methods does not play nicely with JVMTI (which, in this
>>>>>>>> case, propagates to JDI).
>>>>>>>>
>>>>>>>> The reflection code lacks the traditional error handler -
>>>>>>>> therefore,
>>>>>>>> upon throwing the NumberFormatException, the stack is searched for
>>>>>>>> appropriate handlers and none are found. This leaves the
>>>>>>>> "exception_detected" flag set to true while normally it would be
>>>>>>>> reset to false once the exception is handled. The reflection code
>>>>>>>> then goes on and wraps the NumberFormatException into
>>>>>>>> InvocationTargetException and throws it. But, alas, the
>>>>>>>> "exception_detected" flag is still set to true and no JVMTI
>>>>>>>> exception event will be sent out.
>>>>>>>>
>>>>>>>> The proposed solution is to call
>>>>>>>> thread->jvmti_thread_state()->clear_exception_detected() at the
>>>>>>>> appropriate places in the reflection code to reset the
>>>>>>>> "exception_detected" flag and enable the InvocationTargetException
>>>>>>>> be properly reported over JVMTI.
>>>>>>>>
>>>>>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-4505697
>>>>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/4505697/webrev.00
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>> -JB-
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>