Closed Bug 1391857 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: drno, Assigned: mjf)

References

()

Details

Attachments

(1 file)

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.
Rank: 19
I'm going to look into this, if you don't mind.
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 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+
Pushed by mfroman@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/ef2f00ebda5c
fixing ctx flags for e10s stun addr gathering. r=drno
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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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+
You need to log in before you can comment on or make changes to this bug.