917619 - RFC 4572 negotiation doesn't work if answer contains no a=setup line (can't call pre-26 browsers from 26 and later)

Status: VERIFIED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Eric Rescorla (:ekr)]

The code in gsm_sdp.c assumes that if the other side doesn't supply an a=setup line they want to be passive. This is not backwards compatible if the new side is the offerer because then the answerer wants to be active.

Comment 1

[Eric Rescorla (:ekr)]

Attachment: Fix rules

Comment 2

[Eric Rescorla (:ekr)]

This defect means that you can't call old browsers (pre 26) from 26 and later.

I have attached a prototype patch.

This patch also includes a fixed test that should detect this problem.

Comment 3

[Eric Rescorla (:ekr)]

Adam,

Can you take a look at this and see if you think it's right? I was
a little fuzzy on the logic but it seems to work with my minimal
testing. If you think it's good, please r+ and land it.

If you think it's on the wrong track, can you try to rework it and
ask ehugg for review?

Comment 4

[Adam Roach [:abr]]

Attachment: Fix setup direction when is missing

Comment 5

[Adam Roach [:abr]]

Attachment: Fix setup direction when is missing

Comment 6

[Adam Roach [:abr]]

Comment on attachment 806858 [details] [diff] [review]
Fix setup direction when  is missing

Ethan: you'll note that I also removed the a=connection code in the patch and corresponding unit tests. This follows from RFC 5763: "The endpoint MUST NOT use the connection attribute defined in [RFC4145]."

Comment 7

[Ethan Hugg [:ehugg]]

Comment on attachment 806858 [details] [diff] [review]
Fix setup direction when  is missing

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

Looks good and worked for me against FF24 in both directions.  

You could remove gsmsdp_set_connection_attribute() since it is no longer used, or leave it if you think it'll be needed.  I'm not sure it will called be anytime soon.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +2880,5 @@
>    size_t match;
>  
>    a1_.CreateOffer(constraints, OFFER_AUDIO, SHOULD_SENDRECV_AUDIO);
>  
>    // By default the offer should give setup:actpass and connection:new

Remove connection:new from this line

@@ +2927,5 @@
>    size_t match;
>  
>    a1_.CreateOffer(constraints, OFFER_AUDIO, SHOULD_SENDRECV_AUDIO);
>  
>    // By default the offer should give setup:actpass and connection:new

And here.

Comment 8

[Adam Roach [:abr]]

Attachment: Fix setup direction when is missing

Comment 9

[Adam Roach [:abr]]

Comment on attachment 806887 [details] [diff] [review]
Fix setup direction when  is missing

Carrying forward r+ from ehugg.

https://hg.mozilla.org/integration/mozilla-inbound/rev/99002d48464e

Comment 10

[Adam Roach [:abr]]

Comment on attachment 806887 [details] [diff] [review]
Fix setup direction when  is missing

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):
Bug 844107

> User impact if declined: 
Firefox 26 will be unable to set up any WebRTC sessions with  Firefox 25 or earlier.

> Testing completed (on m-c, etc.):
Unit testing to verify behavior. Ethan performed manual verification that the fix allows calls between FFx 26 and FFx 25 (in both directions).
 
> Risk to taking this patch (and alternatives if risky): 
The change is isolated to WebRTC session negotiation code, and the changes are limited to the logic that determines which side of the connection will initiate the DTLS handshake. The worst plausible risk is that we will make an incorrect decision about which peer is supposed to take the active role; but as the existing codebase demonstrably already gets this wrong, this would be a wash at worst.

> String or IDL/UUID changes made by this patch:
None

Comment 11

[Adam Roach [:abr]]

(In reply to Adam Roach [:abr] from comment #10)

> > Bug caused by (feature/regressing bug #):
> Bug 844107

Correction: Bug 844071

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/99002d48464e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/89e6cb534865

Comment 14

[Mihaela Velimiroviciu (:mihaelav)]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:27.0) Gecko/20100101 Firefox/27.0, build ID: 20131010030202
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0, build ID: 20131010004002

Verified that 26.0 and 27.0 versions (caller, 2 person to join) successfully connect with 22.0-25.0 versions (callee, 1st person in call) (on other Mac OS X machine)