Firefox 64 does not respect the RTCConfiguration iceTransportPolicy

VERIFIED FIXED in Firefox 64

Status

()

defect
P2
normal
VERIFIED FIXED
9 months ago
9 months ago

People

(Reporter: adam, Assigned: bwc)

Tracking

({regression})

64 Branch
mozilla65
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 unaffected, firefox64 verified, firefox65 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
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?
Flags: needinfo?(docfaraday)
Lets assume this a regression until it's proven otherwise.
Keywords: regression
Priority: -- → P2
(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.
So it does seem to be using non-relay candidates in beta, but nightly uses only relay candidates. Something is fishy here.
Also, for some reason this test-case breaks the webrtc dev-tools panel.
^
Flags: needinfo?(mfroman)
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
Ok, found the problem. It is a promiscuous typecasting bug.
Assignee: nobody → docfaraday
Flags: needinfo?(docfaraday)
Attachment #9020804 - Flags: review?(mfroman)
Attachment #9020805 - Flags: review?(jib)
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.
Attachment #9020804 - Flags: approval-mozilla-beta?
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?
Attachment #9020805 - Flags: review?(jib) → review-
(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
^
Flags: needinfo?(jib)
Maybe it should? False positives can be a pain.
Flags: needinfo?(jib)
Make waitForIceConnected reject with an Error when ICE fails.
Attachment #9020805 - Attachment is obsolete: true
Attachment #9021258 - Flags: review?(jib)
Comment on attachment 9020804 [details] [diff] [review]
Part 1 (beta only): Pass enough args to NrIceCtx::Create

Looks good.
Flags: needinfo?(mfroman)
Attachment #9020804 - Flags: review?(mfroman) → review+
Attachment #9021258 - Flags: review?(jib) → review+
ni self to request uplift for part 0 once it is on m-c
Flags: needinfo?(docfaraday)
https://hg.mozilla.org/mozilla-central/rev/2fc319aa70e9
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
 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?
Blocks: 1493765
Flags: qe-verify+
Flags: in-testsuite+
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).
Flags: needinfo?(docfaraday)
Attachment #9020804 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.