Closed Bug 1253706 Opened 9 years ago Closed 3 years ago

Implement RTCPeerConnection.setConfiguration

Categories

(Core :: WebRTC: Signaling, enhancement, P2)

Product:
Core
Component:
WebRTC: Signaling
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
99 Branch
Tracking Flags:
Tracking Status
firefox47 --- wontfix
firefox99 --- fixed

People

(Reporter: jib, Assigned: bwc)

References

(Blocks 4 open bugs,
URL
)

Details

(Keywords: dev-doc-complete, parity-chrome, parity-safari)

Attachments

(16 files, 1 obsolete file)

Bug 1253706: Web-platform-tests for bug. r?jib
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Mochitests for setConfiguration. r?jib
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Webidl/JS for RTCPeerConnection.setConfiguration r?jib
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Create and use MediaTransportHandler::SetIceConfig. r?mjf
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Move initial configuration setting into a setConfiguration call. r?mjf
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Create NrIceCtx::SetIceConfig. r?mjf
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Allow empty list of STUN/TURN servers to be set. r?mjf
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Stop storing this as a bare pointer. r?mjf
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Don't bother trying to pair obsolete streams. r?mjf
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: If no pairs are started, but gathering is done, do not cancel the grace period timer. r?mjf
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Give each stream its own copy of the ICE config when it is created. r?mjf
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Set STUN/TURN servers _before_ adding streams, since that is required now. r?mjf
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Fix leaks/uafs caused by repeated setting of STUN/TURN servers. r?mjf
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Let failure to init a non-obsolete stream slide. r?mjf
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Remove this assert, since in ICE restart cases it may not be true. r?mjf
48 bytes, text/x-phabricator-request
Details | Review
Bug 1253706: Compensate for bug 1751509 by waiting for gathering to finish. r?jib
48 bytes, text/x-phabricator-request
Details | Review 
Today people set their config in the RTCPeerConnection constructor, so setConfiguration is only needed to reconfigure an existing peer connection.
Rank: 29
Priority: -- → P2
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
We need this feature to update ICE server list. 
See: https://github.com/versatica/mediasoup-client/issues/28

Is there any alternative way howto update ICE server list or the turn server credential?
Blocks: 1533015
Blocks: 1533017

Setting parity keywords.
We got support info for this wrong in our docs and a user reported: "Because of this bug, Firefox does not fit in our new security concept where the WebRTC user gets an generated username and password for a TURN server,"

See https://github.com/mdn/browser-compat-data/pull/4075

Keywords: parity-chrome, parity-safari
Type: defect → enhancement

In my case it is also needed with mediasoup-client with TURN server that has short-lived credentials.
Essentially, RTCPeerConnection is created upfront and only when it is about to be used credentials are generated and set using peer.setConfiguration() before calling peer.setRemoteDescription().
Not having this feature causes significant inconveniences for me.

Also it is supported in all major browsers except Firefox, please consider implementing it.

We have word from (more) partners this is costing a lot of connections that aren't going to succeed. During setup, sites are unable to adjust the configuration to adapt to changing network conditions, e.g. from RFC8828.

Up-prioritizing.

Priority: P3 → P2
Assignee: nobody → docfaraday

So, looking at this, right now nICEr keeps the set of ICE servers in the main ice context. We probably need to copy this set of ICE servers when creating each nr_ice_media_stream, so each stream has its own copy. This will allow the use of different sets of ICE servers for each stream so we don't run into dangling pointer problems when changing the set. This will also permit gathering to continue (and ICE to connect) on old streams, if for some reason JS waits after calling setConfiguration, which will mesh better with the way we handle ICE restart and rollback. I am not sure what the spec says to do if the ICE servers are changed with setConfiguration, gathering is started with sLD, and then that sLD is rolled back, but it seems reasonable that the code would continue trying to use the old servers until an ICE restart is actually carried out.

We will also need to break out the ICE server validation code into a separate function on MediaTransportHandler; right now that is built into CreateIceCtx, but that is only ever called once. Additionally, we need to do this validation synchronously, because setConfiguration is synchronous. We probably would not want to actually set anything on the NrIceCtx until we get to the StartIceGathering step, for simplicity's sake.

On the subject of ICE transport policy, we probably need to handle this identically to the ICE servers; each nr_ice_media_stream would need to have its own flags field. It would be fine if we copied from the flags field in nr_ice_ctx, probably. This would have the same desirable properties wrt ICE restart and rollback as above.

Lastly, while we have not yet implemented ICE candidate pool logic, the size of this pool can be tweaked with setConfiguration, so we need to think a little about what implementing that would look like. Presumably, with ICE candidate pooling we would either need to pre-create nr_ice_media_streams for later use, or have some other object that serves as a candidate set inside nICEr, which would then have its own copy of the necessary configuration. Also, there is a wrinkle here; ICE pool gathering starts immediately on setConfiguration. This implies that changing either the ICE servers or policy should result in the old pool being discarded immediately as well, even if the pool size has not changed. The spec explicitly mentions this for configuration.iceServers, but not for configuration.iceTransportPolicy (although it does say that "no action will be taken until the next gathering phase", which could be interpreted to apply to pooled ICE candidates). This may require some spec clarification.

On the subject of testing, there is a little bit of coverage in wpt, but it is extremely limited because wpt does not (and can not) test the actual use of ICE servers, or ICE transport policy, because wpt does not have the ability to use STUN/TURN servers. We will need to write some mochitests for this, in addition to enabling some of the wpt suite.

It looks like we can write at least a little bit of ICE testing for this in wpt by using iceTransportPolicy to set up situations where ICE gathering will yield no candidates (using the 'relay' policy). We can also switch from 'all' to 'relay' (and vice-versa) and get observable effects. We will not be able to test the same sort of behavior for iceServers in wpt, but it is at least something.

Blocks: 1739451
No longer depends on: 1739451

Re-enable several tests, and add some ICE testing using iceTransportPolicy.
Cannot test ICE servers, since wpt does not have any harness support for that.

Mainly covers ICE servers, since we don't have support for that in wpt.

Depends on D135361

Depends on D135363

Used to be built into CreateIceCtx, but needed to be stand-alone so it
could be called subsequently. Necessitated adding some members so pref-based
config state could be saved for later use.

Depends on D135364

Having a single codepath for this should make things easier to maintain.

Depends on D135365

Used to be part of NrIceCtx::Init, but needed to be broken out.

Depends on D135366

Once we allow the ICE servers to be set, each stream can have a different array
of ICE servers, and those ICE servers can have different lifetimes. Let's avoid
the potential dangling pointer issue here.

Also, this fixes a minor spec violation where we were doing foundation comparison
incorrectly (we're supposed to only compare the address).

Depends on D135368

This was a pre-existing bug detected by new tests.

Depends on D135369

This was a pre-existing bug detected by new tests. Also add a little logging.

Depends on D135370

Because a given ICE ctx can have multiple configurations active at once (since
setConfiguration is tentative until the ICE restart it requires is finalized), we
give each stream its own config, since that is simple.

I also renamed the stun/turn server fields in nr_ice_ctx, to make it easier to
check that I did not miss any places that needed to be switched over to using the
fields in nr_ice_media_stream.

Depends on D135371

This can happen in cases where we have an old ICE ctx that is infeasible, but have
kicked off an ICE restart with a new configuration that is feasible. The old streams
are not obsolete yet, because we have not committed the ICE restart.

Depends on D135374

It looks like we might need to relax some of the timeouts for windows aarch64. Otherwise, mochitest looks fine. Android 7.0 x86-64 WebRender debug-isolated-process is timing out on the new wpt, similar to how it times out on numerous other wpt already. This is all tier 2 stuff, so maybe not so big of a deal, but I'll at least try to get it working on windows aarch64.

Ok, figured out what was causing the problems on windows aarch64, and filed bug 1751509 for it. Working around by tweaking the test code.

See Also: → 1751509
Depends on: 1752896

The patch from Bug 1748852, combined with bug 1752896, caused lots of new failures after these patches were rebased. Right now, investigating whether there's an easy fix for bug 1752896. If not, I'll have to find a workaround in the tests.

https://treeherder.mozilla.org/jobs?repo=try&revision=0768835aefb20a3da296eea1a2b6fa571ca9bd75

See Also: → 1748852

Also add some logging that was useful, remove some cruft, and add a little
exception-safety.

Depends on D135376

Attachment #9258058 - Attachment is obsolete: true
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f16083aa8183
Web-platform-tests for bug. r=jib
https://hg.mozilla.org/integration/autoland/rev/65e035023ca2
Mochitests for setConfiguration. r=jib
https://hg.mozilla.org/integration/autoland/rev/2a227c50a598
Webidl/JS for RTCPeerConnection.setConfiguration r=jib,emilio
https://hg.mozilla.org/integration/autoland/rev/180fd6a03442
Create and use MediaTransportHandler::SetIceConfig. r=mjf
https://hg.mozilla.org/integration/autoland/rev/095ce58afb6a
Move initial configuration setting into a setConfiguration call. r=mjf,emilio
https://hg.mozilla.org/integration/autoland/rev/77f4be697b38
Create NrIceCtx::SetIceConfig. r=mjf
https://hg.mozilla.org/integration/autoland/rev/61e1c3949250
Allow empty list of STUN/TURN servers to be set. r=mjf
https://hg.mozilla.org/integration/autoland/rev/8efacf071abb
Stop storing this as a bare pointer. r=mjf
https://hg.mozilla.org/integration/autoland/rev/4c158f767584
Don't bother trying to pair obsolete streams. r=mjf
https://hg.mozilla.org/integration/autoland/rev/4c2f46c1235c
If no pairs are started, but gathering is done, do not cancel the grace period timer. r=mjf
https://hg.mozilla.org/integration/autoland/rev/47e6ed20b5af
Give each stream its own copy of the ICE config when it is created. r=mjf
https://hg.mozilla.org/integration/autoland/rev/ee68db9eb695
Set STUN/TURN servers _before_ adding streams, since that is required now. r=mjf
https://hg.mozilla.org/integration/autoland/rev/145e94926991
Fix leaks/uafs caused by repeated setting of STUN/TURN servers. r=mjf
https://hg.mozilla.org/integration/autoland/rev/fe16d24ab482
Let failure to init a non-obsolete stream slide. r=mjf
https://hg.mozilla.org/integration/autoland/rev/91a32bbc9193
Remove this assert, since in ICE restart cases it may not be true. r=mjf
https://hg.mozilla.org/integration/autoland/rev/ff344a02b277
Compensate for bug 1751509 by waiting for gathering to finish. r=jib
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/32725 for changes under testing/web-platform/tests

Backed out for causing Gtest failures.

Flags: needinfo?(docfaraday)
Upstream PR was closed without merging

Ugh. Need to update that test case, since there was a fix from bug 1752896.

Hmm, maybe I'll just remove that fix; disallowing STUN responses from unknown transaction ids (but from a known address) is probably beyond the scope of the STUN filter.

Flags: needinfo?(docfaraday)

Try looks good.

Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13f2b39a4e0b
Web-platform-tests for bug. r=jib
https://hg.mozilla.org/integration/autoland/rev/56935008c600
Mochitests for setConfiguration. r=jib
https://hg.mozilla.org/integration/autoland/rev/82b9f5abf881
Webidl/JS for RTCPeerConnection.setConfiguration r=jib,emilio
https://hg.mozilla.org/integration/autoland/rev/f040f134be49
Create and use MediaTransportHandler::SetIceConfig. r=mjf
https://hg.mozilla.org/integration/autoland/rev/f466f23b4f08
Move initial configuration setting into a setConfiguration call. r=mjf,emilio
https://hg.mozilla.org/integration/autoland/rev/0dfff3bd0b1e
Create NrIceCtx::SetIceConfig. r=mjf
https://hg.mozilla.org/integration/autoland/rev/3bbb2706fe05
Allow empty list of STUN/TURN servers to be set. r=mjf
https://hg.mozilla.org/integration/autoland/rev/b6e4501a6ec8
Stop storing this as a bare pointer. r=mjf
https://hg.mozilla.org/integration/autoland/rev/4ceb2e958df6
Don't bother trying to pair obsolete streams. r=mjf
https://hg.mozilla.org/integration/autoland/rev/faa316e8ed12
If no pairs are started, but gathering is done, do not cancel the grace period timer. r=mjf
https://hg.mozilla.org/integration/autoland/rev/33a99b2ca936
Give each stream its own copy of the ICE config when it is created. r=mjf
https://hg.mozilla.org/integration/autoland/rev/458238d76dc2
Set STUN/TURN servers _before_ adding streams, since that is required now. r=mjf
https://hg.mozilla.org/integration/autoland/rev/9c25fc4e6a83
Fix leaks/uafs caused by repeated setting of STUN/TURN servers. r=mjf
https://hg.mozilla.org/integration/autoland/rev/aaa4dfe8b392
Let failure to init a non-obsolete stream slide. r=mjf
https://hg.mozilla.org/integration/autoland/rev/b946cbf9f5d8
Remove this assert, since in ICE restart cases it may not be true. r=mjf
https://hg.mozilla.org/integration/autoland/rev/9fc47916fba0
Compensate for bug 1751509 by waiting for gathering to finish. r=jib
Upstream PR merged by moz-wptsync-bot

FF99 docs work for this can be tracked in https://github.com/mdn/content/issues/13371#issuecomment-1064737103
It is pretty much complete, and was mostly just browser compatibility update, new release note, and update of docs to include a few missing exceptions from the spec.

Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: