1502766 - Firefox 64 does not respect the RTCConfiguration iceTransportPolicy

Status: VERIFIED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Adam Ullman]

When you set the iceTransportPolicy to 'relay' Firefox 64 still uses peerreflexive. This is a regression, it is not the case in Firefox 63.

Steps to reproduce:

1. Go to https://output.jsbin.com/luqozo
2. Allow access to your devices
3. Go to about:webrtc

Result: You will see that the candidate that succeeded is the peerreflexive candidate.

Expected Result: It should be one of the relayed candidates because we set the iceTransportPolicy to be 'relay'. If you try this in Firefox 63 or in Chrome you will see that it uses the relay candidate.

Comment 1

[Nils Ohlmeier [:drno]]

I do see that for 63 we only give out relay candidates. But with 64 we do give out all candidates (host, srvrflx, relay).

What is strange is that in 63 about:webrtc doesn't show any candidates for the video m-section, even though I don't see a bundle-only.

I don't think we touch that much ICE code in 64 to cause something like this, but you never know.
Byron, could you please have a look if this is really a regression?

Comment 2

[Nils Ohlmeier [:drno]]

Lets assume this a regression until it's proven otherwise.

Comment 3

[Byron Campen [:bwc]]

(In reply to Nils Ohlmeier [:drno] from comment #1)
> I do see that for 63 we only give out relay candidates. But with 64 we do
> give out all candidates (host, srvrflx, relay).
> 
> What is strange is that in 63 about:webrtc doesn't show any candidates for
> the video m-section, even though I don't see a bundle-only.
> 
> I don't think we touch that much ICE code in 64 to cause something like
> this, but you never know.
> Byron, could you please have a look if this is really a regression?

So this code was moved around a little, but I don't see why it would stop working. I'll look into it.

Comment 4

[Byron Campen [:bwc]]

So it does seem to be using non-relay candidates in beta, but nightly uses only relay candidates. Something is fishy here.

Comment 5

[Byron Campen [:bwc]]

Also, for some reason this test-case breaks the webrtc dev-tools panel.

Comment 6

[Byron Campen [:bwc]]

^

Comment 7

[Byron Campen [:bwc]]

We have a test for this, btw, that was disabled for some reason in the webrtc.org 43 merge, despite not being present on any patch on bug 1198458: https://hg.mozilla.org/mozilla-central/rev/ae32ad44ce1c

Comment 8

[Byron Campen [:bwc]]

Ok, found the problem. It is a promiscuous typecasting bug.

Comment 9

[Byron Campen [:bwc]]

Attachment: Part 1 (beta only): Pass enough args to NrIceCtx::Create

Comment 10

[Byron Campen [:bwc]]

Attachment: Part 0: Re-enable test for "relay" ICE policy

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 9020804 [details] [diff] [review]
Part 1 (beta only): Pass enough args to NrIceCtx::Create

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1493765

User impact if declined: iceTransportPolicy will not work properly in 64, which could cause internal addresses to be disclosed when the PeerConnection is configured not to disclose them. This bug was already fixed on 65 by bug 1494301.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is effectively a fix for a syntax error.

String changes made/needed: None.

Comment 12

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ef0af75a7f4332dc1887aab344a04e06bd45e4e

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 9020805 [details] [diff] [review]
Part 0: Re-enable test for "relay" ICE policy

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

::: dom/media/tests/mochitest/test_peerConnection_relayOnly.html
@@ +16,5 @@
>    test.pcLocal._pc.addEventListener("icecandidate", e => isnt(e.candidate));
>  }
>  
>  function PC_BOTH_WAIT_FOR_ICE_FAILED(test) {
> +  var isFail = (f, msg) =>

Maybe const since you're here.

@@ +21,2 @@
>      f().then(() => { throw new Error(msg + " must fail"); },
> +             () => {});

This seems fragile to me. Now basically any error in waitForIceConnected() will give us a false positives here. Why did we remove this?

Comment 14

[Byron Campen [:bwc]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #13)
> Comment on attachment 9020805 [details] [diff] [review]
> Part 0: Re-enable test for "relay" ICE policy
> 
> Review of attachment 9020805 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/tests/mochitest/test_peerConnection_relayOnly.html
> @@ +16,5 @@
> >    test.pcLocal._pc.addEventListener("icecandidate", e => isnt(e.candidate));
> >  }
> >  
> >  function PC_BOTH_WAIT_FOR_ICE_FAILED(test) {
> > +  var isFail = (f, msg) =>
> 
> Maybe const since you're here.
> 
> @@ +21,2 @@
> >      f().then(() => { throw new Error(msg + " must fail"); },
> > +             () => {});
> 
> This seems fragile to me. Now basically any error in waitForIceConnected()
> will give us a false positives here. Why did we remove this?

Because waitForIceConnected doesn't pass anything in the error case anymore:

https://searchfox.org/mozilla-central/source/dom/media/tests/mochitest/pc.js#802

Comment 15

[Byron Campen [:bwc]]

^

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Maybe it should? False positives can be a pain.

Comment 17

[Byron Campen [:bwc]]

Attachment: Part 0: Re-enable test for "relay" ICE policy

Make waitForIceConnected reject with an Error when ICE fails.

Comment 18

[Michael Froman [:mjf]]

Comment on attachment 9020804 [details] [diff] [review]
Part 1 (beta only): Pass enough args to NrIceCtx::Create

Looks good.

Comment 19

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48bbf85ab2a1b181772c6b141fb3841aab28a540

Comment 20

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51613279052549e24bf07259f0332a3edf492a56

Comment 21

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fc319aa70e90cf2933c2c72a530e06301cabec0
Bug 1502766 - Part 0: Re-enable test for "relay" ICE policy. r=jib

Comment 22

[Byron Campen [:bwc]]

ni self to request uplift for part 0 once it is on m-c

Comment 23

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2fc319aa70e9

Comment 24

[Adam Ullman]

 Great to see this has been fixed. I would like to turn my test back on for this. Is it going to be uplifted to Beta?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9020804 [details] [diff] [review]
Part 1 (beta only): Pass enough args to NrIceCtx::Create

[Triage Comment]
Fixes a regression in Fx64 which causes iceTransportPolicy to not work properly in 64. Approved for 64.0b6 (both patches).

Comment 26

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7749a380827a

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/88adf850f1d2

Comment 28

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 65.0a1 (2018-11-01) on Windows 10 x 64, Windows 7 x32, Mac OS X 10.13 and on Ubuntu 16.04 x64.

Comment 29

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox 64.0b6 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.13 and on Ubuntu 16.04 x64.