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

[Daniel D. Daugherty]

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