1603779 - Convert doh-rollout system add-on into a JSM

Status: RESOLVED FIXED

(Firefox :: Security, task, P1)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

We landed the DoH heuristics and opt-out UI in an add-on to aid with the imminent rollout. This is not ideal and we should pay our dues and convert the code to a JSM.

This will likely be done in the 75 cycle. 74 is looking tough and for 73 we're focusing on writing tests and fixing UX issues.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

I started working on this. It will land either in 75 or 76 - more likely 76.

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

My attention has since been diverted and at this point I'd probably rather start from scratch rather than resurrect my WIP. Unassigning for now, let me see if I can get this into the engineering plan for an upcoming release...

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1603779 - Part 1: Implement DoHController.jsm and DoHHeuristics.jsm and minimalize doh-rollout extension. r=prathiksha!,valentin!

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1603779 - Part 2: Move doh-rollout mochitests into browser/components/doh/test/. r=prathiksha!

Depends on D78598

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1603779 - Part 3: Update tests. r=prathiksha!

Depends on D82088

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1603779 - Part 4: Update remaining tests. r=prathiksha!

Depends on D82090

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1603779 - Part 5: Port test_migrations.js into two browser-chrome mochitests. r=prathiksha!

Depends on D82092

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1603779 - Part 4: Mass rename prefs in tests to remove DOH_ suffix. r=prathiksha!

Depends on D82093

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

A couple of notes for reviewers (needinfo'ing for visibility):

1. The doh-rollout add-on needs to continue to exist just so its local storage can continue to be read by our migration code. I've reduced it to a stub (just a manifest) and I'm looking into what it'll take to remove it completely - this'll be follow-up work.
2. There are a few changes in semantics that clean up and simplify the code - for example the "skipHeuristicsCheck" pref value is now something we recompute every time and we don't bother migrating old values etc. It only exists to suppress the doorhanger anyway, at this point. I'm going to file a follow-up to either rename it or find a way to get rid of it entirely.
3. Network link change/captive portal state change handling is implemented in parity with the old code - I'll rework that to use the network connectivity service in a follow-up.
4. The patches are split up to try and make it easy to follow the process of converting this code, for easy reviewing. I'll probably squash the commits into one before landing - I can do that immediately too if you prefer.

I think that's the important stuff. The rest, we can cover inline on phabricator :)

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ce918ed5d11201870dce2bd4fb844960f734cc6

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

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1603779 - Part 5: Make browser_connection_dnsoverhttps.js properly reset DoHController state between tests. r=prathiksha!

Depends on D82094

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Excitingly, the latest try run is looking totally green at least as far as DoH stuff goes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e972457623dc10e17db0ddc952a06fc9fd84fdcd

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1603779 - Part 6: Remove now-obsolete exceptions from .eslintrc.js. r=Gijs!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a75fac54cb2
Part 1: Implement DoHController.jsm and DoHHeuristics.jsm and minimalize doh-rollout extension. r=valentin
https://hg.mozilla.org/integration/autoland/rev/3809cc7bf203
Part 2: Move doh-rollout mochitests into browser/components/doh/test/. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/06f445e2ed29
Part 3: Update tests. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/17fac618c084
Part 4: Mass rename prefs in tests to remove DOH_ suffix. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/66f07eec311b
Part 5: Make browser_connection_dnsoverhttps.js properly reset DoHController state between tests. r=Gijs,preferences-reviewers
https://hg.mozilla.org/integration/autoland/rev/7341f7757a0b
Part 6: Remove now-obsolete exceptions from .eslintrc.js. r=Gijs

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8a75fac54cb2
https://hg.mozilla.org/mozilla-central/rev/3809cc7bf203
https://hg.mozilla.org/mozilla-central/rev/06f445e2ed29
https://hg.mozilla.org/mozilla-central/rev/17fac618c084
https://hg.mozilla.org/mozilla-central/rev/66f07eec311b
https://hg.mozilla.org/mozilla-central/rev/7341f7757a0b