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
Closed
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -46,6 +46,12 @@

#include "fontscaler.h"

#define CHECK_EXCEPTION(env, describe) \
if ((*(env))->ExceptionCheck(env)) { \
if (describe) (*(env))->ExceptionDescribe(env);\
else (*(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
Member 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.


#define ftFixed1 (FT_Fixed) (1 << 16)
#define FloatToFTFixed(f) (FT_Fixed)((f) * (float)(ftFixed1))
#define FTFixedToFloat(x) ((x) / (float)(ftFixed1))
@@ -97,12 +103,18 @@ void z_error(char *s) {}
/**************** Error handling utilities *****************/

static jmethodID invalidateScalerMID;
static jboolean debugFonts; // Stores the value of FontUtilities.debugFonts()

JNIEXPORT void JNICALL
Java_sun_font_FreetypeFontScaler_initIDs(
JNIEnv *env, jobject scaler, jclass FFSClass) {
invalidateScalerMID =
(*env)->GetMethodID(env, FFSClass, "invalidateScaler", "()V");

jboolean ignoreException;
debugFonts = JNU_CallStaticMethodByName(env, &ignoreException,
"sun/font/FontUtilities",
"debugFonts", "()Z").z;
}

static void freeNativeResources(JNIEnv *env, FTScalerInfo* scalerInfo) {
@@ -137,6 +149,9 @@ 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 because it intentionally
// leaves an exception pending.
}

/******************* I/O handlers ***************************/
@@ -187,6 +202,7 @@ static unsigned long ReadTTFontFileFunc(FT_Stream stream,
scalerInfo->font2D,
sunFontIDs.ttReadBlockMID,
bBuffer, offset, numBytes);
CHECK_EXCEPTION(env, debugFonts);
if (bread < 0) {
return 0;
} else {
@@ -206,7 +222,8 @@ static unsigned long ReadTTFontFileFunc(FT_Stream stream,
(*env)->CallObjectMethod(env, scalerInfo->font2D,
sunFontIDs.ttReadBytesMID,
offset, numBytes);
/* If there's an OutofMemoryError then byteArray will be null */
CHECK_EXCEPTION(env, debugFonts);
/* If there's an OutOfMemoryError then byteArray will be null */
if (byteArray == NULL) {
return 0;
} else {
@@ -239,6 +256,7 @@ static unsigned long ReadTTFontFileFunc(FT_Stream stream,
sunFontIDs.ttReadBlockMID,
bBuffer, offset,
scalerInfo->fontDataLength);
CHECK_EXCEPTION(env, debugFonts);
if (bread <= 0) {
return 0;
} else if ((unsigned long)bread < numBytes) {
@@ -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
Member 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
Member 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
Member 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.

}
}

@@ -6569,11 +6569,11 @@ JNIEXPORT void JNICALL
Java_java_awt_Component_initIDs(JNIEnv *env, jclass cls)
{
TRY;
jclass inputEventClazz = env->FindClass("java/awt/event/InputEvent");
CHECK_NULL(inputEventClazz);
jmethodID getButtonDownMasksID = env->GetStaticMethodID(inputEventClazz, "getButtonDownMasks", "()[I");
CHECK_NULL(getButtonDownMasksID);
jintArray obj = (jintArray)env->CallStaticObjectMethod(inputEventClazz, getButtonDownMasksID);
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
Member 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.

CHECK_NULL(obj);
jint * tmp = env->GetIntArrayElements(obj, JNI_FALSE);
CHECK_NULL(tmp);
jsize len = env->GetArrayLength(obj);
@@ -752,6 +752,7 @@ void AwtDesktopProperties::SetStringProperty(LPCTSTR propName, LPTSTR value) {
key, jValue);
GetEnv()->DeleteLocalRef(jValue);
GetEnv()->DeleteLocalRef(key);
(void)safe_ExceptionOccurred(GetEnv());
}

void AwtDesktopProperties::SetIntegerProperty(LPCTSTR propName, int value) {
@@ -764,6 +765,7 @@ void AwtDesktopProperties::SetIntegerProperty(LPCTSTR propName, int value) {
AwtDesktopProperties::setIntegerPropertyID,
key, (jint)value);
GetEnv()->DeleteLocalRef(key);
(void)safe_ExceptionOccurred(GetEnv());
}

void AwtDesktopProperties::SetBooleanProperty(LPCTSTR propName, BOOL value) {
@@ -775,6 +777,7 @@ void AwtDesktopProperties::SetBooleanProperty(LPCTSTR propName, BOOL value) {
AwtDesktopProperties::setBooleanPropertyID,
key, value ? JNI_TRUE : JNI_FALSE);
GetEnv()->DeleteLocalRef(key);
(void)safe_ExceptionOccurred(GetEnv());
}

void AwtDesktopProperties::SetColorProperty(LPCTSTR propName, DWORD value) {
@@ -787,6 +790,7 @@ void AwtDesktopProperties::SetColorProperty(LPCTSTR propName, DWORD value) {
key, GetRValue(value), GetGValue(value),
GetBValue(value));
GetEnv()->DeleteLocalRef(key);
(void)safe_ExceptionOccurred(GetEnv());
}

void AwtDesktopProperties::SetFontProperty(HDC dc, int fontID,
@@ -849,6 +853,7 @@ void AwtDesktopProperties::SetFontProperty(HDC dc, int fontID,
key, fontName, style, pointSize);
GetEnv()->DeleteLocalRef(key);
GetEnv()->DeleteLocalRef(fontName);
(void)safe_ExceptionOccurred(GetEnv());
}
}
delete[] face;
@@ -896,6 +901,7 @@ void AwtDesktopProperties::SetFontProperty(LPCTSTR propName, const LOGFONT & fon
key, fontName, style, pointSize);
GetEnv()->DeleteLocalRef(key);
GetEnv()->DeleteLocalRef(fontName);
(void)safe_ExceptionOccurred(GetEnv());
}

void AwtDesktopProperties::SetSoundProperty(LPCTSTR propName, LPCTSTR winEventName) {
@@ -913,6 +919,7 @@ void AwtDesktopProperties::SetSoundProperty(LPCTSTR propName, LPCTSTR winEventNa
key, event);
GetEnv()->DeleteLocalRef(event);
GetEnv()->DeleteLocalRef(key);
(void)safe_ExceptionOccurred(GetEnv());
}

void AwtDesktopProperties::PlayWindowsSound(LPCTSTR event) {
@@ -0,0 +1,68 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2021, JetBrains s.r.o.. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/* @test
* @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
Member 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
Member 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.

* @run main FreeTypeScalerJNICheck
*/
import javax.swing.*;
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
Member 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.

import jdk.test.lib.process.ProcessTools;
import jdk.test.lib.process.OutputAnalyzer;

public class FreeTypeScalerJNICheck {
public static void main(String[] args) throws Exception {
if (args.length > 0 && args[0].equals("runtest")) {
runTest();
} else {
ProcessBuilder pb = ProcessTools.createTestJvm("-Xcheck:jni", FreeTypeScalerJNICheck.class.getName(), "runtest");
OutputAnalyzer oa = ProcessTools.executeProcess(pb);
oa.shouldContain("Done").shouldNotContain("WARNING").shouldHaveExitValue(0);
}
}

public static void runTest() {
String families[] = GraphicsEnvironment.getLocalGraphicsEnvironment().getAvailableFontFamilyNames();
JFrame frame = new JFrame("test");
BufferedImage bi = new BufferedImage(1, 1, BufferedImage.TYPE_INT_RGB);
Graphics2D g2d = bi.createGraphics();

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
Member 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(...).

Rectangle2D bounds = font.getStringBounds("test", g2d.getFontRenderContext());
FontMetrics metrics = frame.getFontMetrics(font);
System.out.println(bounds.getHeight() + metrics.getHeight()); // use bounds and metrics
}

System.out.println("Done");
}
}