createOffer options spec-change to RTCOfferOptions abruptly breaks things in 33

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: abr, Assigned: jib)

Tracking

33 Branch
mozilla35
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32 unaffected, firefox33+ fixed, firefox34+ fixed, firefox35 fixed, firefox-esr31 unaffected)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Prior to Firefox 32, the syntax for creating an offer with receive-only video looked something like:

  peerConnection.createOffer(successCallback, failureCallback,
                             { mandatory: { OfferToReceiveVideo: true }});

With the changes introduced by Bug 1033833, this syntax changed to:

  peerConnection.createOffer(successCallback, failureCallback,
                             { offerToReceiveVideo: true });

And the old syntax yields an error: {"name":"INVALID_STATE","message":"Cannot create SDP without any streams."}

(To complicate matters, Chrome 38 will not accept the new syntax, instead throwing: "Uncaught TypeError: Failed to execute 'createOffer' on 'RTCPeerConnection': Malformed constraints object," so developers will have to do some pretty advanced UA string sniffing to make this all work)

This means that many currently deployed application are likely to break on Beta right now, and will break on Release in a few weeks, unless we take action.

I propose that we land a fix that looks for "optional" or "mandatory" at the top level of the constraints object; and, if present, performs processing that is identical to or largely similar to the old behavior (including the old label case). We should also emit a deprecation error on the console, and plan to remove this behavior a few releases down the road (the timing of which should take into account when Chrome updates to the new constraints syntax).

Ideally, we uplift this change into Beta so we don't have developers coming for our heads when 33 gets into release. :)
Comment on attachment 8485692 [details] [diff] [review]
support old constraint-like RTCOfferOptions for a bit

Review of attachment 8485692 [details] [diff] [review]:
-----------------------------------------------------------------

This generally looks good -- I don't like the prospect of leaving commented-out code lying around, though.

r+ if nits are addressed.

::: dom/media/PeerConnection.js
@@ +578,5 @@
> +      o.offerToReceiveAudio = old.OfferToReceiveAudio;
> +      o.offerToReceiveVideo = old.OfferToReceiveVideo;
> +      o.mozDontOfferDataChannel = old.MozDontOfferDataChannel;
> +      o.mozBundleOnly = old.MozBundleOnly;
> +      Object.keys(o).forEach(k => (o[k] !== undefined) || delete o[k]);

This formulation sacrifices readability for cleverness. Please reformulate as a more traditional "if" statement.

::: dom/media/tests/mochitest/pc.js
@@ +2050,5 @@
> +               options.optional[0].OfferToReceiveAudio !== undefined) {
> +      offerToReceiveAudio = options.optional[0].OfferToReceiveAudio;
> +    }
> +    if (offerToReceiveAudio) {
> +//  if (options.offerToReceiveAudio) {

Remove rather than comment out, please.

@@ +2098,5 @@
> +               options.optional[0].OfferToReceiveVideo !== undefined) {
> +      offerToReceiveVideo = options.optional[0].OfferToReceiveVideo;
> +    }
> +    if (offerToReceiveVideo) {
> +//  if (options.offerToReceiveVideo) {

Remove

::: dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveAudio.html
@@ +20,5 @@
>      var test = new PeerConnectionTest();
> +
> +    // TODO: Remove tests of old constraint-like RTCOptions soon (Bug 1064223).
> +    test.setOfferOptions({ mandatory: { OfferToReceiveAudio: true } });
> +//  test.setOfferOptions({ offerToReceiveAudio: true });

It seems to me that we should be testing both forms, right? I think you want to add a test rather than modifying the existing one.

::: dom/media/tests/mochitest/test_peerConnection_offerRequiresReceiveVideo.html
@@ +19,5 @@
>    runNetworkTest(function() {
>      var test = new PeerConnectionTest();
> +    // TODO: Remove tests of old constraint-like RTCOptions soon (Bug 1064223).
> +    test.setOfferOptions({ optional: [{ OfferToReceiveAudio: true }] });
> +//  test.setOfferOptions({ offerToReceiveVideo: true });

Same here: add rather than change.
Attachment #8485692 - Flags: review?(adam) → review+
Comment on attachment 8485692 [details] [diff] [review]
support old constraint-like RTCOfferOptions for a bit



>+      if (o.optional) {
>+        o.optional.forEach(one => {
>+          let key = Object.keys(one)[0]; // only one key per entry in old format
I don't understand this.
Attachment #8485692 - Flags: review+ → review?(adam)
oops, I managed to clear adam's r+.
Attachment #8485692 - Flags: review?(adam) → review+
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 8485692 [details] [diff] [review]
> support old constraint-like RTCOfferOptions for a bit
> 
> 
> 
> >+      if (o.optional) {
> >+        o.optional.forEach(one => {
> >+          let key = Object.keys(one)[0]; // only one key per entry in old format
> I don't understand this.


Basically, the way this was defined in older specs was that optional was an array of objects, but each object could have only one attribute in them.

  { optional: [ {name1: value}, {name2: value}, {name3: value} ] }

It wouldn't be legal to have, say:

  { optional [ {name1: value, name2: value} ] }

So what this code is doing is pulling out the first (and only) key name from the object so it can be copied into the new constraints format.
Oh, it relies on the JS-webidl bindings layer to work in a certain way, so that the properties aren't
there if not passed explicitly?
Comment on attachment 8485692 [details] [diff] [review]
support old constraint-like RTCOfferOptions for a bit


>   createOffer: function(onSuccess, onError, options) {
>-    options = options || {};
>+
>+    // TODO: Remove old constraint-like RTCOptions support soon (Bug 1064223).
>+    function convertLegacyOptions(o) {
>+      if (!(o.mandatory || o.optional) ||
>+          Object.keys(o).length != ((o.mandatory && o.optional)? 2:1)) {
space before and after :


>+      if (o.optional) {
>+        o.optional.forEach(one => {
>+          let key = Object.keys(one)[0]; // only one key per entry in old format
Ok, so this could use some comment.
Attachment #8485692 - Flags: review?(bugs) → review+
(In reply to Adam Roach [:abr] from comment #2)
> ::: dom/media/PeerConnection.js
> @@ +578,5 @@
> > +      o.offerToReceiveAudio = old.OfferToReceiveAudio;
> > +      o.offerToReceiveVideo = old.OfferToReceiveVideo;
> > +      o.mozDontOfferDataChannel = old.MozDontOfferDataChannel;
> > +      o.mozBundleOnly = old.MozBundleOnly;
> > +      Object.keys(o).forEach(k => (o[k] !== undefined) || delete o[k]);
> 
> This formulation sacrifices readability for cleverness. Please reformulate
> as a more traditional "if" statement.

I chose this approach here to avoid typo bugs since it produces the least repetition. Note the difference in case in a lot of these. They're each mentioned once right now. Switching to if's doubles this. Not sure I agree arrow functions are less readable. Is there a version that would be suitable short of individual if's?

> It seems to me that we should be testing both forms, right? I think you want
> to add a test rather than modifying the existing one.

There are at least three tests relying on onOfferToReceive, and I changed two of them, one to rely on the mandatory form and one to rely on the optional form. The remaining one continues to rely on the new form, so doesn't that leave us covered?
Flags: needinfo?(adam)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #8)
> (In reply to Adam Roach [:abr] from comment #2)
> > ::: dom/media/PeerConnection.js
> > @@ +578,5 @@
> > > +      o.offerToReceiveAudio = old.OfferToReceiveAudio;
> > > +      o.offerToReceiveVideo = old.OfferToReceiveVideo;
> > > +      o.mozDontOfferDataChannel = old.MozDontOfferDataChannel;
> > > +      o.mozBundleOnly = old.MozBundleOnly;
> > > +      Object.keys(o).forEach(k => (o[k] !== undefined) || delete o[k]);
> > 
> > This formulation sacrifices readability for cleverness. Please reformulate
> > as a more traditional "if" statement.
> 
> I chose this approach here to avoid typo bugs since it produces the least
> repetition. Note the difference in case in a lot of these. They're each
> mentioned once right now. Switching to if's doubles this. Not sure I agree
> arrow functions are less readable. Is there a version that would be suitable
> short of individual if's?

That's not what I meant. I'm objecting to the formulation of "(!x) || (y)" as a stand-in for "if (x) { y; }"

Object.keys(o).forEach(k => {
  if (o[k] === undefined) {
    delete o[k];
  }
});

> > It seems to me that we should be testing both forms, right? I think you want
> > to add a test rather than modifying the existing one.
> 
> There are at least three tests relying on onOfferToReceive, and I changed
> two of them, one to rely on the mandatory form and one to rely on the
> optional form. The remaining one continues to rely on the new form, so
> doesn't that leave us covered?

I suppose that's enough coverage, sure.
Flags: needinfo?(adam)
(In reply to Olli Pettay [:smaug] from comment #6)
> Oh, it relies on the JS-webidl bindings layer to work in a certain way, so
> that the properties aren't there if not passed explicitly?

True. I'll mention that in the comment.
(In reply to Adam Roach [:abr] from comment #9)
> That's not what I meant. I'm objecting to the formulation of "(!x) || (y)"
> as a stand-in for "if (x) { y; }"

Ah, the victory || death pattern.

> Object.keys(o).forEach(k => {
>   if (o[k] === undefined) {
>     delete o[k];
>   }
> });

Sure. All on one line ok?
Incorporated feedback. Carrying forward r=smaug, abr.
Attachment #8485692 - Attachment is obsolete: true
Attachment #8486082 - Flags: review+
Green try from earlier: https://tbpl.mozilla.org/?tree=Try&rev=23b94c92a97d
Keywords: checkin-needed
Summary: Constraints handling change abruptly breaks things → createOffer options spec-change to RTCOfferOptions abruptly breaks things in 33
(In reply to Jan-Ivar Bruaroey [:jib] from comment #11)

> > Object.keys(o).forEach(k => {
> >   if (o[k] === undefined) {
> >     delete o[k];
> >   }
> > });
> 
> Sure. All on one line ok?

Why would you do that? We're not renting space on a per-line basis or anything. You're doing something outside normal style, and it's not clear what is gained.

It's pretty clear what is lost, though: there's good anecdotal evidence and growing experimental evidence [1][2] that varying from expected coding norms has the effect of slowing experienced programmers down to the speed of novices under good circumstances, and actually causing experienced programmers to misread code under bad ones.

And cramming an if statement onto one line when the style of surrounding code rigorously does not, and when the governing code guidelines clearly point away from doing so[3], is only going to harm readability.

<grumpy-mode> I really hate doing re-reviews when the needed changes are trivial, but an "r+ with nits" is exactly that -- if you can't resolve the nits, the r+ doesn't stand unless and until we chase the raised issues to ground. I have a short list of people who I've learned need an r- because I can't be sure they'll do an "r+ with nits" correctly. Neither of us want you on that list. </grumpy-mode>

____
[1] http://synesthesiam.com/posts/modeling-how-programmers-read-code.html
[2] http://synesthesiam.com/posts/what-makes-code-hard-to-understand.html
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
Keywords: checkin-needed
Attachment #8486082 - Flags: review+ → review-
Incorporated feedback correctly.

(In reply to Adam Roach [:abr] from comment #14)
> Why would you do that?

On the off chance that that's not rhetorical: because I'm fascinated with arrow functions and still experimenting with their use. Arrow functions seem to favor declarative over imperative programming, with their waiver of semicolon+brackets for single expressions, implicit inline context, and, you know, the arrow pointing to the right. Arrow functions just read a lot better to me when used declaratively.

I can read x => x*x imperatively as "x is an input to a square function", but declaratively the whole statement screams "I want the square of x", or generally for x=>y "I want y of x". This seems most powerful when the entire small statement can be read in a single line, because it is one idea (that scanning thing you mention factors in).

So I attempted to express what I wanted declaratively - "be undefined or be gone" - rather than as a sequence of imperatives - "if something, then remove something" - since it read better to me as a single idea in the context of an arrow function.

But since it wasn't readable to you, then I may have been wrong about that. That's quite possible as I'm figuring things out and may be pushing too far. Anyhow that's the thinking that then softened my thinking that violating the age-old K&R perhaps wasn't terrible in this instance. That, and rejecting { (x !== undefined)? delete x : true; } along the way.
Attachment #8486082 - Attachment is obsolete: true
Attachment #8486244 - Flags: review?(adam)
Whoops, meant to write: "be defined or be gone".
Comment on attachment 8486244 [details] [diff] [review]
support old constraint-like RTCOfferOptions for a bit (3) r=smaug

Review of attachment 8486244 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

I wouldn't try to lean too far forward on style. If you have some ideas around new code formatting paradigms, I would vet them on #developers or dev-platform before attempting to commit them to the tree.
Attachment #8486244 - Flags: review?(adam) → review+
https://hg.mozilla.org/mozilla-central/rev/9855cd3e32e2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
I presume we want to uplift this to 33 before it escapes to release.
Whiteboard: [webrtc-uplift]
Comment on attachment 8486244 [details] [diff] [review]
support old constraint-like RTCOfferOptions for a bit (3) r=smaug

Approval Request Comment
[Feature/regressing bug #]:
Bug 1033833 broke compatibility with no warning.

[User impact if declined]:
Need both alpha and beta in order to not break compatibility on release in a few weeks, or many currently deployed applications doing receive-only video or audio are likely to break with an error.

[Describe test coverage new/current, TBPL]:
Patch modifies two out of three OfferToReceive* tests to rely on old format (mandatory and optional constraint respectively) and runs green on try (comment 13) and m-c.

[Risks and why]: Very low risk. JS-only change turns old dictionary arg - if detected - into new dictionary arg. Test-verified.

[String/UUID change made/needed]: none
Attachment #8486244 - Flags: approval-mozilla-beta?
Attachment #8486244 - Flags: approval-mozilla-aurora?
Comment on attachment 8486244 [details] [diff] [review]
support old constraint-like RTCOfferOptions for a bit (3) r=smaug

I don't like the idea of developers coming for our heads (comment 0). ;)

Beta+
Aurora+
Attachment #8486244 - Flags: approval-mozilla-beta?
Attachment #8486244 - Flags: approval-mozilla-beta+
Attachment #8486244 - Flags: approval-mozilla-aurora?
Attachment #8486244 - Flags: approval-mozilla-aurora+
Re-opening to discuss how to proceed with the bug which got introduced into test_peerConnection_offerRequiresReceiveVideo.html by the attachment 8486244 [details] [diff] [review].
attachment 8486244 [details] [diff] [review] turned a video test into an audio test, and thus we no longer have any offer requires video test.

Either we patch this from within here, or we open a new bug for the fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Simple one line patch to fix the tests.
Comment on attachment 8490526 [details] [diff] [review]
bug_1063808_fix_offerRequiresVideo.patch

Review of attachment 8490526 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, and sorry about that!
Attachment #8490526 - Flags: review+
Try run looks green, requesting checkin.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9dbcd7b22491
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 1070076
Whiteboard: [webrtc-uplift]
Duplicate of this bug: 1047895
You need to log in before you can comment on or make changes to this bug.