Closed Bug 1313966 Opened 3 years ago Closed 3 years ago

RTCSessionDescription interface doesn't match spec

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: bzbarsky, Assigned: jib)

References

Details

Attachments

(1 file)

The spec has:

  dictionary RTCSessionDescriptionInit {
      required RTCSdpType type;
               DOMString  sdp;
  };

  interface RTCSessionDescription {
    readonly attribute RTCSdpType type;
    readonly attribute DOMString  sdp;

(though it doesn't define what the "sdp" member is supposed to be set to when the dictionary doesn't define that member; that needs a spec issue).  Our IDL has:

  dictionary RTCSessionDescriptionInit {
    RTCSdpType? type = null;
    DOMString? sdp = "";
  };

  interface RTCSessionDescription {
    attribute RTCSdpType? type;
    attribute DOMString? sdp;

which has things writable and nullable when the spec has them as neither.
Flags: needinfo?(jib)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #0)
> (though it doesn't define what the "sdp" member is supposed to be set to
> when the dictionary doesn't define that member; that needs a spec issue). 

The spec says [1] of sdp: "if type is rollback, this member can be left undefined."

It's perhaps not super-clear, but setting a description with a type other than "rollback" without sdp will fail with InvalidModificationError (because we no longer allow user-modification, so the input must always be output from createOffer() or createAnswer() which never produce a description without sdp). [2][3]

Except, we presumably still allow setting a description with type "rollback", but from the same processing steps, that nulls the entire RTCSessionDescription attribute itself instead.

[1] http://w3c.github.io/webrtc-pc/#dom-rtcsessiondescriptioninit-sdp
[2] http://w3c.github.io/webrtc-pc/#set-description
[3] https://github.com/w3c/webrtc-pc/issues/420#issuecomment-162167942
Flags: needinfo?(jib)
> "if type is rollback, this member can be left undefined."

Right, for the dictionary.  But what does that mean for the resulting RTCSessionDescription object?

> but setting a description with a type other than "rollback" without sdp will fail with InvalidModificationError 

Why exactly?  I don't see any evidence for this in the spec.  Not least because the spec doesn't actually define any processing model for the constructor; as I said, that needs a spec issue.

> but from the same processing steps

What processing steps?  To be clear, I am looking at this:

  var desc = new RTCSessionDescription({ type: "answer" });

What is desc.sdp after this call and why?
Flags: needinfo?(jib)
Right, I thought we had deprecated the constructor, my bad.

I've opened https://github.com/w3c/webrtc-pc/issues/897
Flags: needinfo?(jib)
Oh, it's _deprecated_.  But "deprecated" shouldn't mean "yeah, define that it has to be, but not what it does".  ;)
Rank: 25
Priority: -- → P2
Rank: 25 → 15
Priority: P2 → P1
There are some outstanding issues with making this readonly, so how this for now?

[1] https://github.com/w3c/webrtc-pc/issues/573#issuecomment-259103219
[2] https://github.com/w3c/webrtc-pc/issues/897
Depends on: 1263312
Assignee: nobody → jib
Comment on attachment 8808848 [details]
Bug 1313966 - Add deprecation warnings to writable RTCSessionDescription.

https://reviewboard.mozilla.org/r/91580/#review91444
Attachment #8808848 - Flags: review?(bugs) → review+
Attachment #8808848 - Flags: review?(drno)
Comment on attachment 8808848 [details]
Bug 1313966 - Add deprecation warnings to writable RTCSessionDescription.

https://reviewboard.mozilla.org/r/91580/#review91490

LGTM.

Before we make them readonly we also check if none of our tests actually rely on this being writable.

::: dom/media/PeerConnection.js:232
(Diff revision 1)
> +
> +

If these don't serve a purpose I would prefer to remove them.
Attachment #8808848 - Flags: review?(drno) → review+
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ef70e4bf1a7
Add deprecation warnings to writable RTCSessionDescription. r=drno,smaug
Blocks: 1263312
No longer depends on: 1263312
https://hg.mozilla.org/mozilla-central/rev/1ef70e4bf1a7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Will ride the trains
You need to log in before you can comment on or make changes to this bug.