Re: RFR 8193150: Create a jtreg version of the test from JDK-8187143

[Chris Plummer]

On 2/7/18 3:23 PM, serguei.spit...@oracle.com wrote: Hi Chris, On 2/7/18 15:06, Chris Plummer wrote: Hi Paru, On 2/7/18 2:30 PM, Paru Somashekar wrote: Thanks for the review Chris, comments inline.. On 2/7/18, 1:25 PM, Chris Plummer wrote: Hi Paru, Thanks for writing this test. It will make figuring out JDK-8187143 a lot easier. Overall the test looks good. My main concern is the lack of comments. It makes it hard for the reader to understand the flow of the test and to understand some of the less obvious actions being performed. That is especially true for this test, which is doing some really bizarre stuff. Some of this you cover in our RFR summary below, but that info really needs to be in the test itself, along with additional comments. For example, what does pauseAtDebugger() do? It looks to me like it sets a breakpoint on the _javascript_ debugger that has a class name that ends with ScriptRuntime and the method name is DEBUGGER(). But I only figured that out after staring at the code for a while, and recalling a conversation we had a few weeks ago. It's also not described why this is being done. I shall add more comments into the test code to make it easier to understand. However while I understand what is being done ( e.g. adding breakpoint on Nashorn's internal DEBUGGER method) - unfortunately I too am not sure "why" it is being done. I do not have insight on what the author ( bug reporter) was trying to do.. That's ok. They "why" is because this is a test case demonstrating a failure a user ran into. You might want to mention that also, although the @bug reference might enough. Agreed as this is my understanding too. Here's another example:  126         while (!vmDisconnected) {  127             try {  128                 Thread.sleep(100);  129             } catch (InterruptedException ee) {  130             }  131         } I seem to also recall us discussing the need for this, but can no longer recall the reason The above loop is there to make the debugger continue to run until it receives a VMdisconnect event either because the Debuggee crashed / got exception / finished. I shall add a comment for this as well. Another example is findScriptFrame(). What is the significance of the frame whose class name starts with jdk.nashorn.internal.scripts.Script$? I think I understand (it's the generated java method for the _javascript_ you setup in ScriptDebuggee.doit()), but I can only figure this out based on earlier conversations we had and your RFR comments below. I'd expect the uninformed reader to spend a long time coming the same conclusion. Again, I am not clear on the significance of popping frames until this method which is a generated java method for _javascript_ in the debuggee. I could consult with the author and add those comments as well. This is just to recreate the situation the customer saw when running into the bug. We don't need to know the details of why they were doing what they did, only that it resulted in a bug being exposed. I'm mostly asking that you add comments that explain what the test is doing, but not worry so much about explaining the underlying reasons why the test was written in the first place (although that might be useful as part of an overall test summary comment at the top). Right. Thank you for the suggestion! I did not pay attention to it when pre-reviewed. The following are just a few minor things I noticed: Copyright should only have 2018.   57         } catch (Exception npe) { Probably best to call it "ex" instead of "npe".   85         NashornPopFrameTest bbcT = new NashornPopFrameTest(args); It's unclear to me where the name "bbcT" comes from. I shall change that to npft or something like that.  134         if (failReason != null) {  135             failure(failReason);  136         } You have two classes that declare "String failReason" which makes it a bit confusing to track which one the reader is dealing with. Also, the NashornPopFrameTest version is initialized to non-null, so doesn't that make the test always fail when the above code is executed? Even though failReason is initialized, it is reset if the expected breakpoint is reached. And when the breakpoint is reached, it checks the Debuggee version of the field, if it is non null, then this field is set to the non null value - else it is set to null. I shall add some comments to make it less confusing. So in the above check failReason has a double meaning (maybe even triple). It could be set to its original value, which means the breakpoint was never reached, or if the breakpoint is reached it is set to ScriptDebuggee.failReason, which basically represents the result of having called engine.eval(script). I think it would be clearer if you just had a static flag to indicate if the breakpoint was reached and just initialize failReason to null. The static flag does not work as the debuggee is in a different VM process. Of course. Rookie mistake on my part. :) Then the above becomes something like:     if (breakpointReached) {          if (failReason != null) {               failure(failReason);          }     } else {         failure("Expected breakpoint in ScriptDebuggee:" +                ScriptDebuggee.BKPT_LINE + " was not reached");     } But then I wonder, why not just rethrow the exception when engine.eval(script) fails and save yourself from having to fetch ScriptDebuggee.failReason using the debugger, or is that somehow part of what is being tested? It is not going to work if I understand things correctly. Please, check my comment above. In order to make it less confusing, I'd suggest to rename failReason to debuggeeFailReason on the debuggee side. Yeah, maybe. But then you could also call it debuggeeFailReason on the debugger side. That might make more sense. There's no reason for ScriptDebuggee to add the "debuggee" prefix to one of its own fields. I think there's still a need to have cleaner logic for indicating if the breakpoint was reached. Right now we initialize failReason to a potential failed reason string, then clear it if we hit the break point and the debuggee had no previous errors. I think using breakpointReached logic like I have above is a cleaner approach. thanks, Chris Thanks, Serguei Is there a reason why ScriptDebuggee doesn't just put everything in main() and not have a doit() method? No there isn't a specific reason. I noticed that other tests were doing it - like BreakpointTest and for consistency and clarity, i followed that pattern. Ok. thanks, Chris thanks, Paru. thanks, Chris On 2/7/18 12:25 PM, serguei.spit...@oracle.com wrote: Hi Paru, It looks good. Thank you a lot for taking care about this! Could we get at least one more review from the Serviceability team on this new test? Thanks, Serguei On 2/2/18 09:35, Paru Somashekar wrote: Hi, Please review the fix for JDK-8193150. The fix introduces a new jtreg test, NashornPopFrameTest. It is based on the original test from JDK-8187143 that was provided by the customer. Bug : https://bugs.openjdk.java.net/browse/JDK-8193150 Webrev : http://cr.openjdk.java.net/~psomashe/8193150/webrev/ Here is a brief description of what the test does :- * The debuggee,  creates and uses a Nashorn engine to evaluate a simple script. * The debugger  tries to set a breakpoint in Nashorn’s internal DEBUGGER method. * When the breakpoint is reached, it looks for stack frame whose method's declaring type name starts with (nashorn dynamically generated classes) ”jdk.nashorn.internal.scripts.Script$”. * It then pops stack frames using the ThreadReference.popFrames() call, up to and including the above stackframe. * The execution of the debuggee application is resumed after the needed frames have been popped. This test is included in the ProblemList as it fails under some circumstances (bug JDK-8187143). Is always passes with the -Xint flag however always fails with -Xcomp. It fails intermittently with the -Xmixed (default). thanks, Paru.