On 2/7/18 16:47, Chris Plummer 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 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 _javascript_ 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.
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.
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 that was
provided by the customer.
Bug : https://bugs.openjdk.java.net/browse/JDK-8193150
Webrev : http://cr.openjdk.java.net/~psomashe/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). Is always
passes with the -Xint flag however always fails with
-Xcomp. It fails intermittently with the -Xmixed
(default).
thanks,
Paru.
|