[e10s] Gathering option 2 is broken if e10s is on

RESOLVED FIXED in Firefox 56

Status

()

Core
WebRTC: Networking
P1
normal
Rank:
19
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: drno, Assigned: mjf)

Tracking

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox55 wontfix, firefox56 fixed, firefox57 fixed)

Details

(URL)

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
The trickle ICE sample page https://webrtc.github.io/samples/src/content/peerconnection/trickle-ice/ shows only the IP interface of one interface if e10s is turned off, but all interfaces are gathered if e10s is on.

This breaks our default Option 2 of the gathering only the default route if the user hasn't given any gUM consent.
(Reporter)

Updated

4 months ago
Rank: 19
(Assignee)

Comment 1

4 months ago
I'm going to look into this, if you don't mind.
(Assignee)

Comment 2

4 months ago
The check for NR_ICE_CTX_FLAGS_ONLY_DEFAULT_ADDRS at [1] is different when on e10s and non-e10s.  Since this is almost certainly my fault, I'll keep digging.

[1] https://dxr.mozilla.org/mozilla-central/rev/7dddbd85047c6dc73ddbe1e423cd643a217845b3/media/mtransport/third_party/nICEr/src/ice/ice_ctx.c#749
Assignee: nobody → mfroman
Comment hidden (mozreview-request)
(Reporter)

Comment 4

4 months ago
mozreview-review
Comment on attachment 8899939 [details]
Bug 1391857 - fixing ctx flags for e10s stun addr gathering.

https://reviewboard.mozilla.org/r/171270/#review176502

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:749
(Diff revision 1)
> +    r_log(LOG_ICE, LOG_DEBUG,
> +          "ICE(%s): (ctx->flags & NR_ICE_CTX_FLAGS_ONLY_DEFAULT_ADDRS): %d\n",
> +          ctx->label,
> +          (int)(ctx->flags & NR_ICE_CTX_FLAGS_ONLY_DEFAULT_ADDRS?1:0));

Debug message? If not we should probably make it less obscure.
Attachment #8899939 - Flags: review?(drno) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

4 months ago
Pushed by mfroman@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/ef2f00ebda5c
fixing ctx flags for e10s stun addr gathering. r=drno
(Assignee)

Comment 8

4 months ago
Comment on attachment 8899939 [details]
Bug 1391857 - fixing ctx flags for e10s stun addr gathering.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1345511
[User impact if declined]: Machines with multiple interfaces will have more than the default local stun address exposed with ice candidates are gathered.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: It is low risk.
[Why is the change risky/not risky?]: One method was added that sets flags we're already setting later on the ice context.  The flags needed to be set earlier in the processing in e10s mode.
[String changes made/needed]: n/a
Attachment #8899939 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/ef2f00ebda5c
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
status-firefox56: --- → affected
status-firefox55: --- → wontfix
Comment on attachment 8899939 [details]
Bug 1391857 - fixing ctx flags for e10s stun addr gathering.

"Machines with multiple interfaces will have more than the default local stun address exposed with ice candidates are gathered." OK, I get this is something to do with WebRTC networking but no idea what this sentence means as far as user impact. It seems to have regressed in 55 though. 

Let's uplift this patch for beta 7 on the assumption it is important for some WebRTC users.
Attachment #8899939 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 11

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/b20ed3e70390
status-firefox56: affected → fixed
You need to log in before you can comment on or make changes to this bug.