Fix JNI memory-safety and thread-safety bugs in the native bridge - #283
Merged
Conversation
RTCPeerConnection.close() called pc->Close() and cleared the Java handle, but never called pc->Release() on the raw pointer that PeerConnectionFactory::createPeerConnection() had handed to Java via pc.release(). Every closed connection leaked the native PeerConnectionInterface and everything it still owned.
registerObserver() on RTCDataChannel, RTCDtlsTransport, and RTCDtmfSender heap-allocated a native observer and handed it to WebRTC, but never kept a reference to free it: every call leaked the observer plus the JNI global reference it held to the Java listener, and replacing an observer leaked the previous one outright. Track the native observer the same way RTCPeerConnection already tracks its own via an "observerHandle" field, and free it whenever it's replaced, explicitly unregistered, or (for RTCDataChannel) the channel is disposed.
AttachCurrentThread() can return nullptr when attaching the calling native thread to the JVM fails (e.g. during JVM shutdown), a case already handled in LogSink and RTCDataChannelSendObserver. Every other native callback/observer dereferenced the JNIEnv unconditionally, crashing the process instead of just dropping the callback.
VideoTrackSink::OnFrame and DesktopCaptureCallback::OnCaptureResult manually called env->DeleteLocalRef() on jBuffer, a JavaLocalRef that already deletes its own local reference on scope exit. JavaLocalRef's destructor doesn't know the reference was already deleted, so it deletes it again -- undefined behavior per the JNI spec, hit on every video frame and desktop capture frame. Also guard both callbacks against a null JNIEnv after a failed thread attach, matching the rest of the native callback classes.
- RTCPeerConnection: repeated create/close does not crash, guarding the pc->Release() fix on close(). - RTCDataChannel and RTCDtlsTransport: replacing a registered observer stops the previous one from receiving further events, and disposing a channel with an observer still registered does not throw. - RTCDtmfSender: replacing a registered observer stops the previous one from receiving further events. None of these previously had coverage for the observer-replace path, which is what silently leaked the old native observer wrapper.
dispose() only zeroed the native handle when Release() reported that it dropped the last reference; if another reference happened to be alive elsewhere, the handle was left pointing at an object this Java wrapper no longer owns. A retried dispose() (or any other call) would then Release() a reference count unit that isn't this wrapper's to release, corrupting whatever else still legitimately holds a reference to the track.
…ansceivers RTCPeerConnection.getSenders()/getReceivers()/getTransceivers(), RTCRtpTransceiver.getSender()/getReceiver(), and the OnTrack/ OnAddTrack/OnRemoveTrack observer callbacks all constructed their Java wrapper from a raw pointer (scoped_refptr::get()) without ever keeping the reference that pointer came from. The temporary scoped_refptr's destructor released that reference at the end of the same statement, so the Java wrapper ended up backed by no owned reference at all -- relying entirely on WebRTC's internal transceiver list to keep the object alive. That list entry is not guaranteed to survive: WebRTC actually erases a stopped transceiver from PeerConnection's own list during a later SetLocalDescription/SetRemoteDescription that recycles its m= section, or during rollback of a not-yet-negotiated transceiver (confirmed against M152's pc/sdp_offer_answer.cc). A Java RTCRtpSender/ RTCRtpReceiver/RTCRtpTransceiver obtained before that point can end up holding a dangling native pointer. Transfer the reference properly instead (scoped_refptr::release(), or a new createOwningObjectArray() for the array-returning queries), and give RTCRtpSender/RTCRtpReceiver/RTCRtpTransceiver a dispose() to release it -- these types are not exclusively owned the way e.g. MediaStreamTrack is (the owning transceiver/connection, and any other independently queried wrapper, keep their own reference), so unlike other dispose() implementations in this codebase, dropping our reference here is normal and is not reported as an error.
…sses RTCPeerConnection, RTCDtlsTransport, and RTCDtmfSender inherited Object's reference-equality equals()/hashCode(), so two Java wrappers bound to the same native object (e.g. an RTCDtlsTransport queried via two different RTCRtpSenders sharing a bundled transport) compared as unequal -- surprising in Set/Map usage. Compare by native handle instead, via a new protected accessor on NativeObject; a disposed instance (handle 0) falls back to identity so that unrelated disposed objects don't all compare equal to each other. (RTCRtpSender/RTCRtpReceiver/RTCRtpTransceiver already got the same treatment in the previous commit, since they needed touching anyway for their reference-ownership fix.)
The javadoc promised IllegalStateException after disposal, but the native implementation's CHECK_HANDLE guard -- the same one every other disposable class in this codebase relies on -- throws NullPointerException. Document the actual, already-consistent behavior instead of a type nothing throws.
RTCRtpSender, RTCRtpReceiver, and RTCRtpTransceiver are now disposable (previous commit), so the examples need to actually dispose the instances they obtain from addTrack()/addTransceiver() and the onAddTrack/onRemoveTrack/onTrack observer callbacks, instead of leaking them, to demonstrate correct API usage. PeerConnectionManager gained a senders list, disposed in close(), since its addTrack() wrapper previously discarded the returned RTCRtpSender.
RTCDataChannel, RTCDtlsTransport, RTCDtmfSender, and RTCPeerConnection each hand-rolled the same get-old/delete/store-new sequence for freeing a heap-allocated native observer on replace, unregister, or dispose -- the exact pattern a missing step in silently leaked before. Centralize it in jni-voithos so the next JNI wrapper that registers an observer against a "replace the previous one" API gets this for free instead of reimplementing (and potentially forgetting a step of) the same logic.
toJava() wrapped the same raw jobject in two separate JavaLocalRef instances: one temporary handed to the JavaObject constructor, and another to build the return value. The first temporary's destructor deleted the local ref at the end of its full expression, so the second one -- and everything the caller (RTCConfiguration::toJava) did with it afterward, including RTCPeerConnection.getConfiguration() -- used an already-deleted reference. Found via -Xcheck:jni, which reported "FATAL ERROR in native method: Bad global or local ref passed to JNI" and aborted the JVM; not something a normal test run would necessarily catch, since a freed local ref doesn't always misbehave immediately. Keep one named JavaLocalRef alive for the whole function instead.
mvn -pl webrtc test -Pjni-check reruns the test suite under
-Xcheck:jni, which validates every JNI call's local/global refs and
aborts with "FATAL ERROR in native method: ..." on a real violation --
the exact bug class this branch has fixed several times over (a
double-freed local ref, an already-deleted one, a dangling reference
relied on without an owned ref). It just caught another one
(PortAllocatorConfig, previous commit).
Not wired into the default test run or CI: -Xcheck:jni also emits a
"WARNING in native method: JNI call made without checking exceptions"
for every native call in this codebase that doesn't immediately check
for a pending exception -- thousands of them, pre-existing, harmless
on their own, but far too noisy to gate a build on before someone
works through fixing them.
The profile also switches Surefire's fork communication to TCP
sockets (forkNode=SurefireForkNodeFactory), which is required:
-Xcheck:jni writes its diagnostics straight to the process's native
stdout, the same stream Surefire's default pipe-based protocol uses,
corrupting it ("Corrupted channel by directly writing to native
stream") instead of reporting results.
RTCRtpSender, RTCRtpReceiver, and RTCRtpTransceiver are now disposable. Update the guide code samples that obtain one (via addTrack()/addTransceiver() or the onTrack observer callback) to dispose it, matching the examples, and correct dtmf-sender.md's now-inaccurate claim that the RTCDtmfSender's RTCRtpSender never needs explicit disposal.
Editing this file with tooling that writes LF, combined with this checkout's core.autocrlf=true normalizing on the next `git add` regardless of the working tree's actual bytes, silently flipped the whole file from this repo's original CRLF to LF across earlier commits on this branch -- turning a handful of real line changes into a 1179-line diff. Re-add with autocrlf bypassed for just this file so the diff against main reflects only the actual changes.
This was referenced Sep 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A set of native (JNI) memory-safety and thread-safety fixes found via a targeted audit of the JNI bridge, plus the tooling to catch this class of bug going forward.
Reference-counting / lifetime fixes
RTCPeerConnection.close()never released the owning native reference taken at creation — leaked the nativePeerConnectionInterfaceon every close.RTCDataChannel,RTCDtlsTransport,RTCDtmfSenderleaked their native observer wrapper (and its JNI global ref to the Java listener) on everyregisterObserver()call, and doubly so on replace.RTCPeerConnection.getSenders()/getReceivers()/getTransceivers(),RTCRtpTransceiver.getSender()/getReceiver(), and theOnTrack/OnAddTrack/OnRemoveTrackobserver callbacks handed out Java wrappers backed by no owned reference at all — confirmed (against the actual M152 WebRTC source) as a real dangling-pointer risk after a stopped transceiver's m-line is recycled on renegotiation, or on rollback.RTCRtpSender/RTCRtpReceiver/RTCRtpTransceiverare now disposable so callers can release the reference they're handed.MediaStreamTrack.dispose()left the native handle valid (reusable) on its error path, risking a double-release.PortAllocatorConfig::toJavaused an already-deleted local ref, causingRTCPeerConnection.getConfiguration()to abort the JVM under-Xcheck:jni.Thread-safety fixes
JNIEnvafterAttachCurrentThread()(it can fail, e.g. during JVM shutdown) instead of dereferencing unconditionally.DeleteLocalRefinVideoTrackSink::OnFrameandDesktopCaptureCallback::OnCaptureResult— undefined behavior per the JNI spec, hit on every video/capture frame.Consistency / API
ReplaceNativeObserver/ClearNativeObserverhelpers so the observer-lifetime pattern isn't hand-rolled (and potentially half-implemented) per JNI wrapper class.equals()/hashCode()to the native handle wrapper classes.VideoCapture's javadoc to the exception type it actually throws.Tooling
mvn -pl webrtc test -Pjni-checkprofile that reruns the test suite under-Xcheck:jni(with Surefire's fork communication switched to TCP sockets, since-Xcheck:jni's native stdout writes otherwise corrupt Surefire's default pipe-based protocol). It caught thePortAllocatorConfigbug above.Examples and docs updated to match the new disposal requirements on
RTCRtpSender/RTCRtpReceiver/RTCRtpTransceiver.Test plan
mvn verify(native build + fullwebrtctest suite, 145 tests) passes.mvn -pl webrtc test -Pjni-checkpasses with zeroFATAL ERROR(previously caught thePortAllocatorConfigandRTCPeerConnection.getConfiguration()bug).RTCPeerConnectionTests,RTCDataChannelTests,RTCDtmfSenderTests,RTCRtpTransceiverTests, newRTCDtlsTransportTests).webrtc-examplescompiles against the updated API.