RFR 8193150: Create a jtreg version of the test from JDK-8187143
Paru Somashekar
parvathi.somashekar at oracle.com
Thu Feb 8 02:55:28 UTC 2018
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/20180207/062c12de/attachment-0001.html>
More information about the serviceability-dev
mailing list