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

Perform safe addIceCandidate together with signaling handshake #13906

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

Conversation

soareschen
Copy link
Contributor

@soareschen soareschen commented Nov 4, 2018

Fixes #12028.

Note that not all usage of exchangeIceCandidates() is replaced. The remaining should be fixed in another PR.

@sideshowbarker
Copy link
Contributor

stability checker failing https://travis-ci.org/web-platform-tests/wpt/jobs/450421135

@soareschen soareschen force-pushed the safe-add-icecandidate branch from 1937a04 to 52b2344 Compare November 15, 2018 07:16
@wpt-pr-bot wpt-pr-bot requested a review from jan-ivar November 15, 2018 07:16
@soareschen soareschen force-pushed the safe-add-icecandidate branch from 52b2344 to c198b91 Compare November 20, 2018 07:09
@alvestrand
Copy link
Contributor

@soareschen seems we have conflicts again

@soareschen
Copy link
Contributor Author

Rebased now. Will merge if there is no more comment.

@soareschen soareschen force-pushed the safe-add-icecandidate branch from 0612de1 to 81b9916 Compare December 14, 2018 04:56
@soareschen
Copy link
Contributor Author

Not sure why but the Taskcluster check failure is preventing this PR from getting merged.

@jgraham
Copy link
Contributor

jgraham commented Dec 17, 2018

Because the Firefox and Chrome stability jobs are failing, which implies that the tests are flaky in CI. See the logs at [1] [2].

[1] https://taskcluster-artifacts.net/ccHuAqwYRCWwtTC8P9hCqA/0/public/logs/live_backing.log
[2] https://taskcluster-artifacts.net/HZ_QtUelSV-PzdbOJRmWlg/0/public/logs/live_backing.log

// 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().
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).

@foolip
Copy link
Member

foolip commented Dec 20, 2018

This PR is blocked on the required "Travis CI - Pull Request" check after #14499. There are also conflicts, so resolving those by rebasing the PR will cause the new Travis CI check to run.

@fippo
Copy link
Contributor

fippo commented Jan 19, 2024

@jan-ivar I think you fixed that a couple of years back so this is obsolete?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unsafe addIceCandidate() usage
9 participants