Closed Bug 1591533 Opened 5 years ago Closed 6 months ago

GV API to toggle and configure DoH TRR

Categories

(GeckoView :: General, enhancement, P3)

All
Android
enhancement

Tracking

(firefox118 wontfix, firefox119 fixed, firefox122 wontfix, firefox123 wontfix, firefox124 fixed)

RESOLVED FIXED
119 Branch
Tracking Status
firefox118 --- wontfix
firefox119 --- fixed
firefox122 --- wontfix
firefox123 --- wontfix
firefox124 --- fixed

People

(Reporter: cpeterson, Assigned: elizabbennet)

References

(Blocks 1 open bug)

Details

(Whiteboard: [trr])

Attachments

(1 file, 4 obsolete files)

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
Rank: 25
Priority: -- → P2

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

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (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.

Blocks: 1434852
Whiteboard: [trr]
Priority: P2 → P3

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

Assignee: nobody → elizabbennet
Status: NEW → ASSIGNED

(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? :)

Flags: needinfo?(valentin.gosu)

I've filed bug 1724166 for this.

Depends on: 1724166
Flags: needinfo?(valentin.gosu)

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

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

Assignee: elizabbennet → nobody
Status: ASSIGNED → NEW
Severity: normal → --
Priority: P3 → --

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.

Severity: -- → S3
Rank: 25
Priority: -- → P3
Assignee: nobody → elizabbennet
Status: NEW → ASSIGNED
Assignee: elizabbennet → nobody
Status: ASSIGNED → NEW

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

Assignee: nobody → elizabbennet
Status: NEW → ASSIGNED

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

Attachment #9300939 - Attachment description: WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix → Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix
Attachment #9300939 - Attachment description: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix → WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix
Attachment #9234106 - Attachment description: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix → WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix
Attachment #9234106 - Attachment description: WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix → Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix
Attachment #9300939 - Attachment is obsolete: true

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

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

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?

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.

Blocks: 1801530
No longer blocks: doh

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

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

Tasks and enhancements should have severity N/A.

Severity: S3 → N/A
Attachment #9234106 - Attachment description: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix → WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix
Attachment #9234106 - Attachment description: WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix → Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix
Attached file WIP: Bug 1591533 - . (obsolete) —
Attachment #9340865 - Attachment is obsolete: true
Attachment #9234106 - Attachment description: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix → WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix
Attached file Bug 1591533 - rebase (obsolete) —

Depends on D121455

Attached file Bug 1591533 - . (obsolete) —

Depends on D181939

Attachment #9234106 - Attachment description: WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix → Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix
Attachment #9340866 - Attachment description: WIP: Bug 1591533 - rebase → Bug 1591533 - rebase
Attachment #9340867 - Attachment description: WIP: Bug 1591533 - . → Bug 1591533 - .
Attachment #9340866 - Attachment is obsolete: true
Attachment #9340867 - Attachment is obsolete: true
Attachment #9234106 - Attachment description: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix → WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix
Attachment #9234106 - Attachment description: WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix → Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix
Attachment #9234106 - Attachment description: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix → WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix
Attachment #9234106 - Attachment description: WIP: Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix → Bug 1591533 - Add GV API to enable DNS-over-HTTPS capability on Fenix
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3f735a759aae
Add GV API to enable DNS-over-HTTPS capability on Fenix r=geckoview-reviewers,calu,owlish
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f5d973fcd59c
Add GV API to enable DNS-over-HTTPS capability on Fenix r=geckoview-reviewers,calu,owlish
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 119 Branch
Regressions: 1854216
See Also: → 1854216
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8c1aa8dfe1a1
Add GV API to enable DNS-over-HTTPS capability on Fenix r=geckoview-reviewers,calu,owlish
Status: REOPENED → RESOLVED
Closed: 10 months ago6 months ago
Resolution: --- → FIXED

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.

Flags: needinfo?(elizabbennet)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: