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

[Daniel D. Daugherty]

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.

         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.

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!


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


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

Maybe I'm missing something here. Perhaps I've been away
from this code for too long... :-)

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