fix: take Map method IDs from the interface instead of from HashMap - #290
Merged
Merged
Conversation
jni::JavaHashMap resolved put() and entrySet() against java.util.HashMap and
then called them on whatever map it held. One of its constructors takes any
java.util.Map, and PeerConnectionFactory hands it the field trials map the
application passed in, which is rarely a HashMap. Collections.singletonMap,
Collections.emptyMap, Map.of and TreeMap are all other classes.
Using a method ID from one class on an object of another is undefined. It
happens to work on HotSpot, so the tests pass, but the JVM aborts under
-Xcheck:jni:
FATAL ERROR in native method: Wrong object class or methodID passed to JNI call
at dev.onvoid.webrtc.PeerConnectionFactory.initialize(Native Method)
It aborted partway through the suite, which left
"mvn -pl webrtc test -Pjni-check" unusable as a gate on any branch.
Both methods now come from the java.util.Map interface, where virtual dispatch
handles whichever implementation arrives, the way JavaMapIterator already
resolves Set, Iterator and Map.Entry. The class keeps its HashMap reference
for the constructor that creates one. RTCRtpCodecCapability,
RTCRtpCodecParameters and RTCStats wrap caller-supplied maps through the same
helper and are fixed along with it.
The new test builds the factory from several map implementations, none of them
a HashMap. It passes either way on its own and only fails the -Pjni-check
profile, which is where this class of bug is visible.
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
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.
The bug
jni::JavaHashMapin the vendoredjni-voithoshelper resolvedput()andentrySet()againstjava.util.HashMapand then called them on whatever map it held. One of its constructors takes anyjava.util.Map, andPeerConnectionFactoryhands it the field trials map the application passed in, which is rarely aHashMap:Collections.singletonMap,Collections.emptyMap,Map.ofandTreeMapare all other classes.RTCRtpCodecCapabilityandRTCRtpCodecParameterswrap caller-supplied maps through the same helper.Using a method ID from one class on an object of another is undefined. HotSpot tolerates it in a normal run, so the tests pass, but the JVM aborts under
-Xcheck:jni:It aborted partway through
PeerConnectionFactoryTests, which leftmvn -pl webrtc test -Pjni-checkunusable as a gate on any branch.The fix
Both method IDs now come from the
java.util.Mapinterface, where dispatch handles whichever implementation arrives. This is the wayJavaMapIteratoralready resolvesSet,IteratorandMap.Entry. The constructor ID stays onHashMap, since it is only used to create one.Testing
The new
createWithFieldTrialsOfAnyMapTypebuilds the factory from a singleton map, an empty map, aTreeMap, aConcurrentHashMapand an unmodifiable wrapper. None of them extendsHashMap, andLinkedHashMapis deliberately left out because it does. The test passes either way on its own and only fails under-Pjni-check, which is where this class of bug is visible.The full suite runs green under
-Pjni-checkon this branch: 151 tests, no fatal JNI errors. Before the fix the same run aborted increateWithInvalidFieldTrials.This branch does not overlap with #289 and merges cleanly with it in either order.