1600109 - Fix captive portal and network change handling

Status: RESOLVED FIXED

(Firefox :: Security, defect, P1)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

The patches in bug 1598223 at the time of writing this comment implement the following flow:

1. Add a network change listener and a captive portal connectivity available listener. Keep a variable networkSettledTimeout for storing a timeout later.
2. When we get a network "up", if networkSettledTimeout is unset, set a 60 second timer and assign it to networkSettledTimeout. Otherwise, do nothing.
3. When we get a network "down", clear networkSettledTimeout if it's set.
4. When we get a captive portal connectivity available event, clear networkSettledTimeout if it's set, and run heuristics.
5. When the timer fires, run heuristics and clear networkSettledTimeout.

This seems ok except for two flaws,

1. We don't check/set the initial state at extension startup.
2. It's possible for the timer to fire before we get connectivity available, resulting in either running heuristics when we don't have connectivity or running it twice (once when the timer fires and once upon captive portal event).

This bug is about fixing these flaws. I think the following flow (at a high level) should work:

1. Add only the captive portal listener
2. Check the current state. Run heuristics if we have connectivity.
3. Run heuristics when the listener fires.

I don't think we need to worry about network link status at all, because the captive portal system is abstracting that away for us. But we might want to set the network listener anyway in case the user has disabled captive portal detection for some reason.

Dragana, what do you think?

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #0)

The patches in bug 1598223 at the time of writing this comment implement the following flow:

1. Add a network change listener and a captive portal connectivity available listener. Keep a variable networkSettledTimeout for storing a timeout later.
2. When we get a network "up", if networkSettledTimeout is unset, set a 60 second timer and assign it to networkSettledTimeout. Otherwise, do nothing.
3. When we get a network "down", clear networkSettledTimeout if it's set.
4. When we get a captive portal connectivity available event, clear networkSettledTimeout if it's set, and run heuristics.
5. When the timer fires, run heuristics and clear networkSettledTimeout.

This seems ok except for two flaws,

1. We don't check/set the initial state at extension startup.
2. It's possible for the timer to fire before we get connectivity available, resulting in either running heuristics when we don't have connectivity or running it twice (once when the timer fires and once upon captive portal event).

This bug is about fixing these flaws. I think the following flow (at a high level) should work:

1. Add only the captive portal listener
2. Check the current state. Run heuristics if we have connectivity.
3. Run heuristics when the listener fires.

I don't think we need to worry about network link status at all, because the captive portal system is abstracting that away for us. But we might want to set the network listener anyway in case the user has disabled captive portal detection for some reason.

Dragana, what do you think?

Had a chat with Dragana, Valentin, and Maxx (thanks!). Here's the new flow we established:

1. Check network state and captive portal state. If network up and not captive, run heuristics.
2. Add listener for network ID change. When it changes, run heuristics. Listen for captive portal state change.
   a. If we get a locked portal, wait for clear, then run heuristics again. The previous run would have been unsuccessful.
   b. If we get a clear portal, then all is well.

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1600109 - Implement setup function in head.js to set up prefs and telemetry, and call it in all tests. r=dragana!

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1600109 - Fix network change handling and test telemetry. r=dragana!

Depends on D58195

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

This likely won't make the 72 RC build but I'd like to keep an eye on this. Is this important to get into 72 or can it wait for 73?

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Thanks! This is blocking some QA concerns, but engineering is OK with this waiting for 73.

Reasons:

1. The changes are significant.
2. If we don't solve this, there are some extra telemetry events, some of which are marked incorrectly. We don't believe this to be a significant enough problem to mandate uplift.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Nihanth, are you still intending to work on this for 73 or 74?

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

This should land after a couple of review cycles and ideally we should uplift to 73.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76363bc85344
Implement setup function in head.js to set up prefs and telemetry, and call it in all tests. r=dragana,JuniorHsu
https://hg.mozilla.org/integration/autoland/rev/18ebd4ba0148
Fix network change handling and test telemetry. r=dragana

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/76363bc85344
https://hg.mozilla.org/mozilla-central/rev/18ebd4ba0148

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aff3f91d0931
Backed out 2 changesets for browser chrome failures on browser_policyOverride.js CLOSED TREE

Comment 12

[Cosmin Sabou [:CosminS]]

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&searchStr=windows%2C7%2Cshippable%2Copt%2Cmochitests%2Ctest-windows7-32-shippable%2Fopt-mochitest-browser-chrome-e10s-6%2Cm%28bc6%29&group_state=expanded&fromchange=951b824337c651f1dbb23ad19d56647541a81fb7&tochange=aff3f91d0931e798c9be59c1c2f84fab50ec4979&selectedJob=285441490

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=285434869&repo=autoland

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

[task 2020-01-17T23:17:32.802Z] 23:17:32 INFO - TEST-PASS | browser/extensions/doh-rollout/test/browser/browser_policyOverride.js | No change in TRR mode -
[task 2020-01-17T23:17:32.802Z] 23:17:32 INFO - Buffered messages finished
[task 2020-01-17T23:17:32.802Z] 23:17:32 INFO - TEST-UNEXPECTED-FAIL | browser/extensions/doh-rollout/test/browser/browser_policyOverride.js | Uncaught exception - at chrome://mochitests/content/browser/browser/extensions/doh-rollout/test/browser/head.js:73 - TypeError: can't access property "filter", events is undefined
[task 2020-01-17T23:17:32.802Z] 23:17:32 INFO - Stack trace:
[task 2020-01-17T23:17:32.802Z] 23:17:32 INFO - checkHeuristicsTelemetry@chrome://mochitests/content/browser/browser/extensions/doh-rollout/test/browser/head.js:73:3
[task 2020-01-17T23:17:32.802Z] 23:17:32 INFO - testPolicyOverride@chrome://mochitests/content/browser/browser/extensions/doh-rollout/test/browser/browser_policyOverride.js:51:27
[task 2020-01-17T23:17:32.802Z] 23:17:32 INFO - AsyncTester_execTest/<@chrome://mochikit/content/browser-test.js:1062:34
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - asyncTester_execTest@chrome://mochikit/content/browser-test.js:1097:11
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - nextTest/<@chrome://mochikit/content/browser-test.js:925:14
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:808:67
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - Leaving test bound testPolicyOverride
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - Console message: [JavaScript Error: "[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIControllers.removeController]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://global/content/elements/browser-custom-element.js :: destroy :: line 1374" data: no]"]
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - destroy@chrome://global/content/elements/browser-custom-element.js:1374:28
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - disconnectedCallback@chrome://global/content/elements/browser-custom-element.js:408:12
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - _releaseBrowser@resource://gre/modules/ExtensionParent.jsm:1436:18
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - shutdown@resource://gre/modules/ExtensionParent.jsm:1431:12
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - shutdown@chrome://extensions/content/parent/ext-backgroundPage.js:99:11
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - onShutdown@chrome://extensions/content/parent/ext-backgroundPage.js:165:19
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - ExtensionAPI/<@resource://gre/modules/ExtensionCommon.jsm:356:14
[task 2020-01-17T23:17:32.803Z] 23:17:32 INFO - wrapper@resource://gre/modules/ExtensionCommon.jsm:300:14
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - emit@resource://gre/modules/ExtensionCommon.jsm:327:32
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - emit@resource://gre/modules/Extension.jsm:1815:25
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - shutdown@resource://gre/modules/Extension.jsm:2464:10
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - shutdown@resource://gre/modules/Extension.jsm:1561:39
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - callBootstrapMethod@resource://gre/modules/addons/XPIProvider.jsm:1790:33
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - _shutdown@resource://gre/modules/addons/XPIProvider.jsm:1919:17
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - asyncshutdown@resource://gre/modules/addons/XPIProvider.jsm:1912:33
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - disable@resource://gre/modules/addons/XPIProvider.jsm:1933:18
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - updateAddonDisabledState@resource://gre/modules/addons/XPIDatabase.jsm:2572:25
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - setUserDisabled@resource://gre/modules/addons/XPIDatabase.jsm:623:25
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - disable@resource://gre/modules/addons/XPIDatabase.jsm:1136:27
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - resetPrefsAndRestartAddon@chrome://mochitests/content/browser/browser/extensions/doh-rollout/test/browser/head.js:119:15
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - asyncsetup/<@chrome://mochitests/content/browser/browser/extensions/doh-rollout/test/browser/head.js:65:11
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - nextTest@chrome://mochikit/content/browser-test.js:570:35
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - testScope/test_finish/<@chrome://mochikit/content/browser-test.js:1457:25
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO - run@chrome://mochikit/content/browser-test.js:1372:9
[task 2020-01-17T23:17:32.804Z] 23:17:32 INFO -
[task 2020-01-17T23:17:32.819Z] 23:17:32 INFO - GECKO(5024) | MEMORY STAT | vsize 745MB | vsizeMaxContiguous 452MB | residentFast 204MB | heapAllocated 82MB
[task 2020-01-17T23:17:32.819Z] 23:17:32 INFO - TEST-OK | browser/extensions/doh-rollout/test/browser/browser_policyOverride.js | took 15645ms

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged https://hg.mozilla.org/mozilla-central/rev/aff3f91d0931

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea650c48ddcb
Implement setup function in head.js to set up prefs and telemetry, and call it in all tests. r=dragana,JuniorHsu
https://hg.mozilla.org/integration/autoland/rev/0614bbf65c82
Fix network change handling and test telemetry. r=dragana

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ea650c48ddcb
https://hg.mozilla.org/mozilla-central/rev/0614bbf65c82

Comment 16

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 9117671 [details]
Bug 1600109 - Fix network change handling and test telemetry. r=dragana!

Beta/Release Uplift Approval Request

• User impact if declined: Extra superfluous telemetry events, and blocks other uplifts (bug 1608320 and bug 1609037).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Enable DoH on a new profile with some heuristics-failing condition (like a preset enterprise policy). Toggle the network status on the operating system and check telemetry. No telemetry should be sent upon network changes.
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): Functionality change so not low-risk. Covered by automated tests and QA support.
• String changes made/needed:

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9117670 [details]
Bug 1600109 - Implement setup function in head.js to set up prefs and telemetry, and call it in all tests. r=dragana!

Telemetry fixes for DoH rollout targeting 73. Blocks other DoH uplifts too. Approved for 73.0b9.

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/releases/mozilla-beta/rev/80a41f935b68
https://hg.mozilla.org/releases/mozilla-beta/rev/9fa4497af594

Comment 19

[Cosmin Sabou [:CosminS]]

Backed out (Bug 1600109, Bug 1609037, Bug 1608320) for turning Bug 1605297 into permafail.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=mochitest-browser-chrome-e10s-6%2Cwindows7-32-shippable&tochange=ca3381ffade1dcca914741a00ccba55a19b02142&group_state=expanded&fromchange=4287ea98207d8c830c876c64de5d475fdbafbc14&selectedJob=286029897

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=286029897&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/42fdbe58028de404df948fab374b86c1ae4daf41

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/42fdbe58028d

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/056e39833c96
Implement setup function in head.js to set up prefs and telemetry, and call it in all tests. r=dragana,JuniorHsu
https://hg.mozilla.org/integration/autoland/rev/9ca067132edf
Fix network change handling and test telemetry. r=dragana

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/056e39833c96
https://hg.mozilla.org/mozilla-central/rev/9ca067132edf

Comment 23

[Daniel Cicas [:dcicas], Release QA]

Hello,

I can conform that this issue is fixed on Fx 74.0a1 (BuildID: 202000127093737).