JDK-8203350: Crash in vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/TestDescription.java

[Gary Adams]

I've been unsuccessful trying to reproduce the failure in hs201t002.

Issue: https://bugs.openjdk.java.net/browse/JDK-8203350

Colleen made a comment on the bug that the reference
from hs201t002a to class hs201t002 might be an issue
for the redefined class.

I found in test hs201t001 that an intentional reference
before entering hs201t001a.doInit() is made to avoid
that classloader operation.

It's not clear to me why that was done, but the same workaround
could be used in hs201t002a, if it would make the test more robust.


diff --git
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
---
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
+++
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
@@ -26,6 +26,7 @@
public class hs201t002a extends Exception {

public hs201t002a () {
+ System.out.println("Current step: " + hs201t002.currentStep);
// Avoid calling classloader to find hs201t002 in doInit()
doInit();
}

[Chris Plummer]

Hi Gary,

Aren't you just hiding a potential jvmti bug with this change? If you
think this is a test bug and this is a proper fix, I'd like to see an
explanation of how the test is causing the crash. The explanation would
need to involve native code, since pure java should never crash.

thanks,

Chris

Post by Gary Adams
I've been unsuccessful trying to reproduce the failure in hs201t002.
  Issue: https://bugs.openjdk.java.net/browse/JDK-8203350
Colleen made a comment on the bug that the reference
from hs201t002a to class hs201t002 might be an issue
for the redefined class.
I found in test hs201t001 that an intentional reference
before entering hs201t001a.doInit() is made to avoid
that classloader operation.
It's not clear to me why that was done, but the same workaround
could be used in hs201t002a, if it would make the test more robust.
diff --git
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
---
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
+++
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
@@ -26,6 +26,7 @@
 public class hs201t002a extends Exception {
     public hs201t002a () {
+        System.out.println("Current step: " + hs201t002.currentStep);
// Avoid calling classloader to find hs201t002 in doInit()
         doInit();
     }

[Gary Adams]

At this point my first recommendation is to
simply remove the test from the ProblemList,
since it has not been reproduced after 1000s of
testruns with debug and release builds.

What is not clear to me at the moment is -
was it intentional that hs201t001 intentionally placed
the classloader operation outside of doInit() and
hs201t002 placed the classloader operation in the
doInit() method. It appears that the primary
purpose of these two tests is the difference in behavior
for IsMethodObsolete.

My best guess is that at some point the classloader operation
was failing after a RedefineClasses, and a workaround
was introduced into hs201t001. Later on the same
issue was observed with hs201t002, which did not have the
same workaround in place.

My primary goal is to make the test reliable for
functionality it is intended to exercise. My secondary
goal is to have some consistency between these
similar tests.

From the discussions in the related issues, the
crashes were not able to be reproduced so the issues
were resolved as CNR. It was presumed that some other
fix accounted for the problem no longer being visible.

Post by Chris Plummer
Hi Gary,
Aren't you just hiding a potential jvmti bug with this change? If you
think this is a test bug and this is a proper fix, I'd like to see an
explanation of how the test is causing the crash. The explanation
would need to involve native code, since pure java should never crash.
thanks,
Chris

Post by Gary Adams
I've been unsuccessful trying to reproduce the failure in hs201t002.
Issue: https://bugs.openjdk.java.net/browse/JDK-8203350
Colleen made a comment on the bug that the reference
from hs201t002a to class hs201t002 might be an issue
for the redefined class.
I found in test hs201t001 that an intentional reference
before entering hs201t001a.doInit() is made to avoid
that classloader operation.
It's not clear to me why that was done, but the same workaround
could be used in hs201t002a, if it would make the test more robust.
diff --git
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
---
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
+++
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
@@ -26,6 +26,7 @@
public class hs201t002a extends Exception {
public hs201t002a () {
+ System.out.println("Current step: " +
hs201t002.currentStep); // Avoid calling classloader to find
hs201t002 in doInit()
doInit();
}

[Chris Plummer]

Hi Gary,

If you haven't been able to reproduce the failure and just want to take
it off the problemlist, I'm ok with that. I'd rather not see any test
changes that could potentially hide the crash. If it eventually fails
for some other reason and it can be shown that it is testbug due to
improper classloading, then I can see adding the change you suggested.

Chris

Post by Gary Adams
At this point my first recommendation is to
simply remove the test from the ProblemList,
since it has not been reproduced after 1000s of
testruns with debug and release builds.
What is not clear to me at the moment is -
was it intentional that hs201t001 intentionally placed
the classloader operation outside of doInit() and
hs201t002 placed the classloader operation in the
doInit() method. It appears that the primary
purpose of these two tests is the difference in behavior
for IsMethodObsolete.
My best guess is that at some point the classloader operation
was failing after a RedefineClasses, and a workaround
was introduced into hs201t001. Later on the same
issue was observed with hs201t002, which did not have the
same workaround in place.
My primary goal is to make the test reliable for
functionality it is intended to exercise. My secondary
goal is to have some consistency  between these
similar tests.
From the discussions in the related issues, the
crashes were not able to be reproduced so the issues
were resolved as CNR. It was presumed that some other
fix accounted for the problem no longer being visible.

Post by Chris Plummer
Hi Gary,
Aren't you just hiding a potential jvmti bug with this change? If you
think this is a test bug and this is a proper fix, I'd like to see an
explanation of how the test is causing the crash. The explanation
would need to involve native code, since pure java should never crash.
thanks,
Chris

Post by Gary Adams
I've been unsuccessful trying to reproduce the failure in hs201t002.
  Issue: https://bugs.openjdk.java.net/browse/JDK-8203350
Colleen made a comment on the bug that the reference
from hs201t002a to class hs201t002 might be an issue
for the redefined class.
I found in test hs201t001 that an intentional reference
before entering hs201t001a.doInit() is made to avoid
that classloader operation.
It's not clear to me why that was done, but the same workaround
could be used in hs201t002a, if it would make the test more robust.
diff --git
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
---
a/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
+++
b/test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/hotswap/HS201/hs201t002/newclass/hs201t002a.java
@@ -26,6 +26,7 @@
 public class hs201t002a extends Exception {
     public hs201t002a () {
+        System.out.println("Current step: " +
hs201t002.currentStep); // Avoid calling classloader to find
hs201t002 in doInit()
         doInit();
     }