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

[David Holmes]

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.

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

test/hotspot/jtreg/vmTestbase/nsk/jvmti/StopThread/stopthrd006/TestDescription.java

The copyright year should be change to "2018, 2020,".

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

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