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

[serguei.spitsyn at oracle.com]

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

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 



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