Implement draft-ietf-rtcweb-ip-handling

RESOLVED FIXED in Firefox 51

Status

()

Core
WebRTC: Networking
P1
normal
Rank:
15
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: bwc, Assigned: bwc)

Tracking

(Blocks: 1 bug)

48 Branch
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

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

Attachments

(4 attachments)

Comment hidden (empty)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=528531760f59
Rank: 15
Priority: P2 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8784008 - Flags: review?(drno)
Attachment #8784009 - Flags: review?(drno)
Attachment #8784434 - Flags: review?(drno)
Attachment #8787686 - Flags: review?(drno)

Comment 14

a year ago
mozreview-review
Comment on attachment 8784008 [details]
Bug 1297416 - Part 1: Split out the hiding of host candidates, and default route mode.

https://reviewboard.mozilla.org/r/73612/#review75552

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.h:167
(Diff revision 4)
>  #define NR_ICE_CTX_FLAGS_ANSWERER                          (1<<1)
>  #define NR_ICE_CTX_FLAGS_AGGRESSIVE_NOMINATION             (1<<2)
>  #define NR_ICE_CTX_FLAGS_LITE                              (1<<3)
>  #define NR_ICE_CTX_FLAGS_RELAY_ONLY                        (1<<4)
> -#define NR_ICE_CTX_FLAGS_ONLY_DEFAULT_ADDRS                (1<<5)
> +#define NR_ICE_CTX_FLAGS_HIDE_HOST_CANDIDATES              (1<<5)
> +#define NR_ICE_CTX_FLAGS_ONLY_DEFAULT_ADDRS                (1<<6)

Can NR_ICE_CTX_FLAGS_ONLY_DEFAULT_ADDRS get removed?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:485
(Diff revision 4)
>        break;
>      case dom::RTCIceTransportPolicy::Relay:
>        setIceTransportPolicy(NrIceCtx::ICE_POLICY_RELAY);
>        break;
>      case dom::RTCIceTransportPolicy::All:
> +      if (Preferences::GetBool("media.peerconnection.ice.no_host", false)) {

Is this on purpose a hidden pref?
(Assignee)

Comment 15

a year ago
mozreview-review-reply
Comment on attachment 8784008 [details]
Bug 1297416 - Part 1: Split out the hiding of host candidates, and default route mode.

https://reviewboard.mozilla.org/r/73612/#review75552

> Can NR_ICE_CTX_FLAGS_ONLY_DEFAULT_ADDRS get removed?

No, that's orthogonal hiding host candidates in the draft.

> Is this on purpose a hidden pref?

I could go either way on it.

Comment 16

a year ago
mozreview-review
Comment on attachment 8784009 [details]
Bug 1297416 - Part 2: Disable default route mode if the user has granted camera or microphone permissions.

https://reviewboard.mozilla.org/r/73614/#review75574

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:784
(Diff revision 4)
>    bool default_address_only = Preferences::GetBool(
> -    "media.peerconnection.ice.default_address_only", true);
> +    "media.peerconnection.ice.default_address_only",
> +    !MediaManager::Get()->IsActivelyCapturingOrHasAPermission(winId));

Is this on purpose now taking a hidden user pref into account?
Or could this be replaced with just a IsActivelyCapturingOrHasAPermission() call?
(Assignee)

Comment 17

a year ago
mozreview-review-reply
Comment on attachment 8784009 [details]
Bug 1297416 - Part 2: Disable default route mode if the user has granted camera or microphone permissions.

https://reviewboard.mozilla.org/r/73614/#review75574

> Is this on purpose now taking a hidden user pref into account?
> Or could this be replaced with just a IsActivelyCapturingOrHasAPermission() call?

Oh, right. So the behavior I'm shooting for here is IsActivelyCapturingOrHasAPermission is what counts by default, but if the pref is set (either to true or false) it takes precedence.

Comment 18

a year ago
mozreview-review
Comment on attachment 8784434 [details]
Bug 1297416 - Part 3: Add proxy-only mode and pref.

https://reviewboard.mozilla.org/r/73900/#review75584

LGTM

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:799
(Diff revision 3)
> +PeerConnectionMedia::GetPrefProxyOnly() const
> +{
> +  ASSERT_ON_THREAD(mMainThread); // will crash on STS thread
> +
> +#if !defined(MOZILLA_EXTERNAL_LINKAGE)
> +  return Preferences::GetBool("media.peerconnection.ice.proxy_only", false);

On purpose a hidden user pref?
Attachment #8784434 - Flags: review?(drno) → review+

Comment 19

a year ago
mozreview-review
Comment on attachment 8787686 [details]
Bug 1297416 - Part 4: Call close() when create() fails.

https://reviewboard.mozilla.org/r/76378/#review75590

LGTM
Attachment #8787686 - Flags: review?(drno) → review+

Comment 20

a year ago
mozreview-review-reply
Comment on attachment 8784009 [details]
Bug 1297416 - Part 2: Disable default route mode if the user has granted camera or microphone permissions.

https://reviewboard.mozilla.org/r/73614/#review75574

> Oh, right. So the behavior I'm shooting for here is IsActivelyCapturingOrHasAPermission is what counts by default, but if the pref is set (either to true or false) it takes precedence.

Having the ability to over write the IsActivelyCapturingOrHasAPermission() result makes sense. I think I would prefer it for the sake of full disclosure if the user prefs around this whole topic are going to be public and not hidden. But I don't feel super strong about it.
(Assignee)

Comment 21

a year ago
mozreview-review-reply
Comment on attachment 8784009 [details]
Bug 1297416 - Part 2: Disable default route mode if the user has granted camera or microphone permissions.

https://reviewboard.mozilla.org/r/73614/#review75574

> Having the ability to over write the IsActivelyCapturingOrHasAPermission() result makes sense. I think I would prefer it for the sake of full disclosure if the user prefs around this whole topic are going to be public and not hidden. But I don't feel super strong about it.

Is it possible to have a public boolean pref that is not set to anything by default?
(In reply to Byron Campen [:bwc] from comment #21)
> Is it possible to have a public boolean pref that is not set to anything by
> default?

That is a good question. Although even if you would be able to store it uninitialized in all.js you would not be able to load it into a bool.

Looking at this: 'grep pref modules/libpref/init/all.js | cut -d ',' -f 2 | sort | uniq'
it looks like your best bet for an uninitialized user pref is the empty string "". Or use an integer with 0 as uninitialized and 1 and 2 for on and off.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 27

a year ago
(In reply to Nils Ohlmeier [:drno] from comment #22)
> (In reply to Byron Campen [:bwc] from comment #21)
> > Is it possible to have a public boolean pref that is not set to anything by
> > default?
> 
> That is a good question. Although even if you would be able to store it
> uninitialized in all.js you would not be able to load it into a bool.
> 
> Looking at this: 'grep pref modules/libpref/init/all.js | cut -d ',' -f 2 |
> sort | uniq'
> it looks like your best bet for an uninitialized user pref is the empty
> string "". Or use an integer with 0 as uninitialized and 1 and 2 for on and
> off.

Eh, I'm not much of a fan of either of those options. I suppose we could instead have the no_host pref have no effect if set to false.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 32

a year ago
mozreview-review-reply
Comment on attachment 8784009 [details]
Bug 1297416 - Part 2: Disable default route mode if the user has granted camera or microphone permissions.

https://reviewboard.mozilla.org/r/73614/#review75574

> Is it possible to have a public boolean pref that is not set to anything by default?

Ok, I've changed this so that a setting of false has no effect; the pref cannot be used to force Firefox to grant access to all interfaces regardless of permissions.

Comment 33

a year ago
mozreview-review
Comment on attachment 8784008 [details]
Bug 1297416 - Part 1: Split out the hiding of host candidates, and default route mode.

https://reviewboard.mozilla.org/r/73612/#review75576

LGTM
Attachment #8784008 - Flags: review?(drno) → review+

Comment 34

a year ago
mozreview-review
Comment on attachment 8784009 [details]
Bug 1297416 - Part 2: Disable default route mode if the user has granted camera or microphone permissions.

https://reviewboard.mozilla.org/r/73614/#review76428

LGTM
Attachment #8784009 - Flags: review?(drno) → review+

Comment 35

a year ago
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f5a249215f6
Part 1: Split out the hiding of host candidates, and default route mode. r=drno
https://hg.mozilla.org/integration/autoland/rev/ea1469f88272
Part 2: Disable default route mode if the user has granted camera or microphone permissions. r=drno
https://hg.mozilla.org/integration/autoland/rev/f17f59899b31
Part 3: Add proxy-only mode and pref. r=drno
https://hg.mozilla.org/integration/autoland/rev/57647be72e76
Part 4: Call close() when create() fails. r=drno

Comment 36

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2f5a249215f6
https://hg.mozilla.org/mozilla-central/rev/ea1469f88272
https://hg.mozilla.org/mozilla-central/rev/f17f59899b31
https://hg.mozilla.org/mozilla-central/rev/57647be72e76
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1304600
Depends on: 1312304
See Also: → bug 1320595
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1320595
You need to log in before you can comment on or make changes to this bug.