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 1 commit
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) \
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
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))
@@ -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
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.

Done.

// because it intentionally leaves an exception pending.
}

/******************* I/O handlers ***************************/
@@ -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
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.

OK, comments removed.

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
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.

Done.

if (bread < 0) {
return 0;
} else {
@@ -206,7 +216,9 @@ 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 */
// This is a callback, we are not returning immediately to Java and better report exceptions now
CHECK_EXCEPTION(env);
/* If there's an OutOfMemoryError then byteArray will be null */
if (byteArray == NULL) {
return 0;
} else {
@@ -239,6 +251,8 @@ static unsigned long ReadTTFontFileFunc(FT_Stream stream,
sunFontIDs.ttReadBlockMID,
bBuffer, offset,
scalerInfo->fontDataLength);
// This is a callback, we are not returning immediately to Java and better report exceptions now
CHECK_EXCEPTION(env);
if (bread <= 0) {
return 0;
} else if ((unsigned long)bread < numBytes) {
@@ -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
* @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
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.

Sure; dropped the @requires clause.

* @library /test/lib
* @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");
}
}