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

Wed Feb 7 22:30:53 UTC 2018

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..
>
> 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.
>
> 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.

>
> 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.

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/be2a9c6f/attachment.html>