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

[Jaroslav Bachorik]

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-
>>>>>
>>>>
>>>
>>>
>>
>