bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

RTCSessionDescription interface doesn't match spec

RESOLVED FIXED in Firefox 53

Status

()

Core
WebRTC
P1
normal
Rank:
15
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bz, Assigned: jib)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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
Comment hidden (mozreview-request)
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 7

2 years ago
mozreview-review
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 8

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 10

2 years ago
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

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ef70e4bf1a7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Will ride the trains
status-firefox52: affected → ---
You need to log in before you can comment on or make changes to this bug.