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

Chris Plummer chris.plummer at oracle.com
Thu Feb 8 23:36:13 UTC 2018 

Hi Paru,

Looks good. Thanks for the changes.

Chris

On 2/8/18 2:50 PM, Paru Somashekar wrote:
>
> I have incorporated all your feedback and created a new webrev at :
> http://cr.openjdk.java.net/~psomashe/8193150/webrev.01/ 
> <http://cr.openjdk.java.net/%7Epsomashe/8193150/webrev.01/>
>
> - Added comments
> - modified the logic for failReason and breakpoint reached aspect on 
> the debugger side.
>
> thanks,
> Paru.
>
> On 2/7/18, 6:55 PM, Paru Somashekar wrote:
>> Hi Chris, Serguei,
>>
>> On 2/7/18, 4:56 PM, serguei.spitsyn at oracle.com wrote:
>>> On 2/7/18 16:47, Chris Plummer wrote:
>>>> On 2/7/18 3:23 PM, serguei.spitsyn at oracle.com 
>>>> <mailto:serguei.spitsyn at 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 <https://bugs.openjdk.java.net/browse/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 java script 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. :)
>>>
>>> I knew it but had done the same mistake. :)
>>>
>>>
>>>>>> 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.
>>>
>>> Agreed.
>>>
>>>> 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.
>>>
>>> Got it, thanks.
>>> Yes, this will be more clear.
>> I shall change the logic as you have suggested and post another patch 
>> for review.
>>
>> thanks,
>> Paru.
>>
>>>
>>> Thanks,
>>> Serguei
>>>
>>>
>>>>
>>>> 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.spitsyn at oracle.com 
>>>>>>>> <mailto:serguei.spitsyn at 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 
>>>>>>>>>> <https://bugs.openjdk.java.net/browse/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/ 
>>>>>>>>>> <http://cr.openjdk.java.net/%7Epsomashe/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) 
>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8187143>. Is always 
>>>>>>>>>> passes with the -Xint flag however always fails with -Xcomp. 
>>>>>>>>>> It fails intermittently with the -Xmixed (default).
>>>>>>>>>>
>>>>>>>>>> thanks,
>>>>>>>>>> Paru. 
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

More information about the serviceability-dev mailing list