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 all commits
Commits
File filter
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); \
}

This comment has been minimized.

@prrace

prrace Jun 30, 2021
Contributor

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.

This comment has been minimized.

@mkartashev

mkartashev Jun 30, 2021
Author Contributor

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,11 @@ 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 hasException;
JNU_CallMethodByName(env, &hasException, pFlush->runnable, "run", "()V");
if (hasException) {
J2dTraceLn(J2D_TRACE_ERROR, " exception occurred while executing runnable");
}
}
}

@@ -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;

This comment has been minimized.

@mrserb

mrserb Jul 17, 2021
Member

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

This comment has been minimized.

@mkartashev

mkartashev Jul 23, 2021
Author Contributor

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,69 @@
/*
* 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
* @run main FreeTypeScalerJNICheck
*/
import java.awt.Font;
import java.awt.FontMetrics;
import java.awt.GraphicsEnvironment;
import java.awt.Graphics2D;
import java.awt.geom.Rectangle2D;
import java.awt.image.BufferedImage;
import javax.swing.JFrame;
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();
BufferedImage bi = new BufferedImage(1, 1, BufferedImage.TYPE_INT_RGB);
Graphics2D g2d = bi.createGraphics();

for (String ff : families)
{
Font font = new Font(ff, Font.PLAIN, 12);
Rectangle2D bounds = font.getStringBounds("test", g2d.getFontRenderContext());
g2d.setFont(font);
FontMetrics metrics = g2d.getFontMetrics(font);
System.out.println(bounds.getHeight() + metrics.getHeight()); // use bounds and metrics
}

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