Closed Bug 1600109 Opened 3 years ago Closed 2 years ago

Fix captive portal and network change handling

Categories

(Firefox :: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 74
Tracking Status
firefox72 - wontfix
firefox73 + fixed
firefox74 + verified

People

(Reporter: nhnt11, Assigned: nhnt11)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

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?

Flags: needinfo?(dd.mozilla)

(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.
Flags: needinfo?(dd.mozilla)
Component: General → Security
Duplicate of this bug: 1605020

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?

Flags: needinfo?(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.
Flags: needinfo?(nhnt11)

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

Flags: needinfo?(nhnt11)

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

Flags: needinfo?(nhnt11)
Attachment #9117671 - Attachment description: Bug 1600109 - Fix network change handling and test telemetry. r=JuniorHsu!,dragana! → Bug 1600109 - Fix network change handling and test telemetry. r=dragana!
Depends on: 1609028
Pushed by nhnt11@gmail.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aff3f91d0931
Backed out 2 changesets for browser chrome failures on browser_policyOverride.js CLOSED TREE

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 - async
Tester_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 - async
setup/<@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

Status: RESOLVED → REOPENED
Flags: needinfo?(nhnt11)
Resolution: FIXED → ---
Target Milestone: Firefox 74 → ---
Attachment #9117670 - Attachment description: Bug 1600109 - Implement setup function in head.js to set up prefs and telemetry, and call it in all tests. r=dragana!,juniorhsu! → Bug 1600109 - Implement setup function in head.js to set up prefs and telemetry, and call it in all tests. r=dragana!
Pushed by nhnt11@gmail.com:
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
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74

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:
Flags: needinfo?(nhnt11)
Attachment #9117671 - Flags: approval-mozilla-beta?
Attachment #9117670 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Attachment #9117670 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9117671 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Pushed by nhnt11@gmail.com:
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
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74

Hello,

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

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