From 506a593e4dec41c9d86f1db624380f4c2853be35 Mon Sep 17 00:00:00 2001 From: Alex Andres Date: Sun, 25 Oct 2026 19:52:11 +0100 Subject: [PATCH] fix: report Java exceptions from native callbacks in one piece ExceptionDescribe writes "Exception in thread ..." to native stderr and the stack trace to System.err. The prefixes piled up unterminated until a native line ended them (on macOS the Java2D GL warning in VideoViewTests), and setup-java's problem matcher turned that line into an error annotation on every macOS test job. Hand such exceptions to the thread's uncaught exception handler instead, as other callbacks already do, and stop describing in ExceptionCheck, whose exception travels on to be rethrown or reported. --- docs/guide/advanced/video-codecs.md | 2 +- .../jni-voithos/src/JavaUtils.cpp | 3 +- .../src/api/RTCEncodedFrameTransformer.cpp | 4 +- .../cpp/src/media/DeviceChangeListener.cpp | 6 +-- .../src/media/video/codec/VideoCodecUtils.cpp | 4 +- webrtc-jni/src/main/cpp/src/rtc/LogSink.cpp | 19 +++++++-- .../src/main/cpp/include/media/JniBoundary.h | 7 ++++ .../main/cpp/src/media/JavaPlayerObserver.cpp | 11 ++--- .../cpp/src/media/JavaRecorderObserver.cpp | 5 +-- .../src/main/cpp/src/media/JniBoundary.cpp | 41 +++++++++++++++++++ .../webrtc/media/player/MediaPlayerTest.java | 41 +++++++++++++++++++ .../onvoid/webrtc/VideoCodecFactoryTests.java | 17 ++++++++ 12 files changed, 136 insertions(+), 24 deletions(-) diff --git a/docs/guide/advanced/video-codecs.md b/docs/guide/advanced/video-codecs.md index 57f103550..495ff1573 100644 --- a/docs/guide/advanced/video-codecs.md +++ b/docs/guide/advanced/video-codecs.md @@ -281,7 +281,7 @@ class MyDecoder implements VideoDecoder { ## Errors -Whatever an encoder, decoder or factory method throws is logged and treated as `VideoCodecStatus.ERROR`, and a factory that throws from `createEncoder()` or `createDecoder()` creates no codec for that stream. On an error other than `FALLBACK_SOFTWARE` and `UNINITIALIZED`, WebRTC releases the codec and initializes it again. +Whatever an encoder, decoder or factory method throws goes to the calling thread's uncaught exception handler, is logged, and is treated as `VideoCodecStatus.ERROR`, and a factory that throws from `createEncoder()` or `createDecoder()` creates no codec for that stream. On an error other than `FALLBACK_SOFTWARE` and `UNINITIALIZED`, WebRTC releases the codec and initializes it again. ## Related API diff --git a/webrtc-jni/src/main/cpp/dependencies/jni-voithos/src/JavaUtils.cpp b/webrtc-jni/src/main/cpp/dependencies/jni-voithos/src/JavaUtils.cpp index 835c3eb63..f0d6734f7 100644 --- a/webrtc-jni/src/main/cpp/dependencies/jni-voithos/src/JavaUtils.cpp +++ b/webrtc-jni/src/main/cpp/dependencies/jni-voithos/src/JavaUtils.cpp @@ -22,8 +22,9 @@ bool ExceptionCheck(JNIEnv * env) { if (env->ExceptionCheck()) { + // Not described here: the exception travels on, to be thrown back + // into Java or reported by whoever catches it. jthrowable exception = env->ExceptionOccurred(); - env->ExceptionDescribe(); env->ExceptionClear(); if (exception) { diff --git a/webrtc-jni/src/main/cpp/src/api/RTCEncodedFrameTransformer.cpp b/webrtc-jni/src/main/cpp/src/api/RTCEncodedFrameTransformer.cpp index daf7c6c2e..1d3509d9c 100644 --- a/webrtc-jni/src/main/cpp/src/api/RTCEncodedFrameTransformer.cpp +++ b/webrtc-jni/src/main/cpp/src/api/RTCEncodedFrameTransformer.cpp @@ -15,6 +15,7 @@ */ #include "api/RTCEncodedFrameTransformer.h" +#include "api/WebRTCUtils.h" #include "JavaClasses.h" #include "JavaUtils.h" #include "JNI_WebRTC.h" @@ -52,8 +53,7 @@ namespace jni bool ClearException(JNIEnv * env) { if (env->ExceptionCheck()) { - env->ExceptionDescribe(); - env->ExceptionClear(); + ReportPendingException(env); return true; } diff --git a/webrtc-jni/src/main/cpp/src/media/DeviceChangeListener.cpp b/webrtc-jni/src/main/cpp/src/media/DeviceChangeListener.cpp index a452644ce..48b4cb530 100644 --- a/webrtc-jni/src/main/cpp/src/media/DeviceChangeListener.cpp +++ b/webrtc-jni/src/main/cpp/src/media/DeviceChangeListener.cpp @@ -15,6 +15,7 @@ */ #include "media/DeviceChangeListener.h" +#include "api/WebRTCUtils.h" #include "media/audio/AudioDevice.h" #include "media/video/VideoDevice.h" #include "JavaFactories.h" @@ -79,10 +80,7 @@ namespace jni RTC_LOG(LS_ERROR) << "Device change notification failed"; } - if (env->ExceptionCheck()) { - env->ExceptionDescribe(); - env->ExceptionClear(); - } + ReportPendingException(env); } DeviceChangeListener::JavaDeviceChangeListenerClass::JavaDeviceChangeListenerClass(JNIEnv * env) diff --git a/webrtc-jni/src/main/cpp/src/media/video/codec/VideoCodecUtils.cpp b/webrtc-jni/src/main/cpp/src/media/video/codec/VideoCodecUtils.cpp index a24366109..afae14816 100644 --- a/webrtc-jni/src/main/cpp/src/media/video/codec/VideoCodecUtils.cpp +++ b/webrtc-jni/src/main/cpp/src/media/video/codec/VideoCodecUtils.cpp @@ -15,6 +15,7 @@ */ #include "media/video/codec/VideoCodecUtils.h" +#include "api/WebRTCUtils.h" #include "JavaClasses.h" #include "JavaUtils.h" #include "JNI_WebRTC.h" @@ -32,8 +33,7 @@ namespace jni // Cleared before logging, since a Java log sink cannot be called // with an exception pending. - env->ExceptionDescribe(); - env->ExceptionClear(); + ReportPendingException(env); RTC_LOG(LS_WARNING) << "Java video codec threw in " << context; diff --git a/webrtc-jni/src/main/cpp/src/rtc/LogSink.cpp b/webrtc-jni/src/main/cpp/src/rtc/LogSink.cpp index 355dca32a..42325e005 100644 --- a/webrtc-jni/src/main/cpp/src/rtc/LogSink.cpp +++ b/webrtc-jni/src/main/cpp/src/rtc/LogSink.cpp @@ -58,9 +58,22 @@ namespace jni env->CallVoidMethod(javaSink, javaClass->onLogMessage, jSeverity.get(), jMessage.get()); - // What the sink threw is reported, but not rethrown. - if (env->ExceptionCheck()) { - env->ExceptionDescribe(); + // What the sink threw is printed, but not rethrown. It is not handed + // to the uncaught exception handler, as other callbacks do: one that + // logs would come back here, to the logging lock this thread holds. + // Nor is it printed with ExceptionDescribe, which writes the first + // part to native stderr and the stack trace to System.err. + JavaLocalRef exception(env, env->ExceptionOccurred()); + + if (exception.get() != nullptr) { + env->ExceptionClear(); + + JavaLocalRef throwableClass(env, env->GetObjectClass(exception.get())); + jmethodID printStackTrace = env->GetMethodID(throwableClass.get(), "printStackTrace", "()V"); + + if (printStackTrace != nullptr) { + env->CallVoidMethod(exception.get(), printStackTrace); + } env->ExceptionClear(); } } diff --git a/webrtc-media/src/main/cpp/include/media/JniBoundary.h b/webrtc-media/src/main/cpp/include/media/JniBoundary.h index 30b2a299a..04be62acc 100644 --- a/webrtc-media/src/main/cpp/include/media/JniBoundary.h +++ b/webrtc-media/src/main/cpp/include/media/JniBoundary.h @@ -41,6 +41,13 @@ namespace ffmpeg // RuntimeException. Only to be called from a catch block. void ThrowCurrentException(JNIEnv * env) noexcept; + // Clears the Java exception pending on the calling thread, if any, and + // hands it to that thread's uncaught exception handler, as the JVM does + // for one escaping a Java thread. Unlike ExceptionDescribe, which writes + // its first line to native stderr and the stack trace to System.err, the + // whole report goes to where the handler writes it. + void ReportPendingException(JNIEnv * env) noexcept; + // Runs the body of a JNI entry point, which must not let a C++ exception // reach the VM: one that does ends the process. A C++ exception becomes a // Java one, and the entry point returns `on_error`. Whatever the body diff --git a/webrtc-media/src/main/cpp/src/media/JavaPlayerObserver.cpp b/webrtc-media/src/main/cpp/src/media/JavaPlayerObserver.cpp index 9a99ea3fd..a8756da3c 100644 --- a/webrtc-media/src/main/cpp/src/media/JavaPlayerObserver.cpp +++ b/webrtc-media/src/main/cpp/src/media/JavaPlayerObserver.cpp @@ -175,12 +175,9 @@ namespace ffmpeg void JavaPlayerObserver::ClearPendingException(JNIEnv * env) { - if (env->ExceptionCheck() == JNI_TRUE) { - // There is nowhere to throw this: the thread below is decoding, - // not running Java. Reporting and clearing keeps it from taking - // down the next unrelated JNI call instead. - env->ExceptionDescribe(); - env->ExceptionClear(); - } + // There is nowhere to throw this: the thread below is decoding, + // not running Java. Reporting and clearing keeps it from taking + // down the next unrelated JNI call instead. + ReportPendingException(env); } } diff --git a/webrtc-media/src/main/cpp/src/media/JavaRecorderObserver.cpp b/webrtc-media/src/main/cpp/src/media/JavaRecorderObserver.cpp index a44a46e49..459a5a14f 100644 --- a/webrtc-media/src/main/cpp/src/media/JavaRecorderObserver.cpp +++ b/webrtc-media/src/main/cpp/src/media/JavaRecorderObserver.cpp @@ -175,9 +175,6 @@ namespace ffmpeg void JavaRecorderObserver::ClearPendingException(JNIEnv * env) { - if (env->ExceptionCheck() == JNI_TRUE) { - env->ExceptionDescribe(); - env->ExceptionClear(); - } + ReportPendingException(env); } } diff --git a/webrtc-media/src/main/cpp/src/media/JniBoundary.cpp b/webrtc-media/src/main/cpp/src/media/JniBoundary.cpp index c8ddd9eef..e26e7a63c 100644 --- a/webrtc-media/src/main/cpp/src/media/JniBoundary.cpp +++ b/webrtc-media/src/main/cpp/src/media/JniBoundary.cpp @@ -204,6 +204,47 @@ namespace ffmpeg } } + void ReportPendingException(JNIEnv * env) noexcept + { + if (env->ExceptionCheck() == JNI_FALSE) { + return; + } + + LocalRef exception(env, env->ExceptionOccurred()); + env->ExceptionClear(); + + LocalRef thread_class(env, env->FindClass("java/lang/Thread")); + LocalRef handler_class(env, env->FindClass("java/lang/Thread$UncaughtExceptionHandler")); + + if (env->ExceptionCheck() == JNI_TRUE) { + env->ExceptionClear(); + return; + } + + jmethodID current_thread = env->GetStaticMethodID(thread_class.get(), "currentThread", "()Ljava/lang/Thread;"); + jmethodID get_handler = env->GetMethodID(thread_class.get(), "getUncaughtExceptionHandler", + "()Ljava/lang/Thread$UncaughtExceptionHandler;"); + jmethodID uncaught_exception = env->GetMethodID(handler_class.get(), "uncaughtException", + "(Ljava/lang/Thread;Ljava/lang/Throwable;)V"); + + if (env->ExceptionCheck() == JNI_TRUE) { + env->ExceptionClear(); + return; + } + + LocalRef thread(env, env->CallStaticObjectMethod(thread_class.get(), current_thread)); + LocalRef handler(env, env->CallObjectMethod(thread.get(), get_handler)); + + if (env->ExceptionCheck() == JNI_FALSE && handler.get() != nullptr) { + env->CallVoidMethod(handler.get(), uncaught_exception, thread.get(), exception.get()); + } + + // Like the JVM, ignore an exception thrown by the handler itself. + if (env->ExceptionCheck() == JNI_TRUE) { + env->ExceptionClear(); + } + } + JavaStringChars::JavaStringChars(JNIEnv * env, jstring string) noexcept : env_(env), string_(string), diff --git a/webrtc-media/src/test/java/dev/onvoid/webrtc/media/player/MediaPlayerTest.java b/webrtc-media/src/test/java/dev/onvoid/webrtc/media/player/MediaPlayerTest.java index d02d5353a..9d7ebedfe 100644 --- a/webrtc-media/src/test/java/dev/onvoid/webrtc/media/player/MediaPlayerTest.java +++ b/webrtc-media/src/test/java/dev/onvoid/webrtc/media/player/MediaPlayerTest.java @@ -400,6 +400,47 @@ public void onEndOfStream() { } } + @Test + void reportsWhatTheListenerThrows() throws Exception { + // What the listener throws on the player's thread goes to that + // thread's uncaught exception handler, as it would on a Java thread, + // rather than half to native stderr through ExceptionDescribe. + CountDownLatch reported = new CountDownLatch(1); + Thread.UncaughtExceptionHandler previous = Thread.getDefaultUncaughtExceptionHandler(); + + Thread.setDefaultUncaughtExceptionHandler((thread, e) -> { + if (e instanceof IllegalStateException && "listener fails on purpose".equals(e.getMessage())) { + reported.countDown(); + } + else if (previous != null) { + previous.uncaughtException(thread, e); + } + }); + + try (Sources sources = new Sources()) { + MediaPlayer player = new MediaPlayer(new MediaReader(asset()), + sources.video, sources.audio); + + player.setListener(new MediaPlayerListener() { + + @Override + public void onEndOfStream() { + throw new IllegalStateException("listener fails on purpose"); + } + }); + + player.seek(2_500_000); + player.play(); + + assertTrue(reported.await(10, TimeUnit.SECONDS), "not reported"); + + player.close(); + } + finally { + Thread.setDefaultUncaughtExceptionHandler(previous); + } + } + @Test void closesFromStateChangeOfCommand() throws Exception { try (Sources sources = new Sources()) { diff --git a/webrtc/src/test/java/dev/onvoid/webrtc/VideoCodecFactoryTests.java b/webrtc/src/test/java/dev/onvoid/webrtc/VideoCodecFactoryTests.java index e7bc1ac22..ba487a3ef 100644 --- a/webrtc/src/test/java/dev/onvoid/webrtc/VideoCodecFactoryTests.java +++ b/webrtc/src/test/java/dev/onvoid/webrtc/VideoCodecFactoryTests.java @@ -365,6 +365,21 @@ public VideoEncoder createEncoder(VideoCodecInfo info) { // exception was still pending. Logging.addLogSink(Logging.Severity.WARNING, (severity, message) -> { }); + // What the factory threw goes to the uncaught exception handler of the + // thread that called it, as it would from a Java thread, rather than + // half to native stderr through ExceptionDescribe. + CountDownLatch reported = new CountDownLatch(1); + Thread.UncaughtExceptionHandler previous = Thread.getDefaultUncaughtExceptionHandler(); + + Thread.setDefaultUncaughtExceptionHandler((thread, e) -> { + if (e instanceof IllegalStateException && "createEncoder fails on purpose".equals(e.getMessage())) { + reported.countDown(); + } + else if (previous != null) { + previous.uncaughtException(thread, e); + } + }); + PeerConnectionFactory built = testCodecFactory(encoderFactory, new TestDecoderFactory()); try (TestMediaCall call = new TestMediaCall(built, true, false, TEST_CODEC)) { @@ -379,9 +394,11 @@ public VideoEncoder createEncoder(VideoCodecInfo info) { } assertTrue(encoderFactory.created.get() > 0, "the encoder factory was never asked"); + assertTrue(reported.await(TIMEOUT_SECONDS, TimeUnit.SECONDS), "the failure was not reported"); } finally { built.dispose(); + Thread.setDefaultUncaughtExceptionHandler(previous); } }