I am not sure captive portal detection is correct

[ddragana]

    async onReady(details) {
    // Now that we're here, stop listening to the captive portal
    browser.captivePortal.onStateChanged.removeListener(rollout.onReady);

    // Only proceed if we're not behind a captive portal
    if ((details.state !== "unlocked_portal") &&
        (details.state !== "not_captive")) {
      return;
    }

This code removes the listener then check if the captive portal has been cleared and if not it exits. So we do not wait for the captive portal any more and we do not perform heuristics...

I have not look at the code, but at a first glance it looks like we are not checking the captive portal on each network change event - we should. (maybe I am mistaking here I have not look at the code thoroughly)

[nhi-nguyen]

@maxxcrawford ^

[nhnt11]

This is fixed in my patchset to land the add-on in m-c. See https://phabricator.services.mozilla.com/D54626

You are right that the current code is not correct. The fix I implemented was to use the onConnectivityAvailable API instead of onStateChange.

[nhnt11]

@ddragana, added you as a reviewer on that patch.

[nhi-nguyen]

@maxxcrawford can this be closed now?

[maxxcrawford]

I recommend we do – per your comment about not merging the associated PR (#207) as it would become an issue for older browsers (FFv69) which some uses are still on.

Additionally, this fix has already landed in Nightly. Closing!