Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebRTCModule.peerConnectionInit returns false when using TURN server with iceServers key "urls" #1476

Closed
xaleel opened this issue Nov 11, 2023 · 7 comments · Fixed by #1477
Closed
Labels
bug Something isn't working as expected confirmed The issue has been confirmed needs-research Research might be needed

Comments

@xaleel
Copy link

xaleel commented Nov 11, 2023

I spent many days trying to debug this issue, now I feel silly it's this simple, but I don't believe this is the expected behavior.

Expected Behavior:

peerConnection initializes

Observed Behavior:

App crashes with error message Failed to initialize PeerConnection, check the native logs!.

Steps to reproduce the issue:

const peerConstraints = {
	iceServers: [
		{
			url: "stun:stun.l.google.com:19302",
			// urls: "stun:stun.l.google.com:19302",
			// both work fine
		},
		{
			urls: "TURN:freeturn.net:3478", // this fails
			// urls: ["TURN:freeturn.net:3478"], // this also fails
			// url: "TURN:freeturn.net:3478", // (without the 's') this works
			// url: ["TURN:freeturn.net:3478"], // this also works
			username: "free",
			credential: "free",
		},
	],
	iceTransportPolicy: "all",
	rtcpMuxPolicy: "negotiate"
};
export default function CameraComponent(props: any) {
	const [peerConnection, setPeerConnection] = useState(new RTCPeerConnection(peerConstraints));
	...
}

According to w3 docs (MDN) the RTCIceServer Dictionary's key should be "urls". Using "url" should still be supported, in my opinion.

Platform Information

  • React Native Version: 0.72.6
  • WebRTC Module Version: ^111.0.6
  • Platform OS + Version: Android 9, API 28, Samsung SM-G950F, expo ~49.0.18
@xaleel
Copy link
Author

xaleel commented Nov 11, 2023

Additional info:
With the configuration that fails, I added debug lines to node_modules\react-native-webrtc\src\RTCPeerConnection.ts

export default class RTCPeerConnection extends EventTarget<RTCPeerConnectionEventMap> {
   ...
    constructor(configuration) {
        super();

        this._pcId = nextPeerConnectionId++;

        console.log("this._pcId", this._pcId);
        let peerCon = WebRTCModule.peerConnectionInit(configuration, this._pcId);
        console.log("peerCon", peerCon);

        if (!peerCon) {
            throw new Error('Failed to initialize PeerConnection, check the native logs!');
        }
       ...
    }
...
}

Console output:
image

@saghul
Copy link
Member

saghul commented Nov 11, 2023

Thanks for the report!

@saghul
Copy link
Member

saghul commented Nov 11, 2023

What error do you see in logcat?

This is the code for parsing ICE servers (WebRTCModule.java):

    private List<PeerConnection.IceServer> createIceServers(ReadableArray iceServersArray) {
        final int size = (iceServersArray == null) ? 0 : iceServersArray.size();
        List<PeerConnection.IceServer> iceServers = new ArrayList<>(size);
        for (int i = 0; i < size; i++) {
            ReadableMap iceServerMap = iceServersArray.getMap(i);
            boolean hasUsernameAndCredential = iceServerMap.hasKey("username") && iceServerMap.hasKey("credential");
            if (iceServerMap.hasKey("urls")) {
                switch (iceServerMap.getType("urls")) {
                    case String:
                        if (hasUsernameAndCredential) {
                            iceServers.add(createIceServer(iceServerMap.getString("urls"),
                                    iceServerMap.getString("username"),
                                    iceServerMap.getString("credential")));
                        } else {
                            iceServers.add(createIceServer(iceServerMap.getString("urls")));
                        }
                        break;
                    case Array:
                        ReadableArray urls = iceServerMap.getArray("urls");
                        for (int j = 0; j < urls.size(); j++) {
                            String url = urls.getString(j);
                            if (hasUsernameAndCredential) {
                                iceServers.add(createIceServer(
                                        url, iceServerMap.getString("username"), iceServerMap.getString("credential")));
                            } else {
                                iceServers.add(createIceServer(url));
                            }
                        }
                        break;
                }
            }
        }
        return iceServers;
    }

We don't check for url, just urls. So my guess is that the url entries are getting ignored. I wonder if the protocol might be checked in a case sensitive manner? Can you try urls: "turn:freeturn.net:3478", as a TURN server?

@xaleel
Copy link
Author

xaleel commented Nov 11, 2023

There was nothing useful in Logcat. You're right, it was an issue with the case.
urls: "turn:freeturn.net:3478" doesn't cause the exception. I didn't even consider it.
Thank you!

@xaleel xaleel closed this as completed Nov 11, 2023
@saghul saghul reopened this Nov 11, 2023
@saghul
Copy link
Member

saghul commented Nov 11, 2023

Im reopening so we can fix this.

@8BallBomBom 8BallBomBom added bug Something isn't working as expected confirmed The issue has been confirmed needs-research Research might be needed labels Nov 11, 2023
@8BallBomBom
Copy link
Member

Looks like it is also an iOS issue, parsing code here.
But also someone complaining of the same error over here using iOS.

Then again as it is deprecated i'm not surprised 🤔
What does surprise me is how a simple thing causes such an issue.
Definitely something we can fix though 😸

saghul added a commit that referenced this issue Nov 12, 2023
- Make sure the `urls` property is used, while supporting the deprecated
  `url`.
- Make sure all servers are lower-case since the C++ layer does a case
  insensitive parse.

Fixes: #1476
@saghul
Copy link
Member

saghul commented Nov 12, 2023

Fix here: #1477

I chose to sanitize them on the JS side since that way we don't need to fix it twice on the native side.

saghul added a commit that referenced this issue Nov 13, 2023
- Make sure the `urls` property is used, while supporting the deprecated
  `url`.
- Make sure all servers are lower-case since the C++ layer does a case
  insensitive parse.

Fixes: #1476
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected confirmed The issue has been confirmed needs-research Research might be needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants