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, [email protected]
<mailto:[email protected]> 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.
