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

[serguei.spitsyn at oracle.com]

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
>