Missing url in RTCIceCandidateInit

[alvestrand]

#2773 did not specify an URL parameter to RTCIceCandidateInit, which makes a difficulty for creating RTCIceCandidates from scratch (such as when doing emulations).

This will also affect the ToJson algorithm.

[fippo]

that is intentional? This property should not be constructable from JS.

toJSON isn't affected since https://w3c.github.io/webrtc-pc/#dom-rtcicecandidate-tojson only selects a subset of the attributes

[youennf]

This property should not be constructable from JS.

It might not be super useful to construct it but why should it not be constructible from JS?
All other properties are settable from JS so there is an expectation that this also would be constructible.
Maybe this is the reason why it was in the event before and not the candidate itself.
This could for instance create some surprising effects if postMessaging via JSON for instance.

[alvestrand]

if an object is constructible at all, it is nice to be able to construct it with a full set of attributes (unless those attributes are emergent properties of the operation of the object).
A candidate is basically a dead data chunk ("struct") - when writing tests, for instance, constructing candidates with urls in them might be convenient.

[fippo]

All other properties are settable from JS

Most of the properties are not settable from JS but are parsed from the candidate string.
Not having a URL is correct since any candidate created by JS was not gathered from a server.

For testing you can still assign a value candidate.url if you want.

[alvestrand]

From editors meeting discussion: The question is why we want the URL in the candidate in the first place.

• Do we want to send it to the other side?
• Do we want to make sure we hide it from the other side?
• Are there places in the path that can use this information, short of the other side?
• Is testing important?
  Seems to be enough questions.

[fippo]

• Do we want to send it to the other side?

No.

• Do we want to make sure we hide it from the other side?

Yes. This might leak your location which is undesirable.

• Are there places in the path that can use this information, short of the other side?

You may want to act differently depending on whether you use your own "primary" TURN server or a fallback one (i've just seen such a case again). This may be done either as a result of a getStats call (which already has the URL) or in the selectedcandidatepairchange event.

Have you folks looked at https://webrtc.github.io/samples/src/content/peerconnection/trickle-ice/ which actually displays the url?

[steely-glint]

Referring to the (sub) issue that folks are using candidate.toJSON() already and that filtering out the URL would break their expectations - as discussed at today's interim.

Can we leverage the semantic difference between JSON.stringify() and toJSON() ?

[alvestrand]

OK, I'll bite - what is the semantic difference between JSON.stringify() and toJSON()?

[jan-ivar]

... folks are using candidate.toJSON() ...

What's your source for this, and why would this group be larger than folks using JSON.stringify(candidate)?

I don't think there's a semantic difference, and if there is one, I wouldn't exploit it.

Since relayProtocol and url are not implemented by anyone, folks expecting to see them will be disappointed.

Web compat here is that for:

1. JSON.stringify(candidate) — only candidate, sdpMid and sdpMLineIndex appear
2. new RTCIceCandidate(JSON.parse(JSON.stringify(candidate))) — All members (except relayProtocol and url) are rehydrated from the candidate string

I claim there's no web compat problem here. The only concern might be people expecting relayProtocol and url to start showing up in 2, but nothing in the spec supports such an assumption.

[fippo]

I claim there's no web compat problem here

Agree. Further I am not sure that the assumption that folks actually use toJSON or rely on it in production apps is true.
I stopped relying on it after https://bugs.chromium.org/p/chromium/issues/detail?id=467471 and generally prefer explicit signaling.

Web compat here is that for:
JSON.stringify(candidate) — only candidate, sdpMid and sdpMLineIndex appear

This is not what the spec says and we seem to lack a test.
In Chrome usernameFragment is not serialized, Safari serializes it as null, only Firefox ✔️

[youennf]

I claim there's no web compat problem here. The only concern might be people expecting relayProtocol and url to start showing up in 2, but nothing in the spec supports such an assumption.

The spec clearly supports this assumption as it states that all attributes are derived from candidate except candidate, sdpMid, sdpMLineIndex and usernameFragment. These are precisely the 4 members that the spec serialises in toJSON.
I think this explains why url was put in RTCPeerConnectionIceEvent in the first place and not the candidate.
Also, am I right in thinking that all current implementations are actually supporting full fidelity?

relayProtocol and url are new proposals and they both break this assumption.
It is obviously unfair to say that it is safe to do it for url given we have done it for relayProtocol, none of them have been implemented yet AFAIK.

Maybe we should have introduced a RTCIceLocalCandidate that could be partially exported to a RTCIceCandidateInit, representing a remote candidate. This model seems cleaner to me.
That said, this is too late, adding a subclass now is probably not feasible, see RTCPeerConnectionIceEvent constructor.
I do not see any clean solution, sigh...

The current spec state is not great given the merged PRs have been missing some areas where the spec would need to be updated.
To properly expose relayProtocol and url directly in RTCIceCandidate, we might want to:

• change RTCIceCandidate definition to not let people expect toJSON full fidelity. Maybe also add a comment in the WebIDL stating that the fields below are for local candidates only and are not expected to be exported.
• a way to construct a candidate with both relayProtocoland url values being set by JS. This might be a bit odd to add them to RTCIceCandidateInit but maybe this is good enough.
• change RTCPeerConnectionIceEventInit to not include url.
  The alternative is to put relayProtocol and url back in the event, consistent with the spec but this is less ergonomic and less aligned with other web APIs.

[jan-ivar]

The spec clearly supports this assumption as it states that all attributes are derived from candidate except candidate, sdpMid, sdpMLineIndex and usernameFragment.

You're correct it says "Other than candidate, sdpMid, sdpMLineIndex, and usernameFragment, the remaining attributes are derived from parsing the candidate member in candidateInitDict, if it is well formed.", which #2763 broke. We at minimum need a PR to fix that sentence unless we revert both relayProtocol and url.

It is obviously unfair to say that it is safe to do it for url given we have done it for relayProtocol, none of them have been implemented yet AFAIK.

What does "safe" mean? What's an example of a web compat pattern that would break?

Maybe we should have introduced a RTCIceLocalCandidate that could be partially exported to a RTCIceCandidateInit, representing a remote candidate. This model seems cleaner to me.

It might be worth looking at why we have an interface at all:

1. event.candidate cannot be a dictionary
2. It's a convenient candidate string parser: e.g. new RTCIceCandidate(candidate).foundation

Number 2 is likely the only reason we didn't deprecate the constructor like we did for RTCSessionDescription in #302, and I don't see how the lack of relayProtocol and url would impact that use case.

One benefit of keeping them where they are in the spec would be they'd appear in the result from getLocalCandidates():

const [{relayProtocol, url}] = sender.transport.iceTransport.getLocalCandidates();

They would obviously be null in getRemoteCandidates().

[fippo]

Also, am I right in thinking that all current implementations are actually supporting full fidelity?

from https://wpt.fyi/results/webrtc/toJSON.html is a great example of the disconnect between "what the working group thinks" and reality. This was not even covered by WPT.

"web developers" do not seem to care about usernameFragment in toJSON. I have not found a bug complaining about the lack of usernameFragment even and had to file one.

If you look at major opensource projects, Jitsi and MediaSoup don't even bother with the concept of using JSON strings for signaling. Janus has a healthy distrust for JSON.stringify (and toJSON) as evidenced here because of past issues. Browser improved? However, as shown by the lack of WPT the lack of trust is very much justified.

[fippo]

reading the minutes and watching the recording...
please note that as shown by https://jsfiddle.net/fippo/9hcbp7va/2/ JS is perfectly fine to modify read-only properties of the candidate thanks to Object.defineProperty.

Further note that the url of the server a candidate was gathered from is not relevant to the other side (i.e. addIceCandidate). If it was, it would be transported in the candidate string or mentioned in RFC 5245 or its successor.

[jan-ivar]

... JS is perfectly fine to modify read-only properties of the candidate thanks to Object.defineProperty.

Thanks for this discovery (it revealed another bug in firefox in this area as well).

I think we should fix the sentence in #2795 (comment), maybe add a note about relayProtocol and url, and call this done.

IOW relayProtocol and url are local overload APIs (same behavior we'd get from a subclass) of the RTCIceCandidate platform object that don't survive serialization to JSON, because they're not meant for signaling or addIceCandidate.

Libraries that wish to shim can do so using Object.defineProperty. Ready for PR?

[youennf]

A shim cannot modify the internal slot. IIRC, the following points have been expressed during the call:

1. A getter is nothing but a function that can be retrieved from the prototype. It can be called directly and if so would return a the internal slot value which can be different from the shimed value. This can potentially end up into strange edge cases.
2. Setting the actual internal slot would be handy, at least for testing. The shim is not useful if we want to write a WPT test with a candidate that has a url value and we want to check that calling toJSON on this candidate is not exposing url.

[alvestrand]

Shimming could work for the use case of testing Javascript code without having to involve WebRTC, which was the use case I was thinking of when wanting to be able to construct candidates with URL.
But one can always use duck typing - just make up something that quacks like a candidate (with URL).
WPT tests test the implementation; those have to use real objects.

[fippo]

and in WPT we lack the infrastructure to test STUN/TURN which is why several stats are marked as "not testable" already

[jan-ivar]

It can be called directly and if so would return a the internal slot value which can be different from the shimed value. This can potentially end up into strange edge cases.

relayProtocol calls out that these are "only present for local relay candidates". Ditto url.

I don't see any specs referencing these internal slots. Is any spec planning to?

[youennf]

I might be mistaken here, but isn't the the sole purpose of the RTCICeCandidate constructor to create a candidate that will then be fed to RTCPeerConnectionIceEvent?
If so, it would seem strange that we would design a half baked constructor and let it to JS to handle properties we do not deem useful enough.

I don't see any specs referencing these internal slots. Is any spec planning to?

It is not specs. It is JS that can retrieve the value in different ways and the values will defer when shiming is used.

[fippo]

Exposing the constructor in JS is, like RTCSessionDescription an artifact from the time when you had to write code like this:

pc.setRemoteDescription(new RTCSessionDescription({...json})

From this commit I'd say this was before 2016.

[youennf]

Attributes do not like dictionaries.
So, even in a perfect post 2016 world, we would need a RTCIceCandidate constructor to be able to build a RTCPeerConnectionIceEvent. Or we would have passed a RTCIceCandidateInit to RTCPeerConnectionIceEvent constructor, in which case it would seem obvious that we should pass url.

[dontcallmedom-bot]

This issue had an associated resolution in WebRTC January 2023 meeting – 17 January 2023 (Issue #2795 Missing URL in RTCIceCandidateInit 🎞︎):

RESOLUTION: mild preference to add url to icecandidate constructor and consensus to remove url from IceEventInit