8358452: JNI exception pending in Java_sun_awt_screencast_ScreencastHelper_remoteDesktopKeyImpl of screencast_pipewire.c:1214 (ID: 51119) #25605

azvegint · 2025-06-03T03:39:31Z

After calling AWT_UNLOCK(), GetStringUTFChars may be called with a pending exception.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8358452: JNI exception pending in Java_sun_awt_screencast_ScreencastHelper_remoteDesktopKeyImpl of screencast_pipewire.c:1214 (ID: 51119) (Bug - P3)

Reviewers

Alexey Ushakov (@avu - Committer) Review applies to 5f49dc30
Prasanta Sadhukhan (@prsadhuk - Reviewer)
Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25605/head:pull/25605
$ git checkout pull/25605

Update a local copy of the PR:
$ git checkout pull/25605
$ git pull https://git.openjdk.org/jdk.git pull/25605/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25605

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25605.diff

Using Webrev

Link to Webrev Comment

…elper_remoteDesktopKeyImpl of screencast_pipewire.c:1214 (ID: 51119)

bridgekeeper · 2025-06-03T03:40:02Z

👋 Welcome back azvegint! 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 · 2025-06-03T03:40:35Z

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

8358452: JNI exception pending in Java_sun_awt_screencast_ScreencastHelper_remoteDesktopKeyImpl of screencast_pipewire.c:1214 (ID: 51119)

Reviewed-by: psadhukhan, serb, avu

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

b787ff6: 8358538: Update GHA Windows runner to 2025
5b27e9c: 8342818: Implement JEP 509: JFR CPU-Time Profiling
3cf3e4b: 8358339: Handle MethodCounters::_method backlinks after JDK-8355003
... and 70 more: https://git.openjdk.org/jdk/compare/24edd3b2c1324fd58575a6273e5cae17e3d6fbf5...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-06-03T03:41:04Z

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

client

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.

mlbridge · 2025-06-03T03:44:19Z

Webrevs

01: Full - Incremental (f40cd179)
00: Full (5f49dc30)

avu

LGTM

prsadhuk · 2025-06-03T08:35:40Z

src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c

@@ -1206,7 +1206,7 @@ JNIEXPORT jint JNICALL Java_sun_awt_screencast_ScreencastHelper_remoteDesktopKey
    int key = awt_getX11KeySym(jkey);
    AWT_UNLOCK();

-    if (key == NoSymbol) {
+    if (key == NoSymbol || (*env)->ExceptionCheck(env)) {


I guess we normally call ExceptionCheck after we call some JNI methods like (*env)->NewString or (*env)->CallObjectMethod. Here before this check we are not calling any JNI method so I think we should do it after we call (*env)->GetStringUTFChars ie. after l1214

we are not calling any JNI method

We have the AWT_LOCK() / AWT_UNLOCK(); macro expansions with JNI methods being called.

jdk/src/java.desktop/unix/native/common/awt/awt.h

Lines 97 to 110 in 78a392a

#define AWT_NOFLUSH_UNLOCK_IMPL() \

do { \

jthrowable pendingException; \

if ((pendingException = (*env)->ExceptionOccurred(env)) != NULL) { \

(*env)->ExceptionClear(env); \

} \

(*env)->CallStaticVoidMethod(env, tkClass, awtUnlockMID); \

if ((*env)->ExceptionCheck(env)) { \

(*env)->ExceptionClear(env); \

} \

if (pendingException) { \

(*env)->Throw(env, pendingException); \

} \

} while (0)

This is exactly what static code analysis tool complains about.

JNI exception pending in Java_sun_awt_screencast_ScreencastHelper_remoteDesktopKeyImpl of screencast_pipewire.c:1214

But we are calling ExceptionCheck in that macro in l104 so why do we again need to call?

Please note the line 108 of the awt.h

But then it should show JNI exception pending for all places where we call AWT_UNLOCK, why only this place?

We call AWT_UNLOCK in awt_Robot.c, gtk3_interface.c, awt_GraphicsEnv.c where there's no ExceptionCheck after the call.
For example, here ExceptionCheck is called not after AWT_UNLOCK but after the next JNI call (*env)->NewObject

jdk/src/java.desktop/unix/native/libawt_xawt/awt/awt_GraphicsEnv.c

Lines 1281 to 1290 in ebd8528

AWT_LOCK ();

XGetWindowAttributes(awt_display, RootWindow(awt_display, screen),

&xwa);

AWT_UNLOCK ();

bounds = (*env)->NewObject(env, clazz, mid, 0, 0,

xwa.width, xwa.height);

}

if ((*env)->ExceptionCheck(env)) {

Normally we do ExceptionCheck after GetStringUTFChars

jdk/src/java.desktop/windows/native/libawt/windows/ShellFolder2.cpp

Lines 1192 to 1193 in ebd8528

const char *pLibName = env->GetStringUTFChars(libName, NULL);

JNU_CHECK_EXCEPTION_RETURN(env, 0);

Did you verify the warning goes off only by this PR change? If not, I will also recommend doing CHECK_EXCEPTION after GetStringUTFChars too..

We call AWT_UNLOCK in awt_Robot.c, gtk3_interface.c, awt_GraphicsEnv.c where there's no ExceptionCheck after the call.

Neither awt_Robot.c nor gtk3_interface.c calls any JNI functions afterwards.
the awt_GraphicsEnv.c case doesn't call JNI functions between AWT_LOCK / AWT_UNLOCK (see explanation below about false positive).

Normally we do ExceptionCheck after GetStringUTFChars

I added it just in case, but the GetStringUTFChars documentation doesn't mention any exceptions being thrown.

Did you verify the warning goes off only by this PR change?

it is the only reported line by the tool:

JNI exception pending in Java_sun_awt_screencast_ScreencastHelper_remoteDesktopKeyImpl of screencast_pipewire.c:1214

so it should resolve the issue.

But now I think it might be a false positive:

AWT_LOCK() clears all exceptions:

jdk/src/java.desktop/unix/native/common/awt/awt.h

Lines 86 to 95 in 78a392a

#define AWT_LOCK_IMPL() \

do { \

if ((*env)->ExceptionCheck(env)) { \

(*env)->ExceptionClear(env); \

} \

(*env)->CallStaticVoidMethod(env, tkClass, awtLockMID); \

if ((*env)->ExceptionCheck(env)) { \

(*env)->ExceptionClear(env); \

} \

} while(0)

awt_getX11KeySym doesn't call JNI methods at all

AWT_UNLOCK() also clears an exception, and rethrows a previously pending exception, if any.

jdk/src/java.desktop/unix/native/common/awt/awt.h

Lines 97 to 110 in 78a392a

#define AWT_NOFLUSH_UNLOCK_IMPL() \

do { \

jthrowable pendingException; \

if ((pendingException = (*env)->ExceptionOccurred(env)) != NULL) { \

(*env)->ExceptionClear(env); \

} \

(*env)->CallStaticVoidMethod(env, tkClass, awtUnlockMID); \

if ((*env)->ExceptionCheck(env)) { \

(*env)->ExceptionClear(env); \

} \

if (pendingException) { \

(*env)->Throw(env, pendingException); \

} \

} while (0)

Since neither 1. nor 2. produces an exception, no exception will be thrown.

3 rethrows the exception, maybe tool is complaining about that..maybe we should find out what that exception is and who is throwing?
Should we call EXCEPTION_CHECK_DESCRIBE instead to find out what exception it is complaining about by running the tool?

Also, dont we need to do ExceptionClear too in addition to ExcepionCheck to clear out the exception else it will keep it pending?

I don't think so. Let Java catch it.

ok..but checkReturnValue in ScreenCastHelper.java doesnt handle the exception for RESULT_ERROR case so it will be left to application to handle and am not sure if they will expect this internal exception..

prsadhuk

Looks good to me..

mrserb · 2025-06-04T08:40:59Z

It is still unclear why an exception is reported just in this line and others like mention below, can we narrow down what exact line in the code/macro caused that?

azvegint · 2025-06-04T13:23:46Z

It is still unclear why an exception is reported just in this line and others like mention below, can we narrow down what exact line in the code/macro caused that?

It is AWT_UNLOCK(); and Throw in it, but it just rethrows earlier pending exception.

It seems to be a false positive because neither AWT_LOCK nor awt_getX11KeySym leaves any pending exceptions.

#25605 (comment)

8358452: JNI exception pending in Java_sun_awt_screencast_ScreencastH…

5f49dc3

…elper_remoteDesktopKeyImpl of screencast_pipewire.c:1214 (ID: 51119)

openjdk bot added the rfr Pull request is ready for review label Jun 3, 2025

openjdk bot added the client [email protected] label Jun 3, 2025

avu approved these changes Jun 3, 2025

View reviewed changes

prsadhuk reviewed Jun 3, 2025

View reviewed changes

JNU_CHECK_EXCEPTION_RETURN for GetStringUTFChars

f40cd17

prsadhuk approved these changes Jun 4, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 4, 2025

mrserb approved these changes Jun 4, 2025

View reviewed changes

	#define AWT_NOFLUSH_UNLOCK_IMPL() \
	do { \
	jthrowable pendingException; \
	if ((pendingException = (*env)->ExceptionOccurred(env)) != NULL) { \
	(*env)->ExceptionClear(env); \
	} \
	(*env)->CallStaticVoidMethod(env, tkClass, awtUnlockMID); \
	if ((*env)->ExceptionCheck(env)) { \
	(*env)->ExceptionClear(env); \
	} \
	if (pendingException) { \
	(*env)->Throw(env, pendingException); \
	} \
	} while (0)

	AWT_LOCK ();
	XGetWindowAttributes(awt_display, RootWindow(awt_display, screen),
	&xwa);
	AWT_UNLOCK ();

	bounds = (*env)->NewObject(env, clazz, mid, 0, 0,
	xwa.width, xwa.height);
	}

	if ((*env)->ExceptionCheck(env)) {

	const char *pLibName = env->GetStringUTFChars(libName, NULL);
	JNU_CHECK_EXCEPTION_RETURN(env, 0);

	#define AWT_LOCK_IMPL() \
	do { \
	if ((*env)->ExceptionCheck(env)) { \
	(*env)->ExceptionClear(env); \
	} \
	(*env)->CallStaticVoidMethod(env, tkClass, awtLockMID); \
	if ((*env)->ExceptionCheck(env)) { \
	(*env)->ExceptionClear(env); \
	} \
	} while(0)

8358452: JNI exception pending in Java_sun_awt_screencast_ScreencastHelper_remoteDesktopKeyImpl of screencast_pipewire.c:1214 (ID: 51119) #25605

Are you sure you want to change the base?

8358452: JNI exception pending in Java_sun_awt_screencast_ScreencastHelper_remoteDesktopKeyImpl of screencast_pipewire.c:1214 (ID: 51119) #25605

Uh oh!

Conversation

azvegint commented Jun 3, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 3, 2025

Uh oh!

openjdk bot commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jun 3, 2025

Uh oh!

mlbridge bot commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

avu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

azvegint Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsadhuk Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsadhuk left a comment

Choose a reason for hiding this comment

Uh oh!

mrserb commented Jun 4, 2025

Uh oh!

azvegint commented Jun 4, 2025

Uh oh!

Uh oh!

azvegint commented Jun 3, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jun 3, 2025 •

edited

Loading

mlbridge bot commented Jun 3, 2025 •

edited

Loading

azvegint Jun 4, 2025 •

edited

Loading

prsadhuk Jun 4, 2025 •

edited

Loading