Closed Bug 1662430 Opened 5 years ago Closed 5 years ago

Add a pref to toggle whether to clear doh-rollout.mode at shutdown

Categories

(Firefox :: Security, task, P1)

Product:
Firefox
Component:
Security
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
83 Branch
Iteration:
83.1 - Sept 21 - Oct 4
Tracking Flags:
Tracking Status
firefox82 --- fixed
firefox83 --- fixed

People

(Reporter: valentin, Assigned: nhnt11)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1662430 - Allow a pref to toggle whether to clear doh-rollout.mode at shutdown. r=valentin!
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
Details | Review
Bug 1662430 - [Beta only] Expect the correct events in DoH tests in beta. r=valentin!
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
Details | Review

Currently in DoHController.jsm we clear the TRR mode at shutdown.

We should reconsider doing this, as this means starting up the browser would always happen in mode0, followed by the heuristics and confirmation. This means we're skipping TRR for a lot of name resolutions that would otherwise use it during start-up.

I assume the reason we have it structured this way is that we can't be sure that when starting up we will be on the same network - but in my opinion that is by far the most common scenario. There's nothing preventing us from running heuristics after startup and clearing the mode if necessary. We should of course make this a pref to revert behaviour if any fallout occurs.

There is one scenario where this new behaviour might be problematic and that is with TRR steering. Since the steering URI is autodetected during heuristics, and not permanent, at startup we'd start in mode2 - use the default provider, then run the heuristics and switch to ISP's TRR server. Not great, but I think we can improve this.

Iteration: --- → 83.1 - Sept 21 - Oct 4
Pushed by nhnt11@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7e73d7a3ddd2 Allow a pref to toggle whether to clear doh-rollout.mode at shutdown. r=valentin
Pushed by nhnt11@gmail.com: https://hg.mozilla.org/integration/autoland/rev/fccbbed6fc57 Allow a pref to toggle whether to clear doh-rollout.mode at shutdown. r=valentin,preferences-reviewers

Tests are passing for me locally... hmm. Gotta dig into this.

Flags: needinfo?(nhnt11)

Happily, at least the browser_rollback.js failure is reproducible locally after I rebased the patch on the latest central tip! The preferences test could be breaking as a result of broken state left behind by that test perhaps... digging.

The DoHConfig.jsm patch landed before this which necessitated a couple of test updates. I'm expecting the preferences test to "just work" with these, but if it fails I'll know there's something strange going on (still can't repro that one locally on its own).

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

Preferences test still failing. New theory is that the change I made can result in a promise being created that's waiting on a pref observer that never trips - so the promise never resolves and the observer is never cleaned up.

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

That did the trick.

Pushed by nhnt11@gmail.com: https://hg.mozilla.org/integration/autoland/rev/bb81093f7e32 Allow a pref to toggle whether to clear doh-rollout.mode at shutdown. r=valentin,preferences-reviewers

Ugh, I'm not having a good time with this patch. I didn't push the updated patch to phabricator, so the old version of the patch that was failing got re-landed. My apologies to the trees for the wasted CPU cycles on this one...

Flags: needinfo?(nhnt11)
Pushed by nhnt11@gmail.com: https://hg.mozilla.org/integration/autoland/rev/718ec7f40f24 Allow a pref to toggle whether to clear doh-rollout.mode at shutdown. r=valentin,preferences-reviewers

https://hg.mozilla.org/mozilla-central/rev/718ec7f40f24

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox83: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch
Summary: Don't clear doh-rollout.mode at shutdown → Add a pref to toggle whether to clear doh-rollout.mode at shutdown
Component: Networking: DNS → Security
Product: Core → Firefox
Depends on: 1668104

Comment on attachment 9176876 [details]
Bug 1662430 - Allow a pref to toggle whether to clear doh-rollout.mode at shutdown. r=valentin!

Beta/Release Uplift Approval Request

  • User impact if declined: Currently we are seeing evidence that a significant number of DNS lookups that happen around startup are not using DoH because the heuristics haven't had time to run yet. By not clearing state at shutdown, we'll attempt to use DoH at startup if it was turned on in the previous session. The patch allows us to control this with a pref, and we want to use this to experiment on Release. In bug 1668104, we'll turn this on by default in Nightly and Beta.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This is not a simple cosmetic change, but it's also not very complex. Automated tests are in place. The change can be controlled via a pref.
  • String changes made/needed:
Attachment #9176876 - Flags: approval-mozilla-beta?
Blocks: 1668104
No longer depends on: 1668104

Comment on attachment 9176876 [details]
Bug 1662430 - Allow a pref to toggle whether to clear doh-rollout.mode at shutdown. r=valentin!

approved for 82.0b6

Attachment #9176876 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9176876 [details]
Bug 1662430 - Allow a pref to toggle whether to clear doh-rollout.mode at shutdown. r=valentin!

Clearing the approval flag so this doesn't show up as pending uplift.

Attachment #9176876 - Flags: approval-mozilla-beta+
Flags: needinfo?(nhnt11)

Comment on attachment 9179545 [details]
Bug 1662430 - [Beta only] Expect the correct events in DoH tests in beta. r=valentin!

Beta/Release Uplift Approval Request

  • User impact if declined: See comment 16
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: See comment 16
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): See comment 16
  • String changes made/needed:
Attachment #9179545 - Flags: approval-mozilla-beta?
Attachment #9176876 - Flags: approval-mozilla-beta?

Please note: D92398 is not landing in central. It's a patch that makes D90856 compatible with current beta.

Comment on attachment 9176876 [details]
Bug 1662430 - Allow a pref to toggle whether to clear doh-rollout.mode at shutdown. r=valentin!

let's try this again for 82.0b8.

Attachment #9176876 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9179545 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: