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.

Sign up for GitHub

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

Jump to bottom

Perform safe addIceCandidate together with signaling handshake #13906

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Perform safe addIceCandidate together with signaling handshake #13906

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions webrtc/RTCDTMFSender-helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

// The following helper functions are called from RTCPeerConnection-helper.js:
// getTrackFromUserMedia
// doSignalingHandshake
// doSignalingAndCandidateHandshake

// Create a RTCDTMFSender using getUserMedia()
// Connect the PeerConnection to another PC and wait until it is
Expand All @@ -25,8 +25,7 @@ function createDtmfSender(pc = new RTCPeerConnection()) {
// on whether sending should be possible before negotiation.
const pc2 = new RTCPeerConnection();
Object.defineProperty(pc, 'otherPc', { value: pc2 });
exchangeIceCandidates(pc, pc2);
return doSignalingHandshake(pc, pc2);
return doSignalingAndCandidateHandshake(pc, pc2);
}).then(() => {
if (!('canInsertDTMF' in dtmfSender)) {
return Promise.resolve();
Expand Down
6 changes: 2 additions & 4 deletions webrtc/RTCDtlsTransport-getRemoteCertificates.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@
'use strict';

// The following helper functions are called from RTCPeerConnection-helper.js:
// exchangeIceCandidates
// doSignalingHandshake
// doSignalingAndCandidateHandshake

/*
5.5. RTCDtlsTransport Interface
Expand Down Expand Up @@ -43,9 +42,8 @@
t.add_cleanup(() => pc2.close());

pc1.createDataChannel('test');
exchangeIceCandidates(pc1, pc2);

doSignalingHandshake(pc1, pc2)
doSignalingAndCandidateHandshake(pc1, pc2)
.then(t.step_func(() => {
// pc.sctp is set when set*Description(answer) is called
const sctpTransport1 = pc1.sctp;
Expand Down
11 changes: 4 additions & 7 deletions webrtc/RTCPeerConnection-connectionState.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
// https://w3c.github.io/webrtc-pc/archives/20170605/webrtc.htm

// The following helper functions are called from RTCPeerConnection-helper.js:
// exchangeIceCandidates
// doSignalingHandshake
// doSignalingAndCandidateHandshake

/*
4.3.2. Interface Definition
Expand Down Expand Up @@ -133,8 +132,7 @@

pc1.addEventListener('connectionstatechange', onConnectionStateChange);

exchangeIceCandidates(pc1, pc2);
doSignalingHandshake(pc1, pc2);
doSignalingAndCandidateHandshake(pc1, pc2);
}, 'connection with one data channel should eventually have connected connection state');

async_test(t => {
Expand Down Expand Up @@ -165,9 +163,8 @@

pc1.addEventListener('connectionstatechange', onConnectionStateChange);

exchangeIceCandidates(pc1, pc2);
doSignalingHandshake(pc1, pc2);
}, 'connection with one data channel should eventually have transports in connected state');
doSignalingAndCandidateHandshake(pc1, pc2);
}, 'connection with one data channel should eventually have connected connection state');

/*
TODO
Expand Down
7 changes: 2 additions & 5 deletions webrtc/RTCPeerConnection-getStats.https.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@
// assert_stats_report_has_stats

// The following helper function is called from RTCPeerConnection-helper.js
// exchangeIceCandidates
// doSignalingHandshake
// doSignalingAndCandidateHandshake

/*
8.2. getStats
Expand Down Expand Up @@ -331,9 +330,7 @@
}
}));


exchangeIceCandidates(pc1, pc2);
doSignalingHandshake(pc1, pc2);
return doSignalingAndCandidateHandshake(pc1, pc2);
}))
.catch(t.step_func(err => {
assert_unreached(`test failed with error: ${err}`);
Expand Down
45 changes: 25 additions & 20 deletions webrtc/RTCPeerConnection-helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,19 +159,17 @@ function test_never_resolve(testFunc, testName) {
}, testName);
}

// Helper function to exchange ice candidates between
// two local peer connections
function exchangeIceCandidates(pc1, pc2) {
// Helper function to exchange ice candidates between two local peer connections.
// Accepts an optional handshakePromise to wait for setRemoteDescription() to be
// done before calling addIceCandidate().
alvestrand marked this conversation as resolved.
Show resolved Hide resolved
function exchangeIceCandidates(pc1, pc2, handshakePromise) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should land this. This is queuing ICE candidates again, like in #14417 (comment).

It also doesn't appear to fix the problem.

As long as people call exchangeIceCandidates(pc1, pc2) before doSignalingHandshake(pc1, pc2) there should be no race. Let's discuss in #12028 (comment).

// private function
function doExchange(localPc, remotePc) {
localPc.addEventListener('icecandidate', event => {
localPc.addEventListener('icecandidate', async event => {
const { candidate } = event;

// candidate may be null to indicate end of candidate gathering.
// There is ongoing discussion on w3c/webrtc-pc#1213
// that there should be an empty candidate string event
// for end of candidate for each m= section.
if(candidate && remotePc.signalingState !== 'closed') {
await handshakePromise
remotePc.addIceCandidate(candidate);
}
});
Expand All @@ -183,15 +181,23 @@ function exchangeIceCandidates(pc1, pc2) {

// Helper function for doing one round of offer/answer exchange
// betweeen two local peer connections
function doSignalingHandshake(localPc, remotePc) {
return localPc.createOffer()
.then(offer => Promise.all([
localPc.setLocalDescription(offer),
remotePc.setRemoteDescription(offer)]))
.then(() => remotePc.createAnswer())
.then(answer => Promise.all([
remotePc.setLocalDescription(answer),
localPc.setRemoteDescription(answer)]))
async function doSignalingHandshake(localPc, remotePc) {
const offer = await localPc.createOffer();
await localPc.setLocalDescription(offer);
await remotePc.setRemoteDescription(offer);

const answer = await remotePc.createAnswer();
await remotePc.setLocalDescription(answer);
await localPc.setRemoteDescription(answer);
}

// Helper function to do both signaling handshake and ICE candidates
// exchange. The two tasks are done in parallel in a safe way, with
// care to make sure addIceCandidate() called after setRemoteDescription().
async function doSignalingAndCandidateHandshake(pc1, pc2) {
const handshakePromise = doSignalingHandshake(pc1, pc2)
exchangeIceCandidates(pc1, pc2, handshakePromise)
await handshakePromise
}

// Helper function to create a pair of connected data channel.
Expand All @@ -205,8 +211,6 @@ function createDataChannelPair(
{
const channel1 = pc1.createDataChannel('');

exchangeIceCandidates(pc1, pc2);

return new Promise((resolve, reject) => {
let channel2;
let opened1 = false;
Expand Down Expand Up @@ -245,7 +249,8 @@ function createDataChannelPair(

pc2.addEventListener('datachannel', onDataChannel);

doSignalingHandshake(pc1, pc2);
doSignalingAndCandidateHandshake(pc1, pc2)
.catch(reject);
});
}

Expand Down
9 changes: 4 additions & 5 deletions webrtc/RTCPeerConnection-iceConnectionState.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@
// https://w3c.github.io/webrtc-pc/archives/20170605/webrtc.html

// The following helper functions are called from RTCPeerConnection-helper.js:
// exchangeIceCandidates
// doSignalingHandshake
// doSignalingAndCandidateHandshake

/*
4.3.2. Interface Definition
Expand Down Expand Up @@ -105,8 +104,8 @@
async_test(t => {
const pc1 = new RTCPeerConnection();
t.add_cleanup(() => pc1.close());
const pc2 = new RTCPeerConnection();

const pc2 = new RTCPeerConnection();
t.add_cleanup(() => pc2.close());

const onIceConnectionStateChange = t.step_func(() => {
Expand Down Expand Up @@ -139,8 +138,8 @@

pc1.addEventListener('iceconnectionstatechange', onIceConnectionStateChange);

exchangeIceCandidates(pc1, pc2);
doSignalingHandshake(pc1, pc2);
doSignalingAndCandidateHandshake(pc1, pc2)
.catch(t.unreached_func('handshake error'));
}, 'connection with one data channel should eventually have connected connection state');

/*
Expand Down
8 changes: 4 additions & 4 deletions webrtc/RTCPeerConnection-iceGatheringState.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
// https://w3c.github.io/webrtc-pc/archives/20170605/webrtc.html

// The following helper functions are called from RTCPeerConnection-helper.js:
// doSignalingHandshake
// exchangeIceCandidates

// doSignalingAndCandidateHandshake
// generateAudioReceiveOnlyOffer

/*
Expand Down Expand Up @@ -134,8 +134,8 @@
// when icegatheringstatechange event is fired.
pc2.addEventListener('icegatheringstatechange', onIceGatheringStateChange);

exchangeIceCandidates(pc1, pc2);
doSignalingHandshake(pc1, pc2);
doSignalingAndCandidateHandshake(pc1, pc2)
.catch(t.unreached_func('handshake error'));
}, 'connection with one data channel should eventually have connected connection state');

/*
Expand Down
18 changes: 10 additions & 8 deletions webrtc/RTCPeerConnection-ondatachannel.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@
// https://w3c.github.io/webrtc-pc/archives/20170605/webrtc.html

// The following helper functions are called from RTCPeerConnection-helper.js:
// exchangeIceCandidates
// doSignalingHandshake
// doSignalingAndCandidateHandshake

/*
6.2. RTCDataChannel
Expand Down Expand Up @@ -70,8 +69,9 @@
localPc.createDataChannel('test');

remotePc.addEventListener('datachannel', onDataChannel);
exchangeIceCandidates(localPc, remotePc);
doSignalingHandshake(localPc, remotePc);

doSignalingAndCandidateHandshake(localPc, remotePc)
.catch(t.unreached_func('handshake error'));
}, 'datachannel event should fire when new data channel is announced to the remote peer');

/*
Expand Down Expand Up @@ -134,8 +134,9 @@
assert_equals(localChannel.priority, 'high');

remotePc.addEventListener('datachannel', onDataChannel);
exchangeIceCandidates(localPc, remotePc);
doSignalingHandshake(localPc, remotePc);

doSignalingAndCandidateHandshake(localPc, remotePc)
.catch(t.unreached_func('handshake error'));
}, 'Data channel created on remote peer should match the same configuration as local peer');

/*
Expand All @@ -162,8 +163,9 @@
});

remotePc.addEventListener('datachannel', onDataChannel);
exchangeIceCandidates(localPc, remotePc);
doSignalingHandshake(localPc, remotePc);

doSignalingAndCandidateHandshake(localPc, remotePc)
.catch(t.unreached_func('handshake error'));

t.step_timeout(t.step_func_done(), 200);
}, 'Data channel created with negotiated set to true should not fire datachannel event on remote peer');
Expand Down
12 changes: 4 additions & 8 deletions webrtc/RTCPeerConnection-track-stats.https.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,8 +402,7 @@
let [tracks, streams] = await getUserMediaTracksAndStreams(2);
let sender = caller.addTrack(tracks[0], streams[0]);
callee.addTrack(tracks[1], streams[1]);
exchangeIceCandidates(caller, callee);
await doSignalingHandshake(caller, callee);
await doSignalingAndCandidateHandshake(caller, callee);
await onIceConnectionStateCompleted(caller);
let receiver = caller.getReceivers()[0];

Expand Down Expand Up @@ -450,8 +449,7 @@
let [tracks, streams] = await getUserMediaTracksAndStreams(2);
let sender = caller.addTrack(tracks[0], streams[0]);
callee.addTrack(tracks[1], streams[1]);
exchangeIceCandidates(caller, callee);
await doSignalingHandshake(caller, callee);
await doSignalingAndCandidateHandshake(caller, callee);
await onIceConnectionStateCompleted(caller);
let receiver = caller.getReceivers()[0];

Expand Down Expand Up @@ -498,8 +496,7 @@
let [tracks, streams] = await getUserMediaTracksAndStreams(2);
let sender = caller.addTrack(tracks[0], streams[0]);
callee.addTrack(tracks[1], streams[1]);
exchangeIceCandidates(caller, callee);
await doSignalingHandshake(caller, callee);
await doSignalingAndCandidateHandshake(caller, callee);
await onIceConnectionStateCompleted(caller);

// Wait until RTCP has arrived so that it can not arrive between
Expand Down Expand Up @@ -529,8 +526,7 @@
let [tracks, streams] = await getUserMediaTracksAndStreams(2);
let sender = caller.addTrack(tracks[0], streams[0]);
callee.addTrack(tracks[1], streams[1]);
exchangeIceCandidates(caller, callee);
await doSignalingHandshake(caller, callee);
await doSignalingAndCandidateHandshake(caller, callee);
await onIceConnectionStateCompleted(caller);
let receiver = caller.getReceivers()[0];

Expand Down
6 changes: 2 additions & 4 deletions webrtc/RTCRtpReceiver-getContributingSources.https.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@

// The following helper function is called from RTCPeerConnection-helper.js
// getTrackFromUserMedia
// exchangeIceCandidates
// doSignalingHandshake
// doSignalingAndCandidateHandshake

/*
5.3. RTCRtpReceiver Interface
Expand Down Expand Up @@ -58,8 +57,7 @@
return getTrackFromUserMedia('audio')
.then(([track, mediaStream]) => {
pc1.addTrack(track, mediaStream);
exchangeIceCandidates(pc1, pc2);
return doSignalingHandshake(pc1, pc2);
return doSignalingAndCandidateHandshake(pc1, pc2);
})
.then(() => ontrackPromise)
.then(receiver => {
Expand Down
3 changes: 1 addition & 2 deletions webrtc/RTCRtpReceiver-getSynchronizationSources.https.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,7 @@
return getTrackFromUserMedia('audio')
.then(([track, mediaStream]) => {
pc1.addTrack(track, mediaStream);
exchangeIceCandidates(pc1, pc2);
return doSignalingHandshake(pc1, pc2);
return doSignalingAndCandidateHandshake(pc1, pc2);
})
.then(() => ontrackPromise)
.then(receiver => {
Expand Down