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.spit...@oracle.com wrote:
On 2/7/18 16:47, Chris Plummer wrote:
On 2/7/18 3:23 PM, serguei.spit...@oracle.com
<mailto:serguei.spit...@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.spit...@oracle.com
<mailto:serguei.spit...@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.