1591533 - GV API to toggle and configure DoH TRR

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P3)

Description

[Chris Peterson [:cpeterson]]

Fenix wants to be able to toggle and configure Gecko DoH. Emily says this would be a new API on GeckoRuntimeSettings.

vgosu says: "The implementation should work on Fenix with a simple flip of the pref network.trr.mode=2. There are a few differences though:

• on Android we're missing captive portal detection, so that will not factor into the decision whether to use TRR or not - a small chance it'll cause issues in some captive portals.
• the long lived keep-alive connection we use for TRR might cause extra battery usage. An experiment would be useful in that regard.
• Some UI will of course be necessary, since about:preferences does not work in Fenix (AFAIK) and users who want to opt out should not be forced to use about:config for the task

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Chris Peterson [:cpeterson] from comment #0)

Fenix wants to be able to toggle and configure Gecko DoH. Emily says this would be a new API on GeckoRuntimeSettings.

vgosu says: "The implementation should work on Fenix with a simple flip of the pref network.trr.mode=2. There are a few differences though:

• on Android we're missing captive portal detection, so that will not factor into the decision whether to use TRR or not - a small chance it'll cause issues in some captive portals.

Android generally will not activate a wifi connection until after the captive portal has been passed. It uses an internal WebView instance for this. Users can choose to use the connection as-is, but it's probably a fairly unlikely occurrence.

• the long lived keep-alive connection we use for TRR might cause extra battery usage. An experiment would be useful in that regard.

Yikes. We should shut that socket down when the app is in the background.

• Some UI will of course be necessary, since about:preferences does not work in Fenix (AFAIK) and users who want to opt out should not be forced to use about:config for the task

Yeah, presumably Fenix will take care of that.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #1)

• on Android we're missing captive portal detection, so that will not factor into the decision whether to use TRR or not - a small chance it'll cause issues in some captive portals.

Android generally will not activate a wifi connection until after the captive portal has been passed. It uses an internal WebView instance for this. Users can choose to use the connection as-is, but it's probably a fairly unlikely occurrence.

That is great!

• the long lived keep-alive connection we use for TRR might cause extra battery usage. An experiment would be useful in that regard.

Yikes. We should shut that socket down when the app is in the background.

I am not sure if we have a way to do that at the moment. I don't think we currently close keep-alive connections when in the background (it seems we don't but I could be wrong) - the TRR connection is only special in the fact that it has a 10 times larger keepalive. We should probably have a separate bug for that.

Comment 3

[cschanaj]

Attachment: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix

This PR adds a couple GeckoView API to setup and specific DoH TRR mode
and server URI, which enables the DNS-over-HTTPS capability on Firefox
Fenix

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #2)

I am not sure if we have a way to do that at the moment. I don't think we currently close keep-alive connections when in the background (it seems we don't but I could be wrong) - the TRR connection is only special in the fact that it has a 10 times larger keepalive. We should probably have a separate bug for that.

Hey Valentin, has anything changed since you wrote this comment? could we get a bug opened for this? :)

Comment 5

[Valentin Gosu [:valentin] (he/him)]

I've filed bug 1724166 for this.

Comment 6

[cschanaj]

Hi, is there anything I can help with progressing this PR? I am very keen on having TRR as a new feature on Fenix.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 8

[Andrew Creskey [:acreskey]]

There is a performance concern with our current DoH implementation:
When DoH is rolled out in a region, users are pinned to a single regional TRR (with network provider steering being an exception).
But when the user travels with their device the DNS queries are still made to their home region TRR.

Depending on how far from their regional TRR they travel, the increased latency could be significant.
It would be unfortunate if Firefox for android became noticeably slower than the competition when used for personal or business travel.

Comment 9

[cschanaj]

gald to new updates on this ticket, I will update the patch in a few days. thanks.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

(In reply to Andrew Creskey [:acreskey] [he/him] from comment #8)

Depending on how far from their regional TRR they travel, the increased latency could be significant.
It would be unfortunate if Firefox for android became noticeably slower than the competition when used for personal or business travel.

Good point. I think the solution would be to not rollout DoH to Android by default just yet. However, having an API & interface to enable it manually is not blocked on that.

Comment 11

[cschanaj]

Attachment: WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix

Comment 12

[cschanaj]

(In reply to Valentin Gosu [:valentin] (he/him) from comment #10)

(In reply to Andrew Creskey [:acreskey] [he/him] from comment #8)

Depending on how far from their regional TRR they travel, the increased latency could be significant.
It would be unfortunate if Firefox for android became noticeably slower than the competition when used for personal or business travel.

Good point. I think the solution would be to not rollout DoH to Android by default just yet. However, having an API & interface to enable it manually is not blocked on that.

Totally agree. I can imagine there are cases where some users prefer additional security and privacy over internet speed, e.g. upon traveling to regions with government internet censorship / DNS poisoning. It is highly valuable if an user is able to turn DoH on when needed. Even if Mozilla would like to discourage the DoH uses due to the performance concerns, it is not necessary to block API & interface changes.

I updated the differentials in https://phabricator.services.mozilla.com/D121455, and I would like to request prioritizing its review if possible .

Comment 13

[cschanaj]

My patch is passing tests and ready for review. This is a discrepancy of an important security feature of Firefox desktop and Fenix, shall we make this bug priority P1/ P2?

Comment 14

[Chris Peterson [:cpeterson]]

Thanks for your patience! I'm asking the Android engineers to recommend an appropriate code reviewer.

Note that we don't have plans to enable DoH in Fenix or Focus at this time due to performance concerns, but we would like this API so other GeckoView apps can use DoH if they choose.

Comment 15

[cschanaj]

Thanks. I'm looking forward to the code review to take place.

Comment 16

[cschanaj]

(In reply to Chris Peterson [:cpeterson] from comment #14)

Thanks for your patience! I'm asking the Android engineers to recommend an appropriate code reviewer.

Note that we don't have plans to enable DoH in Fenix or Focus at this time due to performance concerns, but we would like this API so other GeckoView apps can use DoH if they choose.

May I know if there is any updates?

Comment 17

[Chris Peterson [:cpeterson]]

Tasks and enhancements should have severity N/A.

Comment 18

[cschanaj]

Attachment: WIP: Bug 1591533 - .

Comment 19

[cschanaj]

Attachment: Bug 1591533 - rebase

Depends on D121455

Comment 20

[cschanaj]

Attachment: Bug 1591533 - .

Depends on D181939

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3f735a759aae
Add GV API to enable DNS-over-HTTPS capability on Fenix r=geckoview-reviewers,calu,owlish

Comment 22

[Sandor Molnar[:smolnar]]

Backed out for causing gv-junit failures on RuntimeSettingsTest#largeKeepaliveFactor

Backout link: https://hg.mozilla.org/integration/autoland/rev/54de5850bdfbf233483c525862a4c48ef767e14c

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.RuntimeSettingsTest#largeKeepaliveFactor | java.lang.IllegalArgumentException: largeKeepaliveFactor must be between 1 to 10 inclusive

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5d973fcd59c
Add GV API to enable DNS-over-HTTPS capability on Fenix r=geckoview-reviewers,calu,owlish

Comment 24

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f5d973fcd59c

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c1aa8dfe1a1
Add GV API to enable DNS-over-HTTPS capability on Fenix r=geckoview-reviewers,calu,owlish

Comment 26

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/8c1aa8dfe1a1

Comment 27

[Chris Peterson [:cpeterson]]

Clearing outdated needinfo for elizabbennet from comment 22 when this patch was backed out five months ago.

No need to uplift this new GV API to Beta 123.