Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8269223: -Xcheck:jni WARNINGs working with fonts on Linux #4572

Closed
wants to merge 6 commits into from

Conversation

@mkartashev
Copy link
Contributor

@mkartashev mkartashev commented Jun 23, 2021

Added an ExceptionCheck() followed by ExceptionDescribe() and ExceptionClear() immediately after the Java calls made from the callback function ReadTTFontFileFunc() in freetypeScaler.c.

The exception(s) need to be cleared because we're not returning immediately to Java that would've been able to handle them gracefully. And in order not to loose the exception entirely (even though the return value would also indicate an error condition), print out the exception with ExceptionDescribe() to aid in debugging.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8269223: -Xcheck:jni WARNINGs working with fonts on Linux

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4572/head:pull/4572
$ git checkout pull/4572

Update a local copy of the PR:
$ git checkout pull/4572
$ git pull https://git.openjdk.java.net/jdk pull/4572/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4572

View PR using the GUI difftool:
$ git pr show -t 4572

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4572.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 23, 2021

👋 Welcome back mkartashev! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Jun 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 23, 2021

@mkartashev The following label will be automatically applied to this pull request:

  • 2d

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.

@openjdk openjdk bot added the 2d label Jun 23, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 23, 2021

/* @test
* @bug 8269223
* @summary Verifies that -Xcheck:jni issues no warnings from freetypeScaler.c
* @requires os.family == "linux"
Copy link
Member

@mrserb mrserb Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we run this test on all platforms? Since this bug was not found, means we did not cover this code by the tests, and it will be useful to test it even if the code path will be different on other platforms.

Copy link
Contributor Author

@mkartashev mkartashev Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure; dropped the @requires clause.

@@ -137,6 +143,8 @@ static void invalidateJavaScaler(JNIEnv *env,
FTScalerInfo* scalerInfo) {
freeNativeResources(env, scalerInfo);
(*env)->CallVoidMethod(env, scaler, invalidateScalerMID);
// NB: Exceptions must not be cleared (and therefore no JNI calls performed) after calling this method
Copy link
Member

@mrserb mrserb Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split long lines to 80 chars per line

Copy link
Contributor Author

@mkartashev mkartashev Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -187,6 +195,8 @@ static unsigned long ReadTTFontFileFunc(FT_Stream stream,
scalerInfo->font2D,
sunFontIDs.ttReadBlockMID,
bBuffer, offset, numBytes);
// This is a callback, we are not returning immediately to Java and better report exceptions now
CHECK_EXCEPTION(env);
Copy link
Member

@mrserb mrserb Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should report it only if "debugFonts" was set?

Copy link
Contributor Author

@mkartashev mkartashev Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if ((*(env))->ExceptionCheck(env)) { \
(*(env))->ExceptionDescribe(env);\
(*(env))->ExceptionClear(env); \
}
Copy link
Contributor

@prrace prrace Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.oracle.com/en/java/javase/11/docs/specs/jni/functions.html#exceptiondescribe

"The pending exception is cleared as a side-effect of calling this function"

So you certainly don't need both of these and I would prefer that Describe only be used if really debugging where we think there's a REAL chance of an exception rather than just to keep JNI happy.

And the upcall that is likely (readBlock) itself will log any IOException (and catch it) in the event of an I/O error so I really think this is unlikely to be useful here.

Copy link
Contributor Author

@mkartashev mkartashev Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, makes sense. I made CHECK_EXCEPTION() call either Clear... or Describe... based on the value of debugFonts.
Please, take a look.

@@ -187,6 +195,8 @@ static unsigned long ReadTTFontFileFunc(FT_Stream stream,
scalerInfo->font2D,
sunFontIDs.ttReadBlockMID,
bBuffer, offset, numBytes);
// This is a callback, we are not returning immediately to Java and better report exceptions now
Copy link
Contributor

@prrace prrace Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment is un-needed .. since the only reason to call CHECK_EXCEPTION() is because of this.
Same for the other case.

Copy link
Contributor Author

@mkartashev mkartashev Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, comments removed.

1. Allowed test to run on any platform.
2. Trimmed comments to fit in with 80 columns.
3. Removed unnecessayr comments.
4. Made the ExceptionDescribe() calls conditional on the value of
   FontUtilities.debugFonts()
@mrserb
Copy link
Member

@mrserb mrserb commented Jul 2, 2021

Looks fine to me, I'll run the tests.

@mkartashev
Copy link
Contributor Author

@mkartashev mkartashev commented Jul 7, 2021

Looks fine to me, I'll run the tests.

@mrserb Any news on that front?

@mrserb
Copy link
Member

@mrserb mrserb commented Jul 9, 2021

@mrserb Any news on that front?

The new test fails on all platforms:

  • Linux/MacOS: The test should be marked as headful, otherwise it may run on headless systems where "No X11 DISPLAY variable was set"
  • WIndows: The jni warnings are generated in the log.

system.
2. Added exception checks to Windows-specific code.
@@ -866,7 +866,8 @@ void D3DRQ_FlushBuffer(void *pParam)

if (!JNU_IsNull(env, pFlush->runnable)) {
J2dTraceLn(J2D_TRACE_VERBOSE, " executing runnable");
JNU_CallMethodByName(env, NULL, pFlush->runnable, "run", "()V");
jboolean ignoreException;
JNU_CallMethodByName(env, &ignoreException, pFlush->runnable, "run", "()V");
Copy link
Member

@mrserb mrserb Jul 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this change? the only difference is that in the second case the ExceptionCheck will be called, does it affect something?

Copy link
Contributor Author

@mkartashev mkartashev Jul 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the ExceptionCheck() call will silence the warnings from -Xcheck:jni.

Copy link
Member

@mrserb mrserb Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it actually suppress the "Xcheck:jni" or it clears a raised exception? If an exception is still "raised" after this call we should do some additional steps to log/clean it.

Copy link
Contributor Author

@mkartashev mkartashev Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the exception will still be "raised" after this call. Since there are no JNI calls (at least those that are forbidden to make with an exception pending) between here and return back to Java, I believe no additional steps are necessary.

Copy link
Member

@mrserb mrserb Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I suggest logging them via some of the trace macros.

Copy link
Contributor Author

@mkartashev mkartashev Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, added some logging.

jboolean ignoreException;
jintArray obj = (jintArray)JNU_CallStaticMethodByName(env, &ignoreException,
"java/awt/event/InputEvent",
"getButtonDownMasks", "()[I").l;
Copy link
Member

@mrserb mrserb Jul 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obj might be null? Can not we just add CHECK_NULL(obj) here?

Copy link
Contributor Author

@mkartashev mkartashev Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obj indeed might be null, especially since all kinds of things could go wrong during class/method resolution. Added CHECK_NULL(obj) here.

1. Added CHECK_NULL() to awt_Component.cpp
@mkartashev
Copy link
Contributor Author

@mkartashev mkartashev commented Aug 4, 2021

Ping.

@prrace
Copy link
Contributor

@prrace prrace commented Aug 12, 2021

I'm going to see if the test passes on the internal systems we use which I think will be mostly variout RH/OL systems for this headless test for Linux - and it also will get run on Windows Server of some kind .. I don't think macOS will hit this code path at all for this test.

import java.awt.*;
import java.awt.geom.Rectangle2D;
import java.awt.image.*;
import java.io.*;
Copy link
Contributor

@prrace prrace Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get rid of all these wild card imports ?

Copy link
Contributor Author

@mkartashev mkartashev Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, replaced with single-class imports.

* @bug 8269223
* @summary Verifies that -Xcheck:jni issues no warnings from freetypeScaler.c
* @library /test/lib
* @key headful
Copy link
Contributor

@prrace prrace Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this test is headful ?

Copy link
Contributor

@prrace prrace Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh you are getting metrics for a JFrame ? That's not going to exercise any new font code so is pointless except to make it so the test has to be headful.

Copy link
Contributor Author

@mkartashev mkartashev Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But getFontMetrics() is the primary "entry point" that generated all the JNI warnings in the first place. And I'm also not sure that we could've gotten all the warnings on Windows without JFrame.

Copy link
Contributor

@prrace prrace Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what's wrong with
g2d.setFont(font);
FontMetrics metrics = g2d.getFontMetrics(font);

?

No frame needed.

Copy link
Contributor Author

@mkartashev mkartashev Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Changed the test per your suggestions.


for (String ff : families)
{
Font font = Font.decode(ff);
Copy link
Contributor

@prrace prrace Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gosh, does anyone still use decode() ? I keep forgetting it exists.
You have all the family names, why not just new Font(ff, Font.PLAIN, 12) ?

Copy link
Contributor Author

@mkartashev mkartashev Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changed to new Font(...).

1. Optimized imports in the test.
2. Got rid of Font.decode().
@prrace
Copy link
Contributor

@prrace prrace commented Aug 13, 2021

passes

Looks good. I tried the test as headless and headful which happens to then exercise a broader set of distros than only one or the other the way our systems are set up.

prrace
prrace approved these changes Aug 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 13, 2021

@mkartashev 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:

8269223: -Xcheck:jni WARNINGs working with fonts on Linux

Reviewed-by: prr, serb

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 739 new commits pushed to the master branch:

  • f77a1a1: 8272472: StackGuardPages test doesn't build with glibc 2.34
  • 04a806e: 8270344: Session resumption errors
  • d85560e: 8267161: Write automated test case for JDK-4479161
  • 1ea437a: 8272720: Fix the implementation of loop unrolling heuristic with LoopPercentProfileLimit
  • 86add21: 8223923: C2: Missing interference with mismatched unsafe accesses
  • c701f6e: 8272739: Misformatted error message in EventHandlerCreator
  • fb1dfc6: 8267185: Add string deduplication support to ParallelGC
  • d874e96: 8271579: G1: Move copy before CAS in do_copy_to_survivor_space
  • 92bde67: 8271946: Cleanup leftovers in Space and subclasses
  • db9834f: 8258951: java/net/httpclient/HandshakeFailureTest.java failed with "RuntimeException: Not found expected SSLHandshakeException in java.io.IOException"
  • ... and 729 more: https://git.openjdk.java.net/jdk/compare/1a818154cfe852a680070c5c07f2df33109e5b54...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prrace, @mrserb) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Aug 13, 2021
1. Added code to log the exception if it occurred when running a flushBuffer's runnable. The logging is minimal in order to avoid replacing the exception with another one before it reaches Java.

2. Made the test not dependent on headful code as per @prrace suggestions.
@mrserb
Copy link
Member

@mrserb mrserb commented Aug 16, 2021

Will the updated test case cover the same code paths where the bugs were found and fixed by this change?

@mkartashev
Copy link
Contributor Author

@mkartashev mkartashev commented Aug 17, 2021

Will the updated test case cover the same code paths where the bugs were found and fixed by this change?

I believe so - on Linux. Windows-specific changes are mostly covered (I verified Java_java_awt_Component_initIDs() (from awt_Component.cpp) and D3DRQ_FlushBuffer() (from D3DRenderQueue.cpp). Not all the AwtDesktopProperties::Set...Property() (from awt_DesktopProperties.cpp ) are touched by the test. I originally applied the changes to all the similar code there without verifying that each piece is invoked by the test.

mrserb
mrserb approved these changes Aug 17, 2021
@mkartashev
Copy link
Contributor Author

@mkartashev mkartashev commented Aug 22, 2021

/integrate

@openjdk openjdk bot added the sponsor label Aug 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 22, 2021

@mkartashev
Your change (at version 4b2febe) is now ready to be sponsored by a Committer.

@mrserb
Copy link
Member

@mrserb mrserb commented Aug 23, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Aug 23, 2021

Going to push as commit 9bc0232.
Since your change was applied there have been 749 commits pushed to the master branch:

  • 2ff4c01: 8271600: C2: CheckCastPP which should closely follow Allocate is sunk of a loop
  • ad92033: 8272736: [JVMCI] Add API for reading and writing JVMCI thread locals
  • 709b591: 8272553: several hotspot runtime/CommandLine tests don't check exit code
  • 1884072: 8265253: javac -Xdoclint:all gives "no comment" warning for code that can't be commented
  • 594e516: 8272778: Consolidate is_instance and is_instance_inlined in java_lang_String
  • d542745: 8267894: Skip work for empty regions in G1 Full GC
  • 741f58c: 8272417: ZGC: fastdebug build crashes when printing ClassLoaderData
  • b7f75c0: 8271142: package help is not displayed for missing X11/extensions/Xrandr.h
  • e8a289e: 8272609: Add string deduplication support to SerialGC
  • b690f29: 8269687: pauth_aarch64.hpp include name is incorrect
  • ... and 739 more: https://git.openjdk.java.net/jdk/compare/1a818154cfe852a680070c5c07f2df33109e5b54...master

Your commit was automatically rebased without conflicts.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 23, 2021

@mrserb @mkartashev Pushed as commit 9bc0232.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants