RFR 8193150: Create a jtreg version of the test from JDK-8187143
Paru Somashekar
parvathi.somashekar at oracle.com
Thu Feb 8 22:50:59 UTC 2018
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
>>>>>>>>> <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.
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180208/2e4f02be/attachment-0001.html>
More information about the serviceability-dev
mailing list