1297416 - Implement draft-ietf-rtcweb-ip-handling

Status: RESOLVED FIXED

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

Comment 1

[Byron Campen [:bwc]]

Attachment: Bug 1297416 - Part 1: Split out the hiding of host candidates, and default route mode.

Review commit: https://reviewboard.mozilla.org/r/73612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73612/

Comment 2

[Byron Campen [:bwc]]

Attachment: Bug 1297416 - Part 2: Disable default route mode if the user has granted camera or microphone permissions.

Review commit: https://reviewboard.mozilla.org/r/73614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73614/

Comment 3

[Byron Campen [:bwc]]

Attachment: Bug 1297416 - Part 3: Add proxy-only mode and pref.

Review commit: https://reviewboard.mozilla.org/r/73900/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73900/

Comment 4

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73612/diff/1-2/

Comment 5

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73614/diff/1-2/

Comment 6

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73612/diff/2-3/

Comment 7

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73614/diff/2-3/

Comment 8

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73900/diff/1-2/

Comment 9

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=528531760f59

Comment 10

[Byron Campen [:bwc]]

Attachment: Bug 1297416 - Part 4: Call close() when create() fails.

Review commit: https://reviewboard.mozilla.org/r/76378/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76378/

Comment 11

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73612/diff/3-4/

Comment 12

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73614/diff/3-4/

Comment 13

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73900/diff/2-3/

Comment 14

[Nils Ohlmeier [:drno]]

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?

Comment 15

[Byron Campen [:bwc]]

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

[Nils Ohlmeier [:drno]]

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?

Comment 17

[Byron Campen [:bwc]]

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

[Nils Ohlmeier [:drno]]

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?

Comment 19

[Nils Ohlmeier [:drno]]

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

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

LGTM

Comment 20

[Nils Ohlmeier [:drno]]

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.

Comment 21

[Byron Campen [:bwc]]

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?

Comment 22

[Nils Ohlmeier [:drno]]

(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 23

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73612/diff/4-5/

Comment 24

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73614/diff/4-5/

Comment 25

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73900/diff/3-4/

Comment 26

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76378/diff/1-2/

Comment 27

[Byron Campen [:bwc]]

(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 28

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73612/diff/5-6/

Comment 29

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73614/diff/5-6/

Comment 30

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73900/diff/4-5/

Comment 31

[Byron Campen [:bwc]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76378/diff/2-3/

Comment 32

[Byron Campen [:bwc]]

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

[Nils Ohlmeier [:drno]]

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

Comment 34

[Nils Ohlmeier [:drno]]

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

Comment 35

[Pulsebot]

Pushed by [e-mail]:
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

[Phil Ringnalda (:philor)]

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