Closed jd20 closed 6 years ago
Did this used to work?
Yes, worked on 8.1.1.
Seems react-native didn't implement goog-active-connection. I wonder if the absence of it implies it's true, seeing as your removal of the check worked fine.
@nazar-pc do you mind taking a look at this issue?
react-native-webrtc seems to be based on Chromium's implementation just like node-webrtc/wrtc (which currently uses M57 implementation). Which version of react-native-webrtc are you using?
I was using react-native-webrtc 1.58.3, which corresponds to M58 according to their chart. Because of the getStats() issue I mentioned above though, I'm pointing to my own fork of their master branch (at github://monolithlabs/react-native-webrtc#getStats_fix).
Just wondering, would this be a reasonable fix: item.type === 'googCandidatePair' && item.googActiveConnection !== 'false'
? It works in my particular case, at least.
Could you try M57-based release, just to make sure it was a change in Chromium's code base?
I downgraded react-native-webrtc to 1.57.1 in my project, but am unable to establish a peer connection, for a totally different error, it's failing in RTCPeerConnection.createOffer(). The oldest version I've used with simple-peer was 1.58.2, so I can only speak to the M58-based releases working. Not sure how much work would be involved to get simple-peer working with M57, without digging deeper to understand why createOffer() is failing.
If you add console.log('items', items);
right after self.getStats(function (err, items) {
in simple-peer sources you can see what getStats()
returns.
In my case it always contains googActiveConnection
, initially false
and later when connection is established it changes to true
. There is no single googCandidatePair
without it in my case.
I'm not sure where webrtc implementation in react-native-webrtc comes from, but it is clearly different.
Also according to https://groups.google.com/forum/#!topic/discuss-webrtc/13BE3fbHcLU M58 should return result in spec-compliant format. Could you post on pastebin or somewhere what you're getting there exactly?
My first comment in the issue shows the output of console.log for the getStats() items. There was only one googCandidatePair item:
4: {id: "Conn-audio-1-0", type: "googCandidatePair", timestamp: 1514769244665.483, values: Array(21)}
You may very well be right that react-native-webrtc is doing something weird here, I can open up an issue on their side to investigate. Still, seems simple-peer shouldn't get stuck in an infinite loop when connecting, would the fix I proposed above make sense? (To assume googActiveConnection
is true if absent)
But what is inside Array(21)
?
Here is how my M57-based candidate looks like:
{ googActiveConnection: 'false',
bytesReceived: '156',
bytesSent: '0',
packetsSent: '0',
googReadable: 'true',
requestsSent: '0',
consentRequestsSent: '0',
responsesSent: '1',
requestsReceived: '1',
responsesReceived: '0',
googChannelId: 'Channel-data-1',
googLocalAddress: '192.168.1.2:51193',
localCandidateId: 'Cand-47/DFZH+',
googLocalCandidateType: 'local',
googRemoteAddress: '192.168.1.2:49743',
remoteCandidateId: 'Cand-lSeCBaTq',
googRemoteCandidateType: 'prflx',
googRtt: '3000',
packetsDiscardedOnSend: '0',
googTransportType: 'udp',
googWritable: 'false',
id: undefined,
type: 'googCandidatePair',
timestamp: 1515123256236.313 },
I suspect than WebRTC spec have changed a bit since relevant piece of code in simple-peer was written and potentially spec-compliant code expects input that is actually not spec-compliant 😕
Not sure if I misunderstood the console output in Chrome, but is the Array(21) represent more keys of the top-level object, or is it an array nestled under a key called values
? (I assumed it's the latter and not relevant, which is why I didn't bother grabbing it). I will take another look in the debugger when I get a chance tonight.
Grab the whole thing, preferably every time it happens through connection lifecycle, so that we can see how properties change. Spec it quite hard to read to be honest.
When updated to 8.2.0 no longer works on Safari (11.0.2 and other 11..). Works fine on Chrome, Firefox etc.. Can be quickly tested with:
const SimplePeer = require('simple-peer')
var peer1 = new SimplePeer({ initiator: true })
var peer2 = new SimplePeer()
peer1.on('signal', function (data) {
console.log(`peer1.on('signal'`, data);
peer2.signal(data)
})
peer2.on('signal', function (data) {
console.log(`peer2.on('signal'`, data);
peer1.signal(data)
})
peer1.on('connect', function () {
console.log(`peer1.on('connect'`);
peer1.send('hello world')
})
peer2.on('data', function (data) {
console.log(`peer2.on('data'`, data);
console.log('got a message from peer1: ' + data)
})
If no one has any immediate explanations to why #215 is causing these issues, we may need to consider rolling back #215 and look at a more contained change to how maybeReady works for wrtc. I think we may have been a little hasty in merging such a large change.
wrtc users can pin to this version for the meantime.
Will get the full console log I promised soon btw, haven't forgotten about this just been sidetracked with other stuff this past week.
@RationalCoding Curious if the fix I suggested above works in your case as well? i.e. change index.js#593 to item.type === 'googCandidatePair' && item.googActiveConnection !== 'false'
I'm pretty sure I'll be able to fix the issue rather quickly if someone can prepare a simple demo with react-native-webrtc
where I can experiment with the fix. I don't have macOS to test in Safari.
@nazar-pc I created this repo which replicates the problem, can you try it out? https://github.com/jd20/SimplePeerDemo/ There are instructions in the README on how to run, I'm guessing since you don't have macOS you will only be able to test Android though. I'm happy to try out any fixes you might have for iOS though.
Also, I grabbed the full contents of getStats received in findCandidatePair(), and added to the repo: https://github.com/jd20/SimplePeerDemo/blob/master/getStats.json
I just grabbed the last one, because simple-peer gets stuck in a loop I had thousands of them in my console so couldn't grab them all. If there's a meaningful number to grab from the beginning though, let me know and I'll grab it, otherwise it's easy enough to setup yourself by just adding console.log(JSON.stringify(items))
to findCandidatePair().
Hope that helps!
I see googActiveConnection
in values
array items, but not in object itself. The question is whether it is react-native-webrtc
's tweak or Chomium's sources changed something internally.
Thanks for getStats.json
, I'll investigate where it comes from.
Can someone help me with https://github.com/jd20/SimplePeerDemo/issues/1 so that I can work on actually fixing this?
Just replied on that issue, sorry I haven't been receiving email notifications on my own repositories.
Still didn't succeed in running demo yet, but from values
property it seems that the issue lies in serializing RTCStatsReport
. Not sure what happens on macOS in Safari, I don't have access to any Mac.
I have a Mac. I don't have much time this week, but I can run some test cases if you send them.
Here is a simple test that should print some useful information about what stats info looks like in form of JSON string:
var peer1 = new RTCPeerConnection({iceServers : []});
var peer2 = new RTCPeerConnection({iceServers : []});
var channel1 = peer1.createDataChannel('data');
var channel2 = peer2.createDataChannel('data');
var waiting_for = 2;
function ready () {
--waiting_for;
if (!waiting_for) {
peer1.getStats().then(function (stats) {
var stats_data = Array.from(stats.values())
console.log(JSON.stringify([stats.toString(), stats_data]));
})
}
}
channel1.onopen = ready;
channel2.onopen = ready;
peer1.createOffer()
.then(function (offer) {
return peer1.setLocalDescription(offer);
})
.then(function () {
peer2.setRemoteDescription(peer1.localDescription);
return peer2.createAnswer();
})
.then(function (answer) {
return peer2.setLocalDescription(answer);
})
.then(function () {
peer1.setRemoteDescription(peer2.localDescription);
});
If it doesn't work, you may want to add ice server {urls: 'stun:stun.l.google.com:19302'}
.
In latest Chromium nightly result is fully spec-compliant, I'm curious what Safari have implemented:
So Safari demands a STUN server, else ICE will be stuck on "gathering" forever. Even worse than that, ICE always fails unless the user goes into Develop > WebRTC > Disable ICE Candidate Restrictions
and restarts Safari... absolutely ridiculous.
Finally, here's the entire stats object from Safari 11.0.3 on OSX 10.13.33.
Code to reproduce (after changing the setting described above):
Thanks for those details!
So Safari implementation tries to follow the spec, but for now seems to be extremely basic and lacks a lot of features.
There are no local-candidate
, no remote-candidate
, no transport
to identify local/remote IP and port used in connection.
This is very sad, but definitely should not prevent simple-peer from working there at all.
Will submit PR with fix for this during next few hours.
@vblagomir, can you test #228? It should resolve an issue for Safari, where there is a candidate pair, but no local/remote candidates.
Yep, will try to check either today or tomorrow evening.
I've managed to run plain WebRTC without simple-peer with react-native-webrtc.
In short, they have Promise-based getStats()
, but instead of returning Map-like structure according to the spec, they're returning this weird array of objects. It would work otherwise, as simple-peer is only using forEach
method, but they have part of the properties in that object directly and part of them as items in nested values
array, which doesn't make any sense to me, but this is how they did it.
I'll try to patch this in react-native-webrtc and submit PR upstream, but this is really not simple-peer's fault.
https://github.com/oney/react-native-webrtc/pull/415 should resolve this issue fully. @jd20, can you test it?
@nazar-pc Tested with your patch, both iOS and Android are connecting successfully! Looks good 👍
Can not test for now because of workload, will be able to come back to this only within a month.
PR is simple enough, hopefully @RationalCoding will have time for review soon.
react-native-webrtc
developers are aware of this issue and seems to agree to fix this upstream (they are currently implementing 2014-ish version of the spec, but that should change at some point).
https://github.com/nazar-pc/react-native-webrtc/tree/patch-1 branch can be used as workaround for now, but there is nothing that should be done in simple-peer and I think this issue can be closed.
Thanks for all the help debugging this issue, @nazar-pc. I just added you as a collaborator to this repo ✨(I hope you don't mind.)
Closing this issue now.
I've been using simple-peer to establish WebRTC connections between node and a react-native app. After upgrading from 8.1.1 to 8.2.0, I'm seeing simple-peer hang on the react-native side after establishing the connection (it's stuck in connecting state, calling getStats() over and over non-stop). This means it never reaches the connected state, even thought the other side thinks it's connected.
Going through the recent commits, the breaking change was #215. From what I can tell, the cause is this: 1) Prior to #215, once we call Peer._maybeReady(), we are considered connected. After #215, we need a candidate pair that satisfies certain conditions, to be considered connected. 2) react-native-webrtc doesn't seem to return a proper candidate pair from getStats(). First, it's not even formatted correctly, it's just a giant string blob (see https://github.com/oney/react-native-webrtc/issues/396). Second, even if it's in the right format, it has a
googCandidatePair
item, but no googActiveConnection flag. This causes simple-peer to keep searching for a connected candidate pair, which will never come, hence the infinite loop.For reference, these are the item pairs I'm getting back from react-native-webrtc's RTCPeerConnection:
Just as a workaround, if I remove the check for googActiveConnection, the connection no longer gets stuck. Unfortunately, I don't understand the WebRTC spec well enough to know what's at fault here: does react-native-webrtc need to fix the output of getStats(), or should simple-peer be more robust to handle this kind of case?