[10] RFR: 8185164: GetOwnedMonitorInfo() returns incorrect owned monitor

[serguei.spitsyn at oracle.com]

Dan,

Thank you for letting me know about this discussion.
I'll try to convert the attached test case to the JTreg format.

Thanks,
Serguei


On 8/4/17 11:16, Daniel D. Daugherty wrote:
> Adding Serguei to this thread directly since he's back from vacation!
>
>
> On 7/31/17 10:14 PM, David Holmes wrote:
>> Hi Dan,
>>
>> On 26/07/2017 11:52 PM, Daniel D. Daugherty wrote:
>>> On 7/26/17 12:11 AM, David Holmes wrote:
>>>> On 26/07/2017 10:27 AM, Yasumasa Suenaga wrote:
>>>>> Hi Dan,
>>>>>
>>>>>> I've added some analysis to the bug report
>>>>>
>>>>> Thanks!
>>>>> I tried to fix this issue in JvmtiEnvBase::get_owned_monitors() at 
>>>>> first.
>>>>> But it is difficult because we cannot know pending monitor if 
>>>>> thread state is MONITOR_CONTENDED_ENTER when get_owned_monitor() 
>>>>> is called.
>>>>
>>>> I need to look closer at this when I get back from vacation next week.
>>>
>>> Seems like you're back already. :-)
>>>
>>>
>>>> A pending monitor should not be reported as owned (unless the spec 
>>>> says otherwise) and it seems odd to me to fix the current problem 
>>>> by marking the monitor as pending earlier.
>>>
>>> It's the updating of the _current_pending_monitor field that
>>> allows JvmtiEnvBase::get_locked_objects_in_frame() to recognize
>>> that the monitor observed in the frame is only pending and
>>> is not owned.
>>>
>>> I put a fairly detailed note in the bug yesterday, but you
>>> should look at that when you're officially back!
>>
>> Thanks for clarifying things. I also added a comment to the bug report.
>>
>> I think the fix is sound and prevents anyone from observing the case 
>> where the monitor will be seen in the stack-frame, but has not yet 
>> been set as the "pending monitor". As far as I can tell it is only 
>> this case (GetOwnedMonitorInfo from the contended-monitor event 
>> callback in the current thread) that will be able to observe the change.
>
> One scenario that I worry about here is that a 
> GetCurrentContendedMonitor()
> call on a target thread will now be able to return a non-NULL value 
> for the
> object, when GetThreadState() will be able to return something other than
> blocked (on monitor enter) for the thread.
>
> I don't see anything in the JVM/TI spec that says such a scenario is
> wrong; I'm only worried about whether we have any tests that would catch
> this slight change in behavior. In any case, one of these operations has
> to "happen first":
>
>   - thread is marked as blocked
>   - monitor is flagged as contended
>
> Currently, they happen in the above order and the fix proposes to
> change the order and I see no reason not to do it.
>
> I would like the test attached to the bug to be converted into a native
> JTREG test that lives in hotspot/test/serviceability/jvmti. See the
> following test as a possible example:
>
>    hotspot/test/serviceability/jvmti/GetNamedModule
>
> for how to do this... I haven't done one of these new native JTREG
> tests myself, but I believe Serguei has...
>
> Dan
>
>
>>
>> Thanks,
>> David
>>
>>> Dan
>>>
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>> Did you run the jdk repo's com/sun/jdi tests with your fix?
>>>>>
>>>>> I have not done yet.
>>>>> I have a trip until 28 July JST. So I will run it after that.
>>>>>
>>>>>
>>>>> Yasumasa
>>>>>
>>>>>
>>>>> On 2017/07/26 7:05, Daniel D. Daugherty wrote:
>>>>>> On 7/24/17 8:40 PM, Yasumasa Suenaga wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I tried to get owned monitors in MonitorContendedEnter JVMTI 
>>>>>>> event handler.
>>>>>>> However GetOwnedMonitorInfo JVMTI function returns a monitor 
>>>>>>> which is
>>>>>>> not yet owned.
>>>>>>>
>>>>>>> I attached reproducer to JBS. Please read README.md.
>>>>>>>
>>>>>>> I think GetOwnedMonitorInfo() should not return a pending monitor.
>>>>>>>
>>>>>>>
>>>>>>> I uploaded webrev. Could you review?
>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8185164/webrev.00/
>>>>>>>
>>>>>>> I hope this fix is applied to 8u or later release.
>>>>>>> I cannot access JPRT. So I need a sponsor.
>>>>>>
>>>>>> Thanks for the bug report. It's nice to have a test case and a 
>>>>>> proposed
>>>>>> fix all in the bug report! I've added some analysis to the bug 
>>>>>> report
>>>>>> and we'll need to run this fix through Oracle's JPDA test stack 
>>>>>> which
>>>>>> is not (yet) open.
>>>>>>
>>>>>> Did you run the jdk repo's com/sun/jdi tests with your fix?
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Yasumasa
>>>>>>
>>>
>