RTCSessionDescription parameters should be readonly.

[jan-ivar]

The current language implies that content can modify the local description like this:

pc.localDescription.type = "answer";

which would be a travesty (an interface as an attribute is by reference).

In light of #302 I propose we solve this by changing RTCSessionDescription's parameters to readonly, and change createOffer and createAnswer to resolve with RTCSessionDescriptionInit instead.

[alvestrand]

"which would be a travesty (an interface as an attribute is by reference)."
I can't parse either the grammar of the sentence, what your opinion is, or why you have it.

The localDescription and remoteDescription parameters of the PC are already readonly, which makes the code you suggest illegal (unless I've misunderstood the properties of a readonly attribute).

4.7.2 says "Attributes on this interface are mutable for legacy reasons." I see no compelling reason to change it.

[adam-be]

The type attribute was readonly to begin with, but we changed it to allow scripts to, for example, switch from an "answer" to a "pranswer" before setting.

[jan-ivar]

I checked with our DOM guys, and readonly just means there's no setter for .localDescription. The returned interface is still mutable, which seems problematic.

If we do what I suggest then there's no reason to leave them for legacy reasons, since createOffer would return RTCSessionDescriptionInit, a dictionary, instead.

[jan-ivar]

Also, dictionaries have copy semantics, whereas interfaces do not. So a conforming implementation of the current webidl MUST work like this:

var pc = new RTCPeerConnection();
pc.createDataChannel("dummy");
pc.createOffer().then(offer => pc.setLocalDescription(offer))
.then(() => {
  console.log(pc.localDescription.type); // offer
  pc.localDescription.type = "answer";
  console.log(pc.localDescription.type); // answer
})
.catch(log);

Luckily, neither Firefox nor Chrome are compliant.

[martinthomson]

You can't have an attribute with a dictionary type. The right solution is to make RTCSessionDescription immutable itself, as @jan-ivar says originally. That we are all current in flagrant defiance of expectations here isn't that bad, we just make the attributes readonly and move on.

[jan-ivar]

Yes I just meant remove the legacy mutability. The interfaces are needed forever for the attributes.

[foolip]

https://crbug.com/662898 is a bug originating from @philn with Igalia where they (I presume) they tried to follow the spec but ran into trouble.

@philn, if you think the spec should change, can you file a new issue describing the compat constraints?

[foolip]

Somewhat relevant is also whatwg/webidl#213, but compat demands what it demands. I've asked on the Chromium issue if appear.in might change in the meantime.

@ShijunS, do you know what the Edge behavior is here?

[philn]

In WebKit we're following what the spec defined: https://github.com/WebKit/webkit/blob/master/Source/WebCore/Modules/mediastream/RTCSessionDescription.idl#L41

[foolip]

Somewhat related is that RTCSessionDescriptionInit's type is required in the spec but not in Gecko or Blink. I'll sprinkle some use counters in Blink, not sure whether reverting the spec change or trying to match the spec is better overall.

[bzbarsky]

I'm 90% sure that both Gecko and Blink implemented RTCSessionDescriptionInit before IDL "required" syntax existed, which is why they're not using it yet...

[foolip]

Indeed, no surprise there.

Per https://twitter.com/pnormand/status/795938320224309249 appear.in changed their code, so let's hope that most other usage in the wild doesn't depend on the current behavior. (Use counters in https://codereview.chromium.org/2490543002/)

[ShijunS]

Re Edge behavior, we have tried to follow the spec before this change. However, in our implementation, both localDescription and remoteDescription are new interface objects copied from the media stack. They can change when app calls setLocalDescription() and setRemoteDescription(). But, we don't pass down any app-level change to the lower level. There is no major conflict to the change of "type" and "sdp" to readonly.

At this point, setLocalDescription() and setRemoteDescription() still require RTCSessionDescription interface object as input, and will throw exception on RTCSessionDescriptionInit. Similarly, createOffer() and createAnswer() will return RTCSessionDescription interface object rather than RTCSessionDescriptionInit dictionary. I expect this option will stay until all major browser implementations and websites have migrated according to the latest spec.

[jan-ivar]

https://crbug.com/662898

Looks like we missed a common use-case when we made this read-only:

new RTCSessionDescription().sdp = "v=0\no=- 627673561523047307...";

I suppose we could solve it by using a different interface for the attribute... RTCSessionDescriptionAttr ?

[jan-ivar]

@foolip Or maybe make RTCSessionDescription writable again and Object.freeze() the specific attributes instead? Is there some webidl for that?

[bzbarsky]

Freeze which attributes? What are their types?

[jan-ivar]

[Frozen]

[jan-ivar]

@bzbarsky pc.localDescription and friends.

[bzbarsky]

I don't see how freezing them would do any good. What would be the goal?

[jan-ivar]

@bzbarsky To prevent pc.localDescription.type = "answer"; as mentioned at the top of this issue.

[bzbarsky]

Freezing it won't prevent that, because the setter isn't mutating any state the freezing freezes.

[jan-ivar]

Ok @bzbarsky showed me freezing only works on value properties, not setters, so that won't work.

So I revert to the RTCReadonlySessionDescription idea, or since the RTCSessionDescription constructor is legacy, we could just ignore the webcompat on this one and hope it is small.