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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Fri Feb 9 00:49:48 UTC 2018 

Hi Paru,

+1

Thanks,
Serguei


On 2/8/18 15:36, Chris Plummer wrote:
> 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