RFR(S): 8134103: JVMTI_ERROR_WRONG_PHASE(112): on checking for an interface

[serguei.spitsyn at oracle.com]

The updated webrev:
http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8134103-jdi-wrong-phase.2/

The change at L152-L153 has been reverted.
Just one sanity check is needed.

Thanks,
Serguei


On 3/2/17 11:21, serguei.spitsyn at oracle.com wrote:
> Dan,
>
> Thank you for reviewing!
> I was waiting for your comments.
>
>
> On 3/2/17 06:59, Daniel D. Daugherty wrote:
>> On 3/1/17 8:49 PM, serguei.spitsyn at oracle.com wrote:
>>> Please, review the JDK 10 fix for:
>>>   https://bugs.openjdk.java.net/browse/JDK-8134103
>>>
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2017/hotspot/8134103-jdi-wrong-phase.1/ 
>>>
>>
>> src/jdk.jdwp.agent/share/native/libjdwp/debugLoop.c
>>     old L152:             } else if (gdata->vmDead &&
>>     old L153:              ((cmd->cmdSet) != 
>> JDWP_COMMAND_SET(VirtualMachine))) {
>>
>>         The old code used to set the error condition when the VM
>>         is dead and the command was not in the VirtualMachine
>>         command set. With the new code:
>>
>>             L150:             } else if (gdata->vmDead) {
>>
>>         The error condition is now set for all command sets
>>         including the VirtualMachine command set. Minimally
>>         that means that this comment needs work:
>>
>>             L152:                  * VirtualMachine cmdSet quietly 
>> ignores some cmds
>>             L153:                  * after VM death, so, it sends 
>> it's own errors.
>>
>>         since you are no longer letting the VirtualMachine cmdSet send
>>         its own errors.
>
> Agreed, good catch, thanks!
>
>
>>
>>         It's not clear to me why you are now setting the error
>>         condition for VirtualMachine cmds instead of letting those
>>         cmds send their own errors.
>>
>>         See more comments below for your summary.
>
> Most likely, you are right here.
> I specifically looked at the VM commands but overlooked the ones that 
> are silently ignored.
> For instance: Resume (9) or exit (10).
>
> So, I will need to restore the L152+L153.
> Please, let me re-test this update and then I'll send another webrev.
> Interesting that no test has caught this as there is a very tiny gap 
> for such an intermittent issue to appear.
>
>
>>
>> src/jdk.jdwp.agent/share/native/libjdwp/eventHelper.c
>>     L243:          * Immediately close out any commands enqueued from a
>>     L244:          * previously attached debugger.
>>
>>         Perhaps L244 can be change to:
>>
>>                    * dead VM or a previously attached debugger.
>>
>>         to match the changed line of code:
>>
>>             L246         if (gdata->vmDead || command->sessionID != 
>> currentSessionID) {
>>
>>>
>>>
>>> Summary:
>>>   This is an intermittent issue in the debugger back-end (JDWP agent)
>>>   that impacts the nightly testing stability.
>>
>> Congrats on tracking down this elusive bug!
>
> Thanks!
>
>>
>>
>>>   The fix adds check guards of gdata->vmDead condition to the:
>>>    - debugLoop (JDWP Event Helper thread) and
>>>    - commandLoop (JDWP Transport Listener thread)
>>>   The commands are ignored in the DEAD phase.
>>
>> In the debugLoop, you aren't ignoring the commands, you
>> are setting an error condition.
>
> Right, I meant: ignored == not executed. :)
>
>
>>
>>
>>>   The check guard in the debugLoop already existed but only for 
>>> VirtualMachine
>>>   command set, so it has been extended to commands from all JDWP 
>>> command sets.
>>
>> I'm reading the existing code exactly opposite of what you
>> say here. The gdata->vmDead check applied to all command
>> sets except for the VirtualMachine command set. I agree
>> that you've extended it to all command sets, but we're
>> not ignoring the commands. We are now setting the error
>> condition for all command sets.
>
> Agreed.
>
>
>>
>> Maybe I'm missing something here. Perhaps I've been away
>> from this code for too long... :-)
>
> No, it is that I'm still learning this code with your help. :-)
>
>
> Thanks a lot!
> Serguei
>
>>
>> Dan
>>
>>
>>>
>>>   I suspect, this bug could also cause some of the timeout and 
>>> socket related issues.
>>>
>>>
>>> Testing:
>>>   The fix was tested with the nsk.jdi, jtreg com/sun/jdi and 100 runs
>>>   of the hotspot/test/closed/compiler/c1/6507107/HeapwalkingTest.java.
>>>   The test results are very clean.
>>>
>>>
>>> Thanks,
>>> Serguei
>>
>