RFR(XS): 8234882: JVM TI StopThread should only allow ThreadDeath

Fri May 29 07:56:07 UTC 2020

On 5/29/20 00:42, serguei.spitsyn at oracle.com wrote:
> Hi David,
>
> Thank you for reviewing this!
>
> On 5/28/20 23:57, David Holmes wrote:
>> Hi Serguei,
>>
>> On 28/05/2020 3:12 pm, serguei.spitsyn at oracle.com wrote:
>>> Hi David,
>>>
>>> I've updated the CSR and webrev in place.
>>>
>>> The changes are:
>>>   - addressed David's suggestion to rephrase StopThread description 
>>> change
>>>   - replaced JVMTI_ERROR_INVALID_OBJECT with 
>>> JVMTI_ERROR_ILLEGAL_ARGUMENT
>>>   - updated the implementation in jvmtiEnv.cpp to return 
>>> JVMTI_ERROR_ILLEGAL_ARGUMENT
>>>   - updated one of the nsk.jvmti StopThread tests to check error 
>>> case with the JVMTI_ERROR_ILLEGAL_ARGUMENT
>>>
>>>
>>> I'm reposting the links for convenience.
>>>
>>> Enhancement:
>>>    https://bugs.openjdk.java.net/browse/JDK-8234882
>>>
>>> CSR draft:
>>>    https://bugs.openjdk.java.net/browse/JDK-8245853
>>
>> Spec updates are good - thanks.
>
> Thank you for the CSR review.
>
>>> Webrev:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 
>>>
>>
>> src/hotspot/share/prims/jvmtiEnv.cpp
>>
>> The ThreadDeath check is fine but I'm a bit confused about the 
>> additional null check that leads to JVMTI_ERROR_INVALID_OBJECT. I 
>> can't see how resolve_external_guard can return NULL when not passed 
>> in NULL. Nor why that would result in JVMTI_ERROR_INVALID_OBJECT 
>> rather than JVMTI_ERROR_NULL_POINTER. And I note 
>> JVMTI_ERROR_NULL_POINTER is not even a listed error for StopThread! 
>> This part of the change seems unrelated to this issue.
>
> I was also surprised with the JVMTI_ERROR_NULL_POINTER and 
> JVMTI_ERROR_INVALID_OBJECT error codes.
> The JVM TI spec automatic generation adds these two error codes for a 
> jobject parameter.
>
> Also, they both are from the Universal Errors section:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#universal-error 
>
>
> You can find a link to this section at the start of the Error section:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 
>
>
> My understanding (not sure, it is right) is that NULL has to be 
> reported with JVMTI_ERROR_NULL_POINTER and a bad
> jobject (for instance, a WeakReference with a GC-ed target) has to be 
> reported with JVMTI_ERROR_INVALID_OBJECT.
> At least, I was not able to construct a test case to get this error 
> code returned.
> So, I'm puzzled with this. I'll try to find some examples with 
> JVMTI_ERROR_NULL_POINTER errors.

Found the explanation.
The JDI file:
   src/jdk.jdi/share/classes/com/sun/tools/jdi/JDWPException.java

has a fragment that translate the INVALID_OBJECT error to the 
ObjectCollectedException:
     RuntimeException toJDIException() {
         switch (errorCode) {
             case JDWP.Error.INVALID_OBJECT:
                 return new ObjectCollectedException();

So, the INVALID_OBJECT is for a jobject handle that is referencing a 
collected object.
It means that previous implementation incorrectly returned 
JVMTI_ERROR_NULL_POINTER error code.

Thanks,
Serguei

>> test/hotspot/jtreg/vmTestbase/nsk/jvmti/StopThread/stopthrd006/TestDescription.java 
>>
>>
>> The copyright year should be change to "2018, 2020,".
> Thank you for the catch.
> I planned to update the copyright comments.
>
>> I'm a little surprised the test doesn't actually check that a valid 
>> call doesn't produce an error. But that's an existing quirk of the 
>> test and not something you need to address here (if indeed it needs 
>> addressing - perhaps there is another test for that).
>
> There are plenty of other nsk.jvmti tests which check valid calls.
>
> Thanks,
> Serguei
>>
>> Thanks,
>> David
>> -----
>>
>>>
>>> Updated JVM TI StopThread spec:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 
>>>
>>>
>>>
>>> The old webrev and spec are here:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.0/
>>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>> On 5/27/20 18:03, serguei.spitsyn at oracle.com wrote:
>>>> Hi David,
>>>>
>>>>
>>>> On 5/27/20 02:00, David Holmes wrote:
>>>>> On 27/05/2020 6:36 pm, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi David,
>>>>>>
>>>>>>
>>>>>> On 5/27/20 00:47, David Holmes wrote:
>>>>>>> Hi Serguei,
>>>>>>>
>>>>>>> On 27/05/2020 1:01 pm, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Please, review a fix for:
>>>>>>>>    https://bugs.openjdk.java.net/browse/JDK-8234882
>>>>>>>>
>>>>>>>> CSR draft (one CSR reviewer is needed before finalizing it):
>>>>>>>>    https://bugs.openjdk.java.net/browse/JDK-8245853
>>>>>>>
>>>>>>> I have some thoughts on the wording which I will add to the CSR.
>>>>>>
>>>>>> Thank you a lot for looking at this!
>>>>>>
>>>>>>> Also on reflection I think JVMTI_ERROR_ILLEGAL_ARGUMENT would 
>>>>>>> the best error to use, and it has an equivalent in JDWP and at 
>>>>>>> the Java level for JDI.
>>>>>>
>>>>>> This is an interesting variant, thanks!
>>>>>> We need to balance on several criteria:
>>>>>>   1) Compatibility: keep returning error as close as possible to 
>>>>>> the current spec
>>>>>
>>>>> If you are adding a new error condition I don't understand what 
>>>>> you mean by "close to the current spec" ??
>>>>
>>>> If the JVMTI_ERROR_INVALID_OBJECT is returned than the JDWP agent 
>>>> does not need any new error handling.
>>>> The same can be true in the JDI if the JDWP returns the same error 
>>>> as it returned before.
>>>> In this case we do not add new error code but extend the existing 
>>>> to cover new error condition.
>>>>
>>>> But, in fact (especially, after rethinking), I do not like the 
>>>> JVMTI_ERROR_INVALID_OBJECT
>>>> error code as it normally means something different.
>>>> So, let's avoid using it and skip this criteria.
>>>> Then we need new error code to cover new error condition.
>>>>
>>>>>>   2) Best error naming match between JVM TI and JDI/JDWP
>>>>>>   3) Best practice in errors naming
>>>>>
>>>>> If the argument is not a ThreadDeath instance then it is an 
>>>>> illegal argument - perfect fit semantically all the specs involved 
>>>>> have an "illegal argument" error form.
>>>>
>>>> I agree with this.
>>>> It is why I like this suggestion. :)
>>>> The JDWP equivalent is: ILLEGAL_ARGUMENT.
>>>> The JDI equivalent is:  IllegalArgumentException
>>>>
>>>> I'll prepare and send the update.
>>>>
>>>> Thanks!
>>>> Serguei
>>>>
>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>>> I think the #1 is most important but will look at it once more.
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>>> Webrev:
>>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/src/ 
>>>>>>>>
>>>>>>>>
>>>>>>>> Updated JVM TI StopThread spec:
>>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2020/jvmti-stop-thread.1/docs/specs/jvmti.html#StopThread 
>>>>>>>>
>>>>>>>>
>>>>>>>> Summary:
>>>>>>>>
>>>>>>>>    The JVM TI StopThread method mirrored the functionality of the
>>>>>>>>    java.lang.Thread::stop(Throwable t) method, in that it 
>>>>>>>> allows any exception
>>>>>>>>    type to be installed as an asynchronous exception in the 
>>>>>>>> target thread.
>>>>>>>>    However, the java.lang.Thread::stop(Throwable t) method was 
>>>>>>>> inherently unsafe
>>>>>>>>    and in Java 8 (under JDK-7059085) it was "retired" so that 
>>>>>>>> it always threw
>>>>>>>>    UnsupportedOperationException.
>>>>>>>>    The updated JVM TI StopThread spec disallows an arbitrary 
>>>>>>>> Throwable from being passed,
>>>>>>>>    and instead restricts the argument to being an instance of 
>>>>>>>> ThreadDeath, thus
>>>>>>>>    mirroring the (deprecated but still functional) 
>>>>>>>> java.lang.Thread::stop() method.
>>>>>>>>    The error JVMTI_ERROR_INVALID_OBJECT is returned if the 
>>>>>>>> exception argument
>>>>>>>>    is not an instance of ThreadDeath.
>>>>>>>>
>>>>>>>>    Also, I will file similar RFE and CSR on the JDI and JDWP spec.
>>>>>>>>
>>>>>>>>
>>>>>>>> Testing:
>>>>>>>>    Built docs and checked the doc has been generated as expected.
>>>>>>>>    Will run the nsk.jvmti tests locally.
>>>>>>>>    Will submit hs-tiers1-3 to make sure there are no 
>>>>>>>> regressions in the JVM TI and JDI tests.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>
>>>>
>>>
>