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

[Daniel D. Daugherty]

Thanks Serguei!

I happen to be doing a test run this weekend that includes most of the
JPDA stack of tests so I'll include the following in my experiment:

$ hg log -v -r tip
changeset:   12872:bb66cd7c61b1
tag:         8185164.patch
tag:         qtip
tag:         tip
user:        dcubed
date:        Fri Aug 04 13:41:29 2017 -0600
files:       src/share/vm/runtime/objectMonitor.cpp
description:
imported patch 8185164.patch


That will get the product code changes a complete round of testing
on Solaris X64 at least... :-)

Dan


On 8/4/17 1:31 PM, serguei.spitsyn at oracle.com wrote:
> 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
>>>>>>>
>>>>
>>
>