8323664: java/awt/font/JNICheck/FreeTypeScalerJNICheck.java still fails with JNI warning on some Windows configurations #17404
Conversation
|
👋 Welcome back clanger! A progress list of the required criteria for merging this PR into |
RealCLanger
force-pushed
the
JDK-8323664
branch
from
f84b132
to
3212874
Compare
openjdk
bot
changed the title
|
@RealCLanger Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
|
@RealCLanger The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
I have not worked out how this can actually happen. I am running my current tests locally on Windows 11, but our CI testing does run this on Windows Server (various versions, 2016, 2019 and 2022) The only way I can make this fail is to ACTUALLY add a 2nd JNI call in there - I was beginning to wonder if somehow we were missing the WARNING, but in fact that is working. So your code path is different but what is the exact reason ? That is what we should understand. Maybe it will |
|
The test is only failing on a few boxes in our CI infrastructure. And also only in the setup of the nightly tests, not if one would connect to the box and run the test there standalone. But I'll try to run the tests with some more instrumentation to get a better understanding. Stay tuned... |
|
Note that because the test is looking for the string WARNING, it captures all the output of the running VM into a log file, and jtreg will leave that log file there. So you should be able to add instrumentation (ie printf's) into the JDK itself to help understand the internal code flow, and let the test run in that CI environment and capture everything for your later inspection |
|
OK, finally it's understood what's happening. Here's the story: We basically run into the assertion reported in JDK-8185862. It causes the JNI warning through a CallStaticBooleanMethod in its depths. In detail: Via initScreens, called by initDisplay, we call Devices::UpdateInstance that triggers AwtWin32GraphicsDevice::Initialize and here we run into the assertion. The assertion is fired by this VERIFY. In Windows we have an assertion callback installed and it calls Java in isHeadless() which brings up the assertion. So, how to continue? I think the assertion itself should be addressed through JDK-8185862. We could maybe make the assert callback more robust by checking for a pending exception before calling the isHeadless function. But I think the currently proposed exception check on calling dwmCompositionChanged is also a viable solution. Or we could remove the dwmCompositionChanged call altogether and set the isDWMCompositionEnabled field through JNI's setField function directly. Furthermore, the test FreeTypeScalerJNICheck could also check for "Assertion" in its output, in addition to "Warning". Then it would bail out on both, JNI Warnings and AWT assertions. WDYT? |
I think that is an acceptable solution for this case. The broader problem is the reason we get here in this situation in the first place. In the absence of (2) it is likely that testing AWT on debug builds + headless will hit more problems. |
|
@RealCLanger This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 192 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Thanks for the review. For some reason, however, I still saw the jni warning in our env with the patch applied. Still looking into it to understand what I'm missing... |
OK, got it. The exception check needs to be before the call to FindClass. Now the patch is correct and does what it is supposed to do. |
|
Now that I re-read the problem statement and the exception handling JNI, the fix doesn't look correct to me. What does the warning say?
To me, it means that As far as I can see, it's the last statement of the Then Wouldn't it be better to check if an exception occurred after SetProcessDPIAwareProperty();
DWMIsCompositionEnabled();
if (env->ExceptionCheck()) {
return;
}
initScreens(env); |
| // be on the safe side and avoid JNI warnings by calling ExceptionCheck | ||
| // an accumulated exception is not cleared | ||
| env->ExceptionCheck(); |
There was a problem hiding this comment.
So, this doesn't actually do anything but avoids JNI warnings, does it?
AwtDebugSupport::AssertCallback can be called at any time, hence calling JNI functions here is unsafe. Adding env->ExceptionCheck() doesn't change anything.
There was a problem hiding this comment.
Yes. However, it's "only" in the assertion callback that only exists in debug VMs. And an original exception isn't lost.
There was a problem hiding this comment.
Perhaps what we should do here is
if (ExceptionCheck) {
ExceptionDescribe
ExceptionClear
}
So someone can "see" [yes, this means it isn't propagated but we've printed it and we have the assert coming up anyway] the text of the original exception, and the debugging code is safe to make the calls it wants.
The alternatives are that the debugging code in the case of ExceptionCheck==TRUE just do what it takes to silence the JNI warnings , assuming that TRUE is never not a possibility, so no real problem, but I don't know see how we can be sure about that for ALL callers of this assert code. (BTW I wonder if the reason the current code didn't do as expected is because ExceptionCheck isn't doing what we expect, but I don't see how), or alternative number 2 is that the debug code simply bails in the face of a pending exception, ie
if (ExceptionCheck) {
return;
}
There was a problem hiding this comment.
As far as I can see, the real problem is that DWMIsCompositionEnabled calls a Java method and does not check if an exception occurred. It should do it according to the JNI specification.
I can assume initScreens(env) does not call JNI methods, therefore no JNI warning is produced in the regular code flow where no assertions fail.
There was a problem hiding this comment.
assuming that TRUE is never not a possibility, so no real problem, but I don't know see how we can be sure about that for ALL callers of this assert code.
We can never be sure about it, even though I tend to believe exceptions are rare.
Essentially, any upcall into Java followed by an assertion will lead to this JNI warning because the assertion handler also upcalls into Java.
There was a problem hiding this comment.
As I said before, the new code just quiesces JNI warnings. It is anyway only used in debug builds. And in the case one sees a "real" exception before entering the assertion handler, this exception will be on the stack when returning to Java and cause the trouble it should then cause. I guess in that case an incorrect tracing of the assertion is no problem. And, after all, the current behavior is not changed.
I would rather like to pull your attention to a "real fix": Please have a look at #17614 (JDK-8185862). It would be good if that can be reviewed and integrated since it will fix a few errors that we see on Windows testing.
There was a problem hiding this comment.
Yes, I'll take a look at that soon. Remember there are two issues behind all of this
(1) We don't have a real headless build for windows and this can cause failures of windows APIs and Java exceptions
(2) debug builds use asserts on failures of windows APIs
(2) is partially addressed here
(1) is partially addressed in the other fix. I can't be sure its completely addressed
There was a problem hiding this comment.
I thought about it a bit more…
Since Java_sun_awt_Win32GraphicsEnvironment_initDisplay does not call other JNI functions after DWMIsCompositionEnabled, it is reasonable to leave a possible raised exception unhandled — it will handled by JVM after initDisplay returns.
Perhaps, it's better to handle the exception inside the assertion handler:
if (env->ExceptionCheck()) {
env->ExceptionDescribe();
env->ExceptionClear();
}Or maybe not, if ExceptionCheck() is enough to silence the warning and the raised exception is still thrown after the control is returned to JVM.
Well, yes - however the initScreens only calls other JNI methods through the assert callback of debug builds, so not a general thing. The initial proposal for a fix was to add an exception check right to the call of JNU_CallStaticMethodByName(...,"dwmCompositionChanged",...) but was dismissed by @prrace I think the current proposal makes the assertion callback more resilient to JNI warnings in general, so I guess it's not bad. |
|
I will integrate this in the next days unless somebody explicitly vetoes (given that the PR shows reviews from 3 persons) |
I guess that would be OK, but we may revisit this to do one of the things I outlined above. |
|
/integrate |
|
Going to push as commit 99c9ae1.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@RealCLanger Pushed as commit 99c9ae1. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This picks up fixing the issue of JDK-8276809 again. A fix had been integrated with #17224 but @prrace had concerns and so it was backed out.
I have now spent quite some thoughts into the problem and end up with the initial commit of #6306 as the most elegant and least intrusive solution.
Why is this?
The JNI warning we observe in the test is:
[WARNING in native method: JNI call made without checking exceptions when required to from CallStaticVoidMethodV at sun.awt.Win32GraphicsEnvironment.initDisplay(java.desktop@22.0.1-internal/Native Method) at sun.awt.Win32GraphicsEnvironment.initDisplayWrapper(java.desktop@22.0.1-internal/Win32GraphicsEnvironment.java:95) at sun.awt.Win32GraphicsEnvironment.<clinit>(java.desktop@22.0.1-internal/Win32GraphicsEnvironment.java:63) ... at FreeTypeScalerJNICheck.runTest(FreeTypeScalerJNICheck.java:53) at FreeTypeScalerJNICheck.main(FreeTypeScalerJNICheck.java:44)This happens because obviously the test FreeTypeScalerJNICheck runs with
-Xcheck:jniand in the scenario where we're observing the warning, a missing exception check for the JNI call tosun.awt.Win32GraphicsEnvironment::dwmCompositionChangedat awt_Win32GraphicsEnv.cpp#L129jdk/src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsEnv.cpp
Line 129 in c5e7245
sun.awt.Win32GraphicsEnvironment::dwmCompositionChanged, likely from the subsequent call to initScreens insun.awt.Win32GraphicsEnvironment::initDisplay.jdk/src/java.desktop/windows/native/libawt/windows/awt_Win32GraphicsEnv.cpp
Line 141 in 8b6293f
DWMIsCompositionEnabled()from somewhere else initially such that the initialization ofWin32GraphicsEnvironmentwould not go throughinitScreensor similar.In any case, I think triggering the invocation of JNI's
ExceptionCheckby passing a non NULL value as second argument to JNU_CallStaticMethodByNamejdk/src/java.base/share/native/libjava/jni_util.c
Line 200 in 8b6293f
@prrace please have a second look.
Also cc @schmelter-sap @MBaesken
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17404/head:pull/17404$ git checkout pull/17404Update a local copy of the PR:
$ git checkout pull/17404$ git pull https://git.openjdk.org/jdk.git pull/17404/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17404View PR using the GUI difftool:
$ git pr show -t 17404Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17404.diff
Webrev
Link to Webrev Comment