From 031ff03b61da3c0ae60ae138274b69ca396c4372 Mon Sep 17 00:00:00 2001 From: Alex Andres Date: Mon, 26 Oct 2026 18:02:25 +0100 Subject: [PATCH] fix: keep DTMF senders alive and observers of other wrappers registered getDtmfSender() handed out a pointer it held no reference on, so using the DTMF sender after the connection had closed and the RTCRtpSender was disposed crashed the VM. The Java object now holds a reference, released by the new RTCDtmfSender.dispose(). Unregistering or disposing one wrapper of a DTLS transport, sender, receiver or DTMF sender removed the observer another wrapper of the same native object had registered since. A process-wide registry now records which observer is registered, and a wrapper only unregisters its own. --- docs/guide/audio/dtmf-sender.md | 17 +++-- .../src/main/cpp/include/JNI_RTCDtmfSender.h | 8 ++ .../cpp/include/api/RegisteredObservers.h | 46 +++++++++++ .../src/main/cpp/src/JNI_RTCDtlsTransport.cpp | 41 ++++++---- .../src/main/cpp/src/JNI_RTCDtmfSender.cpp | 51 +++++++++++-- .../src/main/cpp/src/JNI_RTCRtpReceiver.cpp | 23 ++++-- .../src/main/cpp/src/JNI_RTCRtpSender.cpp | 27 +++++-- .../main/cpp/src/api/RegisteredObservers.cpp | 74 ++++++++++++++++++ .../dev/onvoid/webrtc/RTCDtlsTransport.java | 7 +- .../java/dev/onvoid/webrtc/RTCDtmfSender.java | 42 +++++++--- .../dev/onvoid/webrtc/RTCRtpReceiver.java | 10 +-- .../java/dev/onvoid/webrtc/RTCRtpSender.java | 12 ++- .../dev/onvoid/webrtc/client/SenderImpl.java | 3 +- .../onvoid/webrtc/RTCDtlsTransportTests.java | 62 +++++++++++++++ .../dev/onvoid/webrtc/RTCDtmfSenderTests.java | 76 ++++++++++++++++++- .../webrtc/RTCRtpSenderReceiverTests.java | 40 ++++++++++ 16 files changed, 479 insertions(+), 60 deletions(-) create mode 100644 webrtc-jni/src/main/cpp/include/api/RegisteredObservers.h create mode 100644 webrtc-jni/src/main/cpp/src/api/RegisteredObservers.cpp diff --git a/docs/guide/audio/dtmf-sender.md b/docs/guide/audio/dtmf-sender.md index c3283cf71..a8ab3883a 100644 --- a/docs/guide/audio/dtmf-sender.md +++ b/docs/guide/audio/dtmf-sender.md @@ -132,16 +132,21 @@ int interToneGap = dtmfSender.interToneGap(); ## Cleanup -When you're done with the DTMF sender, you should unregister any observers: +Each call to `getDtmfSender()` returns a new `RTCDtmfSender` that holds its own reference to the +native DTMF sender. It stays safe to use after the connection has closed, when `canInsertDtmf()` +returns `false`, until you dispose it. Disposing also unregisters the observer registered through +it: ```java -// Unregister the observer -dtmfSender.unregisterObserver(); +dtmfSender.dispose(); ``` -`RTCDtmfSender` has no `dispose()` method of its own; it is tied to the native sender's lifetime. -The `RTCRtpSender` it came from, however, is not owned by the peer connection, so make sure to -dispose that once you're done with it: +To stop listening without disposing, call `dtmfSender.unregisterObserver()`. It removes only the +observer registered through that instance; one registered since through another `RTCDtmfSender` of +the same sender stays. + +The `RTCRtpSender` the DTMF sender came from is not owned by the peer connection either, so make +sure to dispose that once you're done with it: ```java sender.dispose(); diff --git a/webrtc-jni/src/main/cpp/include/JNI_RTCDtmfSender.h b/webrtc-jni/src/main/cpp/include/JNI_RTCDtmfSender.h index 6a25bb55e..2d5c62664 100644 --- a/webrtc-jni/src/main/cpp/include/JNI_RTCDtmfSender.h +++ b/webrtc-jni/src/main/cpp/include/JNI_RTCDtmfSender.h @@ -63,6 +63,14 @@ extern "C" { JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCDtmfSender_unregisterObserver (JNIEnv *, jobject); + /* + * Class: dev_onvoid_webrtc_RTCDtmfSender + * Method: dispose + * Signature: ()V + */ + JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCDtmfSender_dispose + (JNIEnv *, jobject); + #ifdef __cplusplus } #endif diff --git a/webrtc-jni/src/main/cpp/include/api/RegisteredObservers.h b/webrtc-jni/src/main/cpp/include/api/RegisteredObservers.h new file mode 100644 index 000000000..5e6356a2e --- /dev/null +++ b/webrtc-jni/src/main/cpp/include/api/RegisteredObservers.h @@ -0,0 +1,46 @@ +/* + * Copyright 2026 Alex Andres + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef JNI_WEBRTC_API_REGISTERED_OBSERVERS_H_ +#define JNI_WEBRTC_API_REGISTERED_OBSERVERS_H_ + +namespace jni +{ + // Which native observer is registered on a WebRTC object that has room for + // one, such as a sender, a receiver or a DTLS transport. + // + // Many Java objects may stand for the same native object, and each owns + // the observer registered through it. Registering through one replaces the + // observer of another, which then must not unregister the one that + // replaced it when it is disposed. The Java objects learn here whether the + // observer they own is still the registered one. + // + // The registry is told before WebRTC is, and its lock is never held while + // WebRTC is called: some observers are called on the thread that runs the + // registration, and their Java code may register again. + namespace RegisteredObservers + { + // Records the observer as the one registered on the owner, in place of + // any other. + void Set(const void * owner, const void * observer); + + // Forgets the observer if it is the one registered on the owner, and + // returns whether it was. Only then is it to be unregistered. + bool Remove(const void * owner, const void * observer); + } +} + +#endif diff --git a/webrtc-jni/src/main/cpp/src/JNI_RTCDtlsTransport.cpp b/webrtc-jni/src/main/cpp/src/JNI_RTCDtlsTransport.cpp index 5b66bfc9c..5c239516b 100644 --- a/webrtc-jni/src/main/cpp/src/JNI_RTCDtlsTransport.cpp +++ b/webrtc-jni/src/main/cpp/src/JNI_RTCDtlsTransport.cpp @@ -15,6 +15,7 @@ */ #include "JNI_RTCDtlsTransport.h" +#include "api/RegisteredObservers.h" #include "api/RTCDtlsTransportObserver.h" #include "rtc/RTCCertificatePEM.h" #include "JavaArrayList.h" @@ -89,6 +90,24 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCDtlsTransport_getRemoteCerti } } +namespace +{ + // Unregisters and frees the observer registered through the Java object. + // WebRTC is told only while that observer is still the registered one: + // another Java object for the same transport may have registered its own + // since, which stays. + void UnregisterOwnObserver(JNIEnv * env, jobject caller, webrtc::DtlsTransportInterface * transport) + { + auto observer = GetHandle(env, caller, "observerHandle"); + + if (jni::RegisteredObservers::Remove(transport, observer)) { + transport->UnregisterObserver(); + } + + ClearNativeObserver(env, caller, "observerHandle"); + } +} + JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCDtlsTransport_registerObserver (JNIEnv * env, jobject caller, jobject observer) { @@ -96,14 +115,16 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCDtlsTransport_registerObserver webrtc::DtlsTransportInterface * transport = GetHandle(env, caller); CHECK_HANDLE(transport); - // Unregister and free a previously registered observer before replacing - // it, otherwise it would leak along with its JNI global reference. - transport->UnregisterObserver(); - auto newObserver = new jni::RTCDtlsTransportObserver(env, jni::JavaGlobalRef(env, observer)); - ReplaceNativeObserver(env, caller, "observerHandle", newObserver); + + // Registering replaces whatever observer the transport had, also one + // registered through another Java object, so the one registered + // through this object before is out of use and can be freed. + jni::RegisteredObservers::Set(transport, newObserver); transport->RegisterObserver(newObserver); + + ReplaceNativeObserver(env, caller, "observerHandle", newObserver); } catch (...) { ThrowCxxJavaException(env); @@ -116,9 +137,7 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCDtlsTransport_unregisterObserve webrtc::DtlsTransportInterface * transport = GetHandle(env, caller); CHECK_HANDLE(transport); - transport->UnregisterObserver(); - - ClearNativeObserver(env, caller, "observerHandle"); + UnregisterOwnObserver(env, caller, transport); } JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCDtlsTransport_dispose @@ -129,11 +148,7 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCDtlsTransport_dispose // WebRTC must let go of an observer registered through this object before // it is deleted. - if (GetHandle(env, caller, "observerHandle") != nullptr) { - transport->UnregisterObserver(); - - ClearNativeObserver(env, caller, "observerHandle"); - } + UnregisterOwnObserver(env, caller, transport); // The connection, its senders and receivers, and other Java objects keep // their own references, so this one need not be the last. diff --git a/webrtc-jni/src/main/cpp/src/JNI_RTCDtmfSender.cpp b/webrtc-jni/src/main/cpp/src/JNI_RTCDtmfSender.cpp index 2a0a157f2..fd3c861fe 100644 --- a/webrtc-jni/src/main/cpp/src/JNI_RTCDtmfSender.cpp +++ b/webrtc-jni/src/main/cpp/src/JNI_RTCDtmfSender.cpp @@ -15,12 +15,31 @@ */ #include "JNI_RTCDtmfSender.h" +#include "api/RegisteredObservers.h" #include "api/RTCDtmfSenderObserver.h" #include "JavaString.h" #include "JavaUtils.h" #include "api/dtmf_sender_interface.h" +namespace +{ + // Unregisters and frees the observer registered through the Java object. + // WebRTC is told only while that observer is still the registered one: + // another Java object for the same DTMF sender may have registered its + // own since, which stays. + void UnregisterOwnObserver(JNIEnv * env, jobject caller, webrtc::DtmfSenderInterface * sender) + { + auto observer = GetHandle(env, caller, "observerHandle"); + + if (jni::RegisteredObservers::Remove(sender, observer)) { + sender->UnregisterObserver(); + } + + ClearNativeObserver(env, caller, "observerHandle"); + } +} + JNIEXPORT jboolean JNICALL Java_dev_onvoid_webrtc_RTCDtmfSender_canInsertDtmf (JNIEnv * env, jobject caller) { @@ -87,14 +106,17 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCDtmfSender_registerObserver webrtc::DtmfSenderInterface * sender = GetHandle(env, caller); CHECK_HANDLE(sender); - // Unregister and free a previously registered observer before replacing - // it, otherwise it would leak along with its JNI global reference. - sender->UnregisterObserver(); - auto observer = new jni::RTCDtmfSenderObserver(env, jni::JavaGlobalRef(env, jObserver)); - ReplaceNativeObserver(env, caller, "observerHandle", observer); + + // Registering replaces whatever observer the DTMF sender had, also one + // registered through another Java object. It runs on the signaling + // thread, which is also the thread that calls the observer, so the one + // registered through this object before is out of use once it returns. + jni::RegisteredObservers::Set(sender, observer); sender->RegisterObserver(observer); + + ReplaceNativeObserver(env, caller, "observerHandle", observer); } catch (...) { ThrowCxxJavaException(env); @@ -107,7 +129,22 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCDtmfSender_unregisterObserver webrtc::DtmfSenderInterface * sender = GetHandle(env, caller); CHECK_HANDLE(sender); - sender->UnregisterObserver(); + UnregisterOwnObserver(env, caller, sender); +} + +JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCDtmfSender_dispose +(JNIEnv * env, jobject caller) +{ + webrtc::DtmfSenderInterface * sender = GetHandle(env, caller); + CHECK_HANDLE(sender); + + // WebRTC must let go of an observer registered through this object before + // it is deleted. + UnregisterOwnObserver(env, caller, sender); + + // The RTCRtpSender and other Java objects keep their own references, so + // this one need not be the last. + sender->Release(); - ClearNativeObserver(env, caller, "observerHandle"); + SetHandle(env, caller, nullptr); } \ No newline at end of file diff --git a/webrtc-jni/src/main/cpp/src/JNI_RTCRtpReceiver.cpp b/webrtc-jni/src/main/cpp/src/JNI_RTCRtpReceiver.cpp index d4e805094..da95519e9 100644 --- a/webrtc-jni/src/main/cpp/src/JNI_RTCRtpReceiver.cpp +++ b/webrtc-jni/src/main/cpp/src/JNI_RTCRtpReceiver.cpp @@ -15,6 +15,7 @@ */ #include "JNI_RTCRtpReceiver.h" +#include "api/RegisteredObservers.h" #include "api/RTCRtpParameters.h" #include "api/RTCRtpReceiverObserver.h" #include "api/RTCRtpContributingSource.h" @@ -204,10 +205,20 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCRtpReceiver_setObserver observer = new jni::RTCRtpReceiverObserver(env, jni::JavaGlobalRef(env, jObserver)); } + // Setting replaces whatever observer the receiver had, also one set + // through another Java object. Removing removes only the one set + // through this object, unless another has replaced it since. + if (observer != nullptr) { + jni::RegisteredObservers::Set(receiver, observer); + + receiver->SetObserver(observer); + } + else if (jni::RegisteredObservers::Remove(receiver, GetHandle(env, caller, "observerHandle"))) { + receiver->SetObserver(nullptr); + } + // Runs on the signaling thread, which is also the thread that calls // the observer, so the previous one is out of use once this returns. - receiver->SetObserver(observer); - ReplaceNativeObserver(env, caller, "observerHandle", observer); } catch (...) { @@ -222,13 +233,13 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCRtpReceiver_dispose CHECK_HANDLE(receiver); // WebRTC must let go of an observer set through this object before it is - // deleted. - if (GetHandle(env, caller, "observerHandle") != nullptr) { + // deleted. One that another Java object has set since stays. + if (jni::RegisteredObservers::Remove(receiver, GetHandle(env, caller, "observerHandle"))) { receiver->SetObserver(nullptr); - - ClearNativeObserver(env, caller, "observerHandle"); } + ClearNativeObserver(env, caller, "observerHandle"); + // Unlike e.g. MediaStreamTrack, an RTCRtpReceiver is not exclusively // owned by one Java wrapper: the owning RtpTransceiver keeps its own // reference, and other Java wrappers may have been obtained via separate diff --git a/webrtc-jni/src/main/cpp/src/JNI_RTCRtpSender.cpp b/webrtc-jni/src/main/cpp/src/JNI_RTCRtpSender.cpp index a64c7f335..f5a3e8d79 100644 --- a/webrtc-jni/src/main/cpp/src/JNI_RTCRtpSender.cpp +++ b/webrtc-jni/src/main/cpp/src/JNI_RTCRtpSender.cpp @@ -16,6 +16,7 @@ #include "JNI_RTCRtpSender.h" #include "api/EncodedFrameInjectorObserver.h" +#include "api/RegisteredObservers.h" #include "api/RTCRtpSenderObserver.h" #include "api/RTCRtpSendParameters.h" #include "api/WebRTCUtils.h" @@ -146,7 +147,9 @@ JNIEXPORT jobject JNICALL Java_dev_onvoid_webrtc_RTCRtpSender_getDtmfSender auto jDtmfSender = jni::RTCDtmfSender::toJava(env); - SetHandle(env, jDtmfSender, dtmfSender.get()); + // The Java object holds a reference until it is disposed, since the + // sender frees its DTMF sender once the connection drops the sender. + SetHandle(env, jDtmfSender, dtmfSender.release()); return jDtmfSender.release(); } @@ -320,10 +323,20 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCRtpSender_setObserver observer = new jni::RTCRtpSenderObserver(env, jni::JavaGlobalRef(env, jObserver)); } + // Setting replaces whatever observer the sender had, also one set + // through another Java object. Removing removes only the one set + // through this object, unless another has replaced it since. + if (observer != nullptr) { + jni::RegisteredObservers::Set(sender, observer); + + sender->SetObserver(observer); + } + else if (jni::RegisteredObservers::Remove(sender, GetHandle(env, caller, "observerHandle"))) { + sender->SetObserver(nullptr); + } + // Runs on the signaling thread, which is also the thread that calls // the observer, so the previous one is out of use once this returns. - sender->SetObserver(observer); - ReplaceNativeObserver(env, caller, "observerHandle", observer); } catch (...) { @@ -338,13 +351,13 @@ JNIEXPORT void JNICALL Java_dev_onvoid_webrtc_RTCRtpSender_dispose CHECK_HANDLE(sender); // WebRTC must let go of an observer set through this object before it is - // deleted. - if (GetHandle(env, caller, "observerHandle") != nullptr) { + // deleted. One that another Java object has set since stays. + if (jni::RegisteredObservers::Remove(sender, GetHandle(env, caller, "observerHandle"))) { sender->SetObserver(nullptr); - - ClearNativeObserver(env, caller, "observerHandle"); } + ClearNativeObserver(env, caller, "observerHandle"); + // Unlike e.g. MediaStreamTrack, an RTCRtpSender is not exclusively owned // by one Java wrapper: the owning RtpTransceiver keeps its own reference, // and other Java wrappers may have been obtained via separate diff --git a/webrtc-jni/src/main/cpp/src/api/RegisteredObservers.cpp b/webrtc-jni/src/main/cpp/src/api/RegisteredObservers.cpp new file mode 100644 index 000000000..b09529926 --- /dev/null +++ b/webrtc-jni/src/main/cpp/src/api/RegisteredObservers.cpp @@ -0,0 +1,74 @@ +/* + * Copyright 2026 Alex Andres + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "api/RegisteredObservers.h" + +#include +#include + +namespace jni +{ + namespace + { + using RegistryMap = std::unordered_map; + + // Never destroyed: a Java object may be disposed while the process + // shuts down, after static destructors have run. + std::mutex & RegistryMutex() + { + static std::mutex * mutex = new std::mutex(); + return *mutex; + } + + // An entry stays only as long as the Java object that registered the + // observer, which holds a reference on the owner, so an owner's address + // is not reused while its entry is here. + RegistryMap & Registry() + { + static RegistryMap * registry = new RegistryMap(); + return *registry; + } + } + + namespace RegisteredObservers + { + void Set(const void * owner, const void * observer) + { + std::lock_guard lock(RegistryMutex()); + + Registry()[owner] = observer; + } + + bool Remove(const void * owner, const void * observer) + { + if (observer == nullptr) { + return false; + } + + std::lock_guard lock(RegistryMutex()); + + auto found = Registry().find(owner); + + if (found == Registry().end() || found->second != observer) { + return false; + } + + Registry().erase(found); + + return true; + } + } +} diff --git a/webrtc/src/main/java/dev/onvoid/webrtc/RTCDtlsTransport.java b/webrtc/src/main/java/dev/onvoid/webrtc/RTCDtlsTransport.java index fe86728fc..9aca15673 100644 --- a/webrtc/src/main/java/dev/onvoid/webrtc/RTCDtlsTransport.java +++ b/webrtc/src/main/java/dev/onvoid/webrtc/RTCDtlsTransport.java @@ -80,13 +80,18 @@ public class RTCDtlsTransport extends DisposableNativeObject { /** * Register an observer to receive events from this transport. The observer * will replace the previously registered observer. + *

+ * A transport has one observer, also when several RTCDtlsTransport + * instances stand for it: registering through one replaces the observer + * registered through another. * * @param observer The new DTLS transport observer. */ public native void registerObserver(RTCDtlsTransportObserver observer); /** - * Unregister the last set RTCDtlsTransportObserver. + * Unregister the RTCDtlsTransportObserver last registered through this + * instance. An observer that another instance has registered since stays. */ public native void unregisterObserver(); diff --git a/webrtc/src/main/java/dev/onvoid/webrtc/RTCDtmfSender.java b/webrtc/src/main/java/dev/onvoid/webrtc/RTCDtmfSender.java index a3b31976d..2eb92c21f 100644 --- a/webrtc/src/main/java/dev/onvoid/webrtc/RTCDtmfSender.java +++ b/webrtc/src/main/java/dev/onvoid/webrtc/RTCDtmfSender.java @@ -16,7 +16,7 @@ package dev.onvoid.webrtc; -import dev.onvoid.webrtc.internal.NativeObject; +import dev.onvoid.webrtc.internal.DisposableNativeObject; /** * Represents a DTMF (Dual-Tone Multi-Frequency) sender for WebRTC. @@ -29,18 +29,24 @@ * DTMF tones are the audible tones generated when pressing keys on a telephone keypad. * The supported DTMF tones are: 0-9, A-D, *, and #. In addition, the special characters * ',' (comma) can be used to insert a 2-second delay between tones. + *

+ * Each instance holds a reference to the native DTMF sender, which keeps it + * usable after the connection has closed or dropped its {@link RTCRtpSender}; + * it then can no longer insert tones. Release it with {@link #dispose()} once + * it is no longer needed. * * @author Alex Andres * * @see W3C WebRTC 1.0: RTCDtmfSender */ -public class RTCDtmfSender extends NativeObject { +public class RTCDtmfSender extends DisposableNativeObject { /** * The native observer registered via {@link #registerObserver}. The * RTCDtmfSender does not take ownership of the Java observer, but it - * owns the native observer wrapper; it is freed when replaced and when - * {@link #unregisterObserver()} is called. + * owns the native observer wrapper; it is freed when replaced, when + * {@link #unregisterObserver()} is called, and when this instance is + * disposed. */ @SuppressWarnings("unused") private long observerHandle; @@ -135,22 +141,40 @@ public class RTCDtmfSender extends NativeObject { * Registers an observer to receive notifications about DTMF-related events. *

* Only one observer can be registered at a time. Registering a new observer - * when one is already registered will replace the existing observer. + * when one is already registered will replace the existing observer, also + * one registered through another RTCDtmfSender instance of the same + * {@link RTCRtpSender}. * * @param observer The DTMF sender observer to register. */ public native void registerObserver(RTCDtmfSenderObserver observer); /** - * Unregisters the previously registered DTMF sender observer. + * Unregisters the DTMF sender observer last registered through this + * instance. *

- * After calling this method, no observer will receive notifications about - * DTMF-related events until a new observer is registered. + * After calling this method, that observer receives no more notifications + * about DTMF-related events. An observer that another instance has + * registered since stays registered. *

- * If no observer is currently registered, this method has no effect. + * If no observer is currently registered through this instance, this + * method has no effect. */ public native void unregisterObserver(); + /** + * Unregisters the observer registered through this instance, if any, and + * releases the native reference held by this instance. + *

+ * The DTMF sender is not exclusively owned by this instance: its {@link + * RTCRtpSender} and other RTCDtmfSender instances obtained via separate + * calls to {@link RTCRtpSender#getDtmfSender()} keep their own references. + * Disposing this instance does not affect the DTMF sender itself or any + * other instance referring to it. + */ + @Override + public native void dispose(); + /** * Two RTCDtmfSender instances are equal if they are bound to the same * native sender, e.g. when obtained from two separate calls to {@link diff --git a/webrtc/src/main/java/dev/onvoid/webrtc/RTCRtpReceiver.java b/webrtc/src/main/java/dev/onvoid/webrtc/RTCRtpReceiver.java index cbb3bcab0..c6b346e66 100644 --- a/webrtc/src/main/java/dev/onvoid/webrtc/RTCRtpReceiver.java +++ b/webrtc/src/main/java/dev/onvoid/webrtc/RTCRtpReceiver.java @@ -58,11 +58,11 @@ private RTCRtpReceiver() { * RTP packet, replacing any observer set before. {@code null} removes the * observer. *

- * A receiver has one observer. It belongs to the RTCRtpReceiver object it - * was set through: disposing that object removes it, also when it was set - * through another object of the same receiver in the meantime, so set it - * through one object, such as the one {@link - * PeerConnectionObserver#onTrack} provides. + * A receiver has one observer, also when several RTCRtpReceiver objects + * stand for it: setting it through one replaces the observer set through + * another. Removing it, or disposing this object, removes only the + * observer set through this object, and only while no other object has + * replaced it. * * @param observer The observer, or {@code null}. */ diff --git a/webrtc/src/main/java/dev/onvoid/webrtc/RTCRtpSender.java b/webrtc/src/main/java/dev/onvoid/webrtc/RTCRtpSender.java index d8a54851f..d2c041b0b 100644 --- a/webrtc/src/main/java/dev/onvoid/webrtc/RTCRtpSender.java +++ b/webrtc/src/main/java/dev/onvoid/webrtc/RTCRtpSender.java @@ -60,10 +60,11 @@ private RTCRtpSender() { * packet, replacing any observer set before. {@code null} removes the * observer. *

- * A sender has one observer. It belongs to the RTCRtpSender object it was - * set through: disposing that object removes it, also when it was set - * through another object of the same sender in the meantime, so set it - * through one object. + * A sender has one observer, also when several RTCRtpSender objects stand + * for it: setting it through one replaces the observer set through + * another. Removing it, or disposing this object, removes only the + * observer set through this object, and only while no other object has + * replaced it. * * @param observer The observer, or {@code null}. */ @@ -153,6 +154,9 @@ private RTCRtpSender() { * Returns the RTCDtmfSender associated with this RTCRtpSender. The RTCDtmfSender * enables the transmission of DTMF (Dual-Tone Multi-Frequency) tones over the * RTCPeerConnection. + *

+ * Each call returns a new instance holding its own reference to the DTMF + * sender, to be released with {@link RTCDtmfSender#dispose()}. * * @return The DTMF sender object associated with this RTP sender, or null if DTMF * is not supported for the media type of the associated track. diff --git a/webrtc/src/main/java/dev/onvoid/webrtc/client/SenderImpl.java b/webrtc/src/main/java/dev/onvoid/webrtc/client/SenderImpl.java index 9b06d5a2a..90a810c51 100644 --- a/webrtc/src/main/java/dev/onvoid/webrtc/client/SenderImpl.java +++ b/webrtc/src/main/java/dev/onvoid/webrtc/client/SenderImpl.java @@ -350,7 +350,8 @@ private void release() { removed = true; if (dtmf != null) { - dtmf.unregisterObserver(); + // Unregisters the observer as well. + dtmf.dispose(); dtmf = null; } if (pendingTones != null) { diff --git a/webrtc/src/test/java/dev/onvoid/webrtc/RTCDtlsTransportTests.java b/webrtc/src/test/java/dev/onvoid/webrtc/RTCDtlsTransportTests.java index 2bc10edd5..ea5a946bd 100644 --- a/webrtc/src/test/java/dev/onvoid/webrtc/RTCDtlsTransportTests.java +++ b/webrtc/src/test/java/dev/onvoid/webrtc/RTCDtlsTransportTests.java @@ -225,6 +225,68 @@ public void onError(String error) { } assertDoesNotThrow(transport::unregisterObserver); } + @Test + void unregisterKeepsObserverOfOtherInstance() throws Exception { + assertObserverOfOtherInstanceStays(false); + } + + @Test + void disposeKeepsObserverOfOtherInstance() throws Exception { + assertObserverOfOtherInstanceStays(true); + } + + private void assertObserverOfOtherInstanceStays(boolean dispose) throws Exception { + RTCDtlsTransport other = sender.getTransport(); + + AtomicInteger firstObserverCalls = new AtomicInteger(); + CompletableFuture closedState = new CompletableFuture<>(); + + try { + transport.registerObserver(new RTCDtlsTransportObserver() { + @Override + public void onStateChange(RTCDtlsTransportState state) { + firstObserverCalls.incrementAndGet(); + } + + @Override + public void onError(String error) { } + }); + other.registerObserver(new RTCDtlsTransportObserver() { + @Override + public void onStateChange(RTCDtlsTransportState state) { + if (state == RTCDtlsTransportState.CLOSED) { + closedState.complete(state); + } + } + + @Override + public void onError(String error) { } + }); + + // The observer registered through the other instance replaced the + // first one, which leaves this instance nothing to remove from the + // transport. + if (dispose) { + transport.dispose(); + transport = null; + } + else { + transport.unregisterObserver(); + } + + caller.close(); + callee.close(); + sender.dispose(); + + assertEquals(RTCDtlsTransportState.CLOSED, closedState.get(5, TimeUnit.SECONDS), + "The observer of the other instance was removed"); + assertEquals(0, firstObserverCalls.get(), "The replaced observer must not receive events"); + } + finally { + other.dispose(); + } + } + @Test void observerCanUseRtpSender() throws Exception { // WebRTC reports DTLS states on the network thread, which is also the diff --git a/webrtc/src/test/java/dev/onvoid/webrtc/RTCDtmfSenderTests.java b/webrtc/src/test/java/dev/onvoid/webrtc/RTCDtmfSenderTests.java index b4a069ceb..8b4add500 100644 --- a/webrtc/src/test/java/dev/onvoid/webrtc/RTCDtmfSenderTests.java +++ b/webrtc/src/test/java/dev/onvoid/webrtc/RTCDtmfSenderTests.java @@ -81,6 +81,7 @@ boolean awaitCompletion() throws InterruptedException { TestPeerConnection caller; TestPeerConnection callee; + RTCRtpSender sender; RTCDtmfSender dtmfSender; @@ -98,7 +99,7 @@ void init() throws Exception { List streamIds = new ArrayList<>(); streamIds.add("stream1"); - RTCRtpSender sender = peerConnection.addTrack(audioTrack, streamIds); + sender = peerConnection.addTrack(audioTrack, streamIds); dtmfSender = sender.getDtmfSender(); caller.setRemotePeerConnection(callee); @@ -123,6 +124,79 @@ void init() throws Exception { void dispose() { caller.close(); callee.close(); + + if (sender != null) { + sender.dispose(); + } + dtmfSender.dispose(); + } + + @Test + void dtmfSenderOutlivesClosedConnection() { + TestDtmfSenderObserver observer = new TestDtmfSenderObserver(); + + dtmfSender.registerObserver(observer); + + // Closing frees the connection's senders, and with the RTCRtpSender + // gone nothing else refers to the DTMF sender. The instance must keep + // the DTMF sender it refers to alive, rather than point to freed + // memory; this used to crash the VM. + caller.close(); + callee.close(); + sender.dispose(); + sender = null; + + for (int i = 0; i < 1000; i++) { + assertFalse(dtmfSender.canInsertDtmf()); + assertFalse(dtmfSender.insertDtmf("1", 100, 70)); + assertNull(dtmfSender.tones()); + } + + assertDoesNotThrow(dtmfSender::unregisterObserver); + } + + @Test + void unregisterKeepsObserverOfOtherInstance() throws InterruptedException { + assertObserverOfOtherInstanceStays(false); + } + + @Test + void disposeKeepsObserverOfOtherInstance() throws InterruptedException { + assertObserverOfOtherInstanceStays(true); + } + + private void assertObserverOfOtherInstanceStays(boolean dispose) throws InterruptedException { + RTCDtmfSender first = sender.getDtmfSender(); + RTCDtmfSender second = sender.getDtmfSender(); + + TestDtmfSenderObserver firstObserver = new TestDtmfSenderObserver(); + TestDtmfSenderObserver secondObserver = new TestDtmfSenderObserver(); + + try { + first.registerObserver(firstObserver); + second.registerObserver(secondObserver); + + // The second observer replaced the first, which leaves the first + // instance nothing to remove from the DTMF sender. + if (dispose) { + first.dispose(); + } + else { + first.unregisterObserver(); + } + + assertTrue(dtmfSender.insertDtmf("1", 100, 70)); + assertTrue(secondObserver.awaitCompletion(), "The observer of the other instance was removed"); + + assertEquals(Arrays.asList("1", null), secondObserver.getTones()); + assertTrue(firstObserver.getTones().isEmpty(), "The replaced observer must not receive events"); + } + finally { + if (!dispose) { + first.dispose(); + } + second.dispose(); + } } @Test diff --git a/webrtc/src/test/java/dev/onvoid/webrtc/RTCRtpSenderReceiverTests.java b/webrtc/src/test/java/dev/onvoid/webrtc/RTCRtpSenderReceiverTests.java index 5b41d963d..767c7cffc 100644 --- a/webrtc/src/test/java/dev/onvoid/webrtc/RTCRtpSenderReceiverTests.java +++ b/webrtc/src/test/java/dev/onvoid/webrtc/RTCRtpSenderReceiverTests.java @@ -95,6 +95,46 @@ void firstPacketEvents() throws Exception { } } + @Test + void disposeKeepsObserverOfOtherObject() throws Exception { + try (TestMediaCall call = new TestMediaCall(factory, true, false)) { + call.negotiate(); + + RTCRtpSender sender = call.getCallerSenders()[0]; + RTCRtpReceiver receiver = call.getReceiver(MediaStreamTrack.VIDEO_TRACK_KIND); + RTCRtpReceiver otherReceiver = call.getReceiver(MediaStreamTrack.VIDEO_TRACK_KIND); + + assertEquals(call.getVideoSender(), sender); + + CountDownLatch sent = new CountDownLatch(1); + CountDownLatch received = new CountDownLatch(1); + + try { + sender.setObserver(type -> { }); + call.getVideoSender().setObserver(type -> sent.countDown()); + + receiver.setObserver(type -> { }); + otherReceiver.setObserver(type -> received.countDown()); + + // The observers set through the other objects replaced the + // first ones, which leaves these objects nothing to remove. + sender.dispose(); + receiver.dispose(); + + call.awaitConnected(); + call.startMedia(); + + assertTrue(sent.await(10, TimeUnit.SECONDS), "The observer of the other sender object was removed"); + assertTrue(received.await(10, TimeUnit.SECONDS), "The observer of the other receiver object was removed"); + + call.getVideoSender().setObserver(null); + } + finally { + otherReceiver.dispose(); + } + } + } + @Test void senderIdAndStreams() { RTCPeerConnection connection = factory.createPeerConnection(new RTCConfiguration(), candidate -> { });